4:22 p.m.
This miniseries converts the functions for reserving PCI addresses
to use virDevicePCIAddress which we use in virDeviceInfo for PCI addresses
(not to be confused with virPCIDeviceAddress from virpci.h used for host
devices which should probably be merged with virDevicePCIAddress in a followup
patch) and it makes them accept bus numbers up to 'maxbus', which should be set
when creating the address set if multiple buses are allowed.
Right now it's initialized to zero and these patches have no effect.
Ján Tomko (2):
qemu: switch PCI address alocation to use virDevicePCIAddress
qemu: allow multiple buses in PCI address alocation
src/qemu/qemu_command.c | 288 ++++++++++++++++++++++--------------------------
src/qemu/qemu_command.h | 13 +--
src/qemu/qemu_hotplug.c | 16 +--
3 files changed, 147 insertions(+), 170 deletions(-)
--
1.7.12.4
4:22 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: switch PCI address alocation to use virDevicePCIAddress
Some functions were using virDomainDeviceInfo where virDevicePCIAddress
would suffice. Some were only using integers for slots and functions,
assuming the bus numbers are always 0.
Switch from virDomainDeviceInfoPtr to virDevicePCIAddressPtr:
qemuPCIAddressAsString
qemuDomainPCIAddressCheckSlot
qemuDomainPCIAddressReserveAddr
qemuDomainPCIAddressReleaseAddr
Switch from int slot to virDevicePCIAddressPtr:
qemuDomainPCIAddressReserveSlot
qemuDomainPCIAddressReleaseSlot
qemuDomainPCIAddressGetNextSlot
Deleted functions (they would take the same parameters
as ReserveAddr/ReleaseAddr do now.)
qemuDomainPCIAddressReserveFunction
qemuDomainPCIAddressReleaseFunction
---
src/qemu/qemu_command.c | 267 +++++++++++++++++++++---------------------------
src/qemu/qemu_command.h | 13 +--
src/qemu/qemu_hotplug.c | 16 +--
3 files changed, 127 insertions(+), 169 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c28123..b50e779 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -953,30 +953,30 @@ cleanup:
#define QEMU_PCI_ADDRESS_LAST_FUNCTION 8
struct _qemuDomainPCIAddressSet {
virHashTablePtr used;
- int nextslot;
+ virDevicePCIAddress lastaddr;
};
-static char *qemuPCIAddressAsString(virDomainDeviceInfoPtr dev)
+static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
{
- char *addr;
+ char *str;
- if (dev->addr.pci.domain != 0 ||
- dev->addr.pci.bus != 0) {
+ if (addr->domain != 0 ||
+ addr->bus != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Only PCI domain 0 and bus 0 are available"));
return NULL;
}
- if (virAsprintf(&addr, "%d:%d:%d.%d",
- dev->addr.pci.domain,
- dev->addr.pci.bus,
- dev->addr.pci.slot,
- dev->addr.pci.function) < 0) {
+ if (virAsprintf(&str, "%d:%d:%d.%d",
+ addr->domain,
+ addr->bus,
+ addr->slot,
+ addr->function) < 0) {
virReportOOMError();
return NULL;
}
- return addr;
+ return str;
}
@@ -999,7 +999,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
}
- addr = qemuPCIAddressAsString(info);
+ addr = qemuPCIAddressAsString(&info->addr.pci);
if (!addr)
goto cleanup;
@@ -1024,12 +1024,11 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
if ((info->addr.pci.function == 0) &&
(info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) {
/* a function 0 w/o multifunction=on must reserve the entire slot */
- int function;
- virDomainDeviceInfo temp_info = *info;
+ virDevicePCIAddress tmp_addr = info->addr.pci;
+ unsigned int *func = &tmp_addr.function;
- for (function = 1; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- temp_info.addr.pci.function = function;
- addr = qemuPCIAddressAsString(&temp_info);
+ for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ addr = qemuPCIAddressAsString(&tmp_addr);
if (!addr)
goto cleanup;
@@ -1142,90 +1141,75 @@ error:
* is used by the other device.
*/
static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev)
+ virDevicePCIAddressPtr addr)
{
- char *addr;
- virDomainDeviceInfo temp_dev;
- int function;
-
- temp_dev = *dev;
- for (function = 0; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- temp_dev.addr.pci.function = function;
- addr = qemuPCIAddressAsString(&temp_dev);
- if (!addr)
+ char *str;
+ virDevicePCIAddress tmp_addr = *addr;
+ unsigned int *func = &(tmp_addr.function);
+
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ str = qemuPCIAddressAsString(&tmp_addr);
+ if (!str)
return -1;
- if (virHashLookup(addrs->used, addr)) {
- VIR_FREE(addr);
+ if (virHashLookup(addrs->used, str)) {
+ VIR_FREE(str);
return -1;
}
- VIR_FREE(addr);
+ VIR_FREE(str);
}
return 0;
}
int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev)
+ virDevicePCIAddressPtr addr)
{
- char *addr;
+ char *str;
- addr = qemuPCIAddressAsString(dev);
- if (!addr)
+ str = qemuPCIAddressAsString(addr);
+ if (!str)
return -1;
- VIR_DEBUG("Reserving PCI addr %s", addr);
+ VIR_DEBUG("Reserving PCI addr %s", str);
- if (virHashLookup(addrs->used, addr)) {
+ if (virHashLookup(addrs->used, str)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to reserve PCI address %s"), addr);
- VIR_FREE(addr);
+ _("unable to reserve PCI address %s"), str);
+ VIR_FREE(str);
return -1;
}
- if (virHashAddEntry(addrs->used, addr, addr)) {
- VIR_FREE(addr);
+ if (virHashAddEntry(addrs->used, str, str)) {
+ VIR_FREE(str);
return -1;
}
- if (dev->addr.pci.slot > addrs->nextslot) {
- addrs->nextslot = dev->addr.pci.slot + 1;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < addrs->nextslot)
- addrs->nextslot = 0;
- }
-
+ addrs->lastaddr = *addr;
+ addrs->lastaddr.function = 0;
+ addrs->lastaddr.multi = 0;
return 0;
}
-int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function)
-{
- virDomainDeviceInfo dev;
-
- dev.addr.pci.domain = 0;
- dev.addr.pci.bus = 0;
- dev.addr.pci.slot = slot;
- dev.addr.pci.function = function;
-
- return qemuDomainPCIAddressReserveAddr(addrs, &dev);
-}
-
int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
- int slot)
+ virDevicePCIAddressPtr addr)
{
- int function;
+ virDevicePCIAddress tmp_addr = *addr;
+ unsigned int *func = &tmp_addr.function;
+ int i;
- for (function = 0; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- if (qemuDomainPCIAddressReserveFunction(addrs, slot, function) < 0)
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0)
goto cleanup;
}
return 0;
cleanup:
- for (function--; function >= 0; function--) {
- qemuDomainPCIAddressReleaseFunction(addrs, slot, function);
+ for (i = *func; i >= 0; i--) {
+ *func = i;
+ qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr);
}
return -1;
}
@@ -1245,7 +1229,7 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr
addrs,
return -1;
}
- ret = qemuDomainPCIAddressReserveSlot(addrs, dev->addr.pci.slot);
+ ret = qemuDomainPCIAddressReserveSlot(addrs, &dev->addr.pci);
} else {
ret = qemuDomainPCIAddressSetNextAddr(addrs, dev);
}
@@ -1254,59 +1238,43 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr
addrs,
int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev)
+ virDevicePCIAddressPtr addr)
{
- char *addr;
+ char *str;
int ret;
- addr = qemuPCIAddressAsString(dev);
- if (!addr)
+ str = qemuPCIAddressAsString(addr);
+ if (!str)
return -1;
- ret = virHashRemoveEntry(addrs->used, addr);
+ ret = virHashRemoveEntry(addrs->used, str);
- VIR_FREE(addr);
+ VIR_FREE(str);
return ret;
}
-int qemuDomainPCIAddressReleaseFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function)
+int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
{
- virDomainDeviceInfo dev;
-
- dev.addr.pci.domain = 0;
- dev.addr.pci.bus = 0;
- dev.addr.pci.slot = slot;
- dev.addr.pci.function = function;
-
- return qemuDomainPCIAddressReleaseAddr(addrs, &dev);
-}
-
-int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot)
-{
- virDomainDeviceInfo dev;
- char *addr;
+ char *str;
int ret = 0;
- unsigned int *function = &dev.addr.pci.function;
-
- dev.addr.pci.domain = 0;
- dev.addr.pci.bus = 0;
- dev.addr.pci.slot = slot;
+ virDevicePCIAddress tmp_addr = *addr;
+ unsigned int *func = &tmp_addr.function;
- for (*function = 0; *function < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*function)++) {
- addr = qemuPCIAddressAsString(&dev);
- if (!addr)
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ str = qemuPCIAddressAsString(&tmp_addr);
+ if (!str)
return -1;
- if (!virHashLookup(addrs->used, addr)) {
- VIR_FREE(addr);
+ if (!virHashLookup(addrs->used, str)) {
+ VIR_FREE(str);
continue;
}
- VIR_FREE(addr);
+ VIR_FREE(str);
- if (qemuDomainPCIAddressReleaseFunction(addrs, slot, *function) < 0)
+ if (qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr) < 0)
ret = -1;
}
@@ -1323,28 +1291,24 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr
addrs)
}
-static int qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
+static int
+qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr next_addr)
{
+ virDevicePCIAddress tmp_addr = addrs->lastaddr;
int i;
- int iteration;
-
- for (i = addrs->nextslot, iteration = 0;
- iteration <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, iteration++) {
- virDomainDeviceInfo maybe;
- char *addr;
+ char *addr;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < i)
- i = 0;
- memset(&maybe, 0, sizeof(maybe));
- maybe.addr.pci.domain = 0;
- maybe.addr.pci.bus = 0;
- maybe.addr.pci.slot = i;
- maybe.addr.pci.function = 0;
+ tmp_addr.slot++;
+ for (i = 0; i <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, tmp_addr.slot++) {
+ if (QEMU_PCI_ADDRESS_LAST_SLOT < tmp_addr.slot) {
+ tmp_addr.slot = 0;
+ }
- if (!(addr = qemuPCIAddressAsString(&maybe)))
+ if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
return -1;
- if (qemuDomainPCIAddressCheckSlot(addrs, &maybe) < 0) {
+ if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
VIR_DEBUG("PCI addr %s already in use", addr);
VIR_FREE(addr);
continue;
@@ -1353,7 +1317,9 @@ static int
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
VIR_DEBUG("Found free PCI addr %s", addr);
VIR_FREE(addr);
- return i;
+ addrs->lastaddr = tmp_addr;
+ *next_addr = tmp_addr;
+ return 0;
}
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1364,24 +1330,17 @@ static int
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev)
{
- int slot = qemuDomainPCIAddressGetNextSlot(addrs);
-
- if (slot < 0)
+ virDevicePCIAddress addr;
+ if (qemuDomainPCIAddressGetNextSlot(addrs, &addr) < 0)
return -1;
- if (qemuDomainPCIAddressReserveSlot(addrs, slot) < 0)
+ if (qemuDomainPCIAddressReserveSlot(addrs, &addr) < 0)
return -1;
dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
- dev->addr.pci.bus = 0;
- dev->addr.pci.domain = 0;
- dev->addr.pci.slot = slot;
- dev->addr.pci.function = 0;
-
- addrs->nextslot = slot + 1;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < addrs->nextslot)
- addrs->nextslot = 0;
+ dev->addr.pci = addr;
+ addrs->lastaddr = addr;
return 0;
}
@@ -1433,11 +1392,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
size_t i, j;
bool reservedIDE = false;
bool reservedUSB = false;
- int function;
bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps,
QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ virDevicePCIAddress tmp_addr;
+ virDevicePCIAddressPtr addrptr;
+ unsigned int *func = &tmp_addr.function;
+
- /* Host bridge */
- if (qemuDomainPCIAddressReserveSlot(addrs, 0) < 0)
+ /* Reserve slot 0 for the host bridge */
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
+ if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0)
goto error;
/* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */
@@ -1491,13 +1454,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
/* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller)
* hardcoded slot=1, multifunction device
*/
- for (function = 0; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- if ((function == 1 && reservedIDE) ||
- (function == 2 && reservedUSB))
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
+ tmp_addr.slot = 1;
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ if ((*func == 1 && reservedIDE) ||
+ (*func == 2 && reservedUSB))
/* we have reserved this pci address */
continue;
- if (qemuDomainPCIAddressReserveFunction(addrs, 1, function) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0)
goto error;
}
@@ -1509,11 +1474,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
primaryVideo->info.addr.pci.bus = 0;
primaryVideo->info.addr.pci.slot = 2;
primaryVideo->info.addr.pci.function = 0;
+ addrptr = &primaryVideo->info.addr.pci;
- if (qemuDomainPCIAddressCheckSlot(addrs, &primaryVideo->info) < 0)
{
+ if (qemuDomainPCIAddressCheckSlot(addrs, addrptr) < 0) {
if (qemuDeviceVideoUsable) {
virResetLastError();
- if (qemuDomainPCIAddressSetNextAddr(addrs,
&primaryVideo->info) < 0)
+ if (qemuDomainPCIAddressSetNextAddr(addrs,&primaryVideo->info)
< 0)
goto error;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1521,7 +1487,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
"QEMU needs it for primary video"));
goto error;
}
- } else if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) {
+ } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr) < 0) {
goto error;
}
} else if (!qemuDeviceVideoUsable) {
@@ -1537,16 +1503,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
* has already reserved the address, so we must skip */
}
} else if (!qemuDeviceVideoUsable) {
- virDomainDeviceInfo dev;
- memset(&dev, 0, sizeof(dev));
- dev.addr.pci.slot = 2;
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
+ tmp_addr.slot = 2;
- if (qemuDomainPCIAddressCheckSlot(addrs, &dev) < 0) {
+ if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
VIR_DEBUG("PCI address 0:0:2.0 in use, future addition of a video"
" device will not be possible without manual"
" intervention");
virResetLastError();
- } else if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) {
+ } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) {
goto error;
}
}
@@ -1609,6 +1574,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
/* USB2 needs special handling to put all companions in the same slot */
if (IS_USB2_CONTROLLER(def->controllers[i])) {
virDevicePCIAddress addr = { 0, 0, 0, 0, false };
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
for (j = 0 ; j < i ; j++) {
if (IS_USB2_CONTROLLER(def->controllers[j]) &&
def->controllers[j]->idx == def->controllers[i]->idx) {
@@ -1636,19 +1602,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
if (addr.slot == 0) {
/* This is the first part of the controller, so need
* to find a free slot & then reserve a function */
- int slot = qemuDomainPCIAddressGetNextSlot(addrs);
- if (slot < 0)
+ if (qemuDomainPCIAddressGetNextSlot(addrs, &tmp_addr) < 0)
goto error;
- addr.slot = slot;
- addrs->nextslot = addr.slot + 1;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < addrs->nextslot)
- addrs->nextslot = 0;
+ addr.bus = tmp_addr.bus;
+ addr.slot = tmp_addr.slot;
}
/* Finally we can reserve the slot+function */
- if (qemuDomainPCIAddressReserveFunction(addrs,
- addr.slot,
- addr.function) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &addr) < 0)
goto error;
def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 7e52c5d..e4db000 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -194,21 +194,18 @@ int qemuDomainAssignPCIAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainObjPtr obj);
qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);
-int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function);
int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
- int slot);
+ virDevicePCIAddressPtr addr);
int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev);
+ virDevicePCIAddressPtr addr);
int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev);
int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev);
int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev);
-int qemuDomainPCIAddressReleaseFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function);
-int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot);
+ virDevicePCIAddressPtr addr);
+int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr);
void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs);
int qemuAssignDevicePCISlots(virDomainDefPtr def,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 488a440..4b34122 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -325,7 +325,7 @@ error:
(disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- disk->info.addr.pci.slot) < 0)
+ &disk->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on %s", disk->src);
if (virSecurityManagerRestoreImageLabel(driver->securityManager,
@@ -402,7 +402,7 @@ cleanup:
(controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- controller->info.addr.pci.slot) < 0)
+ &controller->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on controller");
VIR_FREE(devstr);
@@ -899,7 +899,7 @@ cleanup:
(net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- net->info.addr.pci.slot) < 0)
+ &net->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on NIC");
if (iface_connected) {
@@ -1042,7 +1042,7 @@ error:
(hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- hostdev->info->addr.pci.slot) < 0)
+ &hostdev->info->addr.pci) < 0)
VIR_WARN("Unable to release PCI address on host device");
qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1);
@@ -2084,7 +2084,7 @@ int qemuDomainDetachPciDiskDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info.addr.pci.slot) < 0)
+ &detach->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on %s",
dev->data.disk->src);
virDomainDiskRemove(vm->def, i);
@@ -2321,7 +2321,7 @@ int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info.addr.pci.slot) < 0)
+ &detach->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on controller");
ret = 0;
@@ -2397,7 +2397,7 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info->addr.pci.slot) < 0)
+ &detach->info->addr.pci) < 0)
VIR_WARN("Unable to release PCI address on host device");
cleanup:
@@ -2649,7 +2649,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info.addr.pci.slot) < 0)
+ &detach->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on NIC");
virDomainConfNWFilterTeardown(detach);
--
1.7.12.4
11:02 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: switch PCI address alocation to use virDevicePCIAddress
On 02/15/2013 03:22 AM, Ján Tomko wrote:
Some functions were using virDomainDeviceInfo where
virDevicePCIAddress
would suffice. Some were only using integers for slots and functions,
assuming the bus numbers are always 0.
Switch from virDomainDeviceInfoPtr to virDevicePCIAddressPtr:
qemuPCIAddressAsString
qemuDomainPCIAddressCheckSlot
qemuDomainPCIAddressReserveAddr
qemuDomainPCIAddressReleaseAddr
Switch from int slot to virDevicePCIAddressPtr:
qemuDomainPCIAddressReserveSlot
qemuDomainPCIAddressReleaseSlot
qemuDomainPCIAddressGetNextSlot
Deleted functions (they would take the same parameters
as ReserveAddr/ReleaseAddr do now.)
qemuDomainPCIAddressReserveFunction
qemuDomainPCIAddressReleaseFunction
---
src/qemu/qemu_command.c | 267 +++++++++++++++++++++---------------------------
src/qemu/qemu_command.h | 13 +--
src/qemu/qemu_hotplug.c | 16 +--
3 files changed, 127 insertions(+), 169 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c28123..b50e779 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -953,30 +953,30 @@ cleanup:
#define QEMU_PCI_ADDRESS_LAST_FUNCTION 8
struct _qemuDomainPCIAddressSet {
virHashTablePtr used;
- int nextslot;
+ virDevicePCIAddress lastaddr;
};
-static char *qemuPCIAddressAsString(virDomainDeviceInfoPtr dev)
+static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
{
- char *addr;
+ char *str;
- if (dev->addr.pci.domain != 0 ||
- dev->addr.pci.bus != 0) {
+ if (addr->domain != 0 ||
+ addr->bus != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Only PCI domain 0 and bus 0 are available"));
return NULL;
}
- if (virAsprintf(&addr, "%d:%d:%d.%d",
- dev->addr.pci.domain,
- dev->addr.pci.bus,
- dev->addr.pci.slot,
- dev->addr.pci.function) < 0) {
+ if (virAsprintf(&str, "%d:%d:%d.%d",
+ addr->domain,
+ addr->bus,
+ addr->slot,
+ addr->function) < 0) {
virReportOOMError();
return NULL;
}
- return addr;
+ return str;
}
@@ -999,7 +999,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
}
- addr = qemuPCIAddressAsString(info);
+ addr = qemuPCIAddressAsString(&info->addr.pci);
if (!addr)
goto cleanup;
@@ -1024,12 +1024,11 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
if ((info->addr.pci.function == 0) &&
(info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) {
/* a function 0 w/o multifunction=on must reserve the entire slot */
- int function;
- virDomainDeviceInfo temp_info = *info;
+ virDevicePCIAddress tmp_addr = info->addr.pci;
+ unsigned int *func = &tmp_addr.function;
- for (function = 1; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- temp_info.addr.pci.function = function;
- addr = qemuPCIAddressAsString(&temp_info);
+ for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ addr = qemuPCIAddressAsString(&tmp_addr);
if (!addr)
goto cleanup;
@@ -1142,90 +1141,75 @@ error:
* is used by the other device.
*/
static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev)
+ virDevicePCIAddressPtr addr)
{
- char *addr;
- virDomainDeviceInfo temp_dev;
- int function;
-
- temp_dev = *dev;
- for (function = 0; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- temp_dev.addr.pci.function = function;
- addr = qemuPCIAddressAsString(&temp_dev);
- if (!addr)
+ char *str;
+ virDevicePCIAddress tmp_addr = *addr;
+ unsigned int *func = &(tmp_addr.function);
+
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ str = qemuPCIAddressAsString(&tmp_addr);
+ if (!str)
return -1;
- if (virHashLookup(addrs->used, addr)) {
- VIR_FREE(addr);
+ if (virHashLookup(addrs->used, str)) {
+ VIR_FREE(str);
return -1;
}
- VIR_FREE(addr);
+ VIR_FREE(str);
}
return 0;
}
int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev)
+ virDevicePCIAddressPtr addr)
{
- char *addr;
+ char *str;
- addr = qemuPCIAddressAsString(dev);
- if (!addr)
+ str = qemuPCIAddressAsString(addr);
+ if (!str)
return -1;
- VIR_DEBUG("Reserving PCI addr %s", addr);
+ VIR_DEBUG("Reserving PCI addr %s", str);
- if (virHashLookup(addrs->used, addr)) {
+ if (virHashLookup(addrs->used, str)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to reserve PCI address %s"), addr);
- VIR_FREE(addr);
+ _("unable to reserve PCI address %s"), str);
+ VIR_FREE(str);
return -1;
}
- if (virHashAddEntry(addrs->used, addr, addr)) {
- VIR_FREE(addr);
+ if (virHashAddEntry(addrs->used, str, str)) {
+ VIR_FREE(str);
return -1;
}
- if (dev->addr.pci.slot > addrs->nextslot) {
- addrs->nextslot = dev->addr.pci.slot + 1;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < addrs->nextslot)
- addrs->nextslot = 0;
- }
-
+ addrs->lastaddr = *addr;
+ addrs->lastaddr.function = 0;
+ addrs->lastaddr.multi = 0;
return 0;
}
-int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function)
-{
- virDomainDeviceInfo dev;
-
- dev.addr.pci.domain = 0;
- dev.addr.pci.bus = 0;
- dev.addr.pci.slot = slot;
- dev.addr.pci.function = function;
-
- return qemuDomainPCIAddressReserveAddr(addrs, &dev);
-}
-
int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
- int slot)
+ virDevicePCIAddressPtr addr)
{
- int function;
+ virDevicePCIAddress tmp_addr = *addr;
+ unsigned int *func = &tmp_addr.function;
+ int i;
- for (function = 0; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- if (qemuDomainPCIAddressReserveFunction(addrs, slot, function) < 0)
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0)
goto cleanup;
}
return 0;
cleanup:
- for (function--; function >= 0; function--) {
- qemuDomainPCIAddressReleaseFunction(addrs, slot, function);
+ for (i = *func; i >= 0; i--) {
+ *func = i;
+ qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr);
The original loop started at function - 1 and continued while function
= 0. The new loop starts at function and continues while function
>= 0
- it does one extra call at the beginning.
}
return -1;
}
@@ -1245,7 +1229,7 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr
addrs,
return -1;
}
- ret = qemuDomainPCIAddressReserveSlot(addrs, dev->addr.pci.slot);
+ ret = qemuDomainPCIAddressReserveSlot(addrs, &dev->addr.pci);
} else {
ret = qemuDomainPCIAddressSetNextAddr(addrs, dev);
}
@@ -1254,59 +1238,43 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr
addrs,
int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev)
+ virDevicePCIAddressPtr addr)
{
- char *addr;
+ char *str;
int ret;
- addr = qemuPCIAddressAsString(dev);
- if (!addr)
+ str = qemuPCIAddressAsString(addr);
+ if (!str)
return -1;
- ret = virHashRemoveEntry(addrs->used, addr);
+ ret = virHashRemoveEntry(addrs->used, str);
- VIR_FREE(addr);
+ VIR_FREE(str);
return ret;
}
-int qemuDomainPCIAddressReleaseFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function)
+int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
{
- virDomainDeviceInfo dev;
-
- dev.addr.pci.domain = 0;
- dev.addr.pci.bus = 0;
- dev.addr.pci.slot = slot;
- dev.addr.pci.function = function;
-
- return qemuDomainPCIAddressReleaseAddr(addrs, &dev);
-}
-
-int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot)
-{
- virDomainDeviceInfo dev;
- char *addr;
+ char *str;
int ret = 0;
- unsigned int *function = &dev.addr.pci.function;
-
- dev.addr.pci.domain = 0;
- dev.addr.pci.bus = 0;
- dev.addr.pci.slot = slot;
+ virDevicePCIAddress tmp_addr = *addr;
+ unsigned int *func = &tmp_addr.function;
- for (*function = 0; *function < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*function)++) {
- addr = qemuPCIAddressAsString(&dev);
- if (!addr)
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ str = qemuPCIAddressAsString(&tmp_addr);
+ if (!str)
return -1;
- if (!virHashLookup(addrs->used, addr)) {
- VIR_FREE(addr);
+ if (!virHashLookup(addrs->used, str)) {
+ VIR_FREE(str);
continue;
}
- VIR_FREE(addr);
+ VIR_FREE(str);
- if (qemuDomainPCIAddressReleaseFunction(addrs, slot, *function) < 0)
+ if (qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr) < 0)
ret = -1;
}
@@ -1323,28 +1291,24 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr
addrs)
}
-static int qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
+static int
+qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr next_addr)
{
+ virDevicePCIAddress tmp_addr = addrs->lastaddr;
int i;
- int iteration;
-
- for (i = addrs->nextslot, iteration = 0;
- iteration <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, iteration++) {
- virDomainDeviceInfo maybe;
- char *addr;
+ char *addr;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < i)
- i = 0;
- memset(&maybe, 0, sizeof(maybe));
- maybe.addr.pci.domain = 0;
- maybe.addr.pci.bus = 0;
- maybe.addr.pci.slot = i;
- maybe.addr.pci.function = 0;
+ tmp_addr.slot++;
+ for (i = 0; i <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, tmp_addr.slot++) {
+ if (QEMU_PCI_ADDRESS_LAST_SLOT < tmp_addr.slot) {
+ tmp_addr.slot = 0;
+ }
- if (!(addr = qemuPCIAddressAsString(&maybe)))
+ if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
return -1;
- if (qemuDomainPCIAddressCheckSlot(addrs, &maybe) < 0) {
+ if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
VIR_DEBUG("PCI addr %s already in use", addr);
VIR_FREE(addr);
continue;
@@ -1353,7 +1317,9 @@ static int
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
VIR_DEBUG("Found free PCI addr %s", addr);
VIR_FREE(addr);
- return i;
+ addrs->lastaddr = tmp_addr;
+ *next_addr = tmp_addr;
+ return 0;
}
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1364,24 +1330,17 @@ static int
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev)
{
- int slot = qemuDomainPCIAddressGetNextSlot(addrs);
-
- if (slot < 0)
+ virDevicePCIAddress addr;
+ if (qemuDomainPCIAddressGetNextSlot(addrs, &addr) < 0)
return -1;
- if (qemuDomainPCIAddressReserveSlot(addrs, slot) < 0)
+ if (qemuDomainPCIAddressReserveSlot(addrs, &addr) < 0)
return -1;
dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
- dev->addr.pci.bus = 0;
- dev->addr.pci.domain = 0;
- dev->addr.pci.slot = slot;
- dev->addr.pci.function = 0;
-
- addrs->nextslot = slot + 1;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < addrs->nextslot)
- addrs->nextslot = 0;
+ dev->addr.pci = addr;
+ addrs->lastaddr = addr;
return 0;
}
@@ -1433,11 +1392,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
size_t i, j;
bool reservedIDE = false;
bool reservedUSB = false;
- int function;
bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps,
QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ virDevicePCIAddress tmp_addr;
+ virDevicePCIAddressPtr addrptr;
+ unsigned int *func = &tmp_addr.function;
+
- /* Host bridge */
- if (qemuDomainPCIAddressReserveSlot(addrs, 0) < 0)
+ /* Reserve slot 0 for the host bridge */
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
+ if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0)
goto error;
/* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */
@@ -1491,13 +1454,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
/* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller)
* hardcoded slot=1, multifunction device
*/
- for (function = 0; function < QEMU_PCI_ADDRESS_LAST_FUNCTION; function++) {
- if ((function == 1 && reservedIDE) ||
- (function == 2 && reservedUSB))
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
+ tmp_addr.slot = 1;
+ for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
+ if ((*func == 1 && reservedIDE) ||
+ (*func == 2 && reservedUSB))
/* we have reserved this pci address */
continue;
- if (qemuDomainPCIAddressReserveFunction(addrs, 1, function) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0)
goto error;
}
@@ -1509,11 +1474,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
primaryVideo->info.addr.pci.bus = 0;
primaryVideo->info.addr.pci.slot = 2;
primaryVideo->info.addr.pci.function = 0;
+ addrptr = &primaryVideo->info.addr.pci;
- if (qemuDomainPCIAddressCheckSlot(addrs, &primaryVideo->info) < 0)
{
+ if (qemuDomainPCIAddressCheckSlot(addrs, addrptr) < 0) {
if (qemuDeviceVideoUsable) {
virResetLastError();
- if (qemuDomainPCIAddressSetNextAddr(addrs,
&primaryVideo->info) < 0)
+ if
(qemuDomainPCIAddressSetNextAddr(addrs,&primaryVideo->info) < 0)
You just removed a space between args in the line above ^^^
goto error;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1521,7 +1487,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
"QEMU needs it for primary video"));
goto error;
}
- } else if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) {
+ } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr) < 0) {
goto error;
}
} else if (!qemuDeviceVideoUsable) {
@@ -1537,16 +1503,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
* has already reserved the address, so we must skip */
}
} else if (!qemuDeviceVideoUsable) {
- virDomainDeviceInfo dev;
- memset(&dev, 0, sizeof(dev));
- dev.addr.pci.slot = 2;
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
+ tmp_addr.slot = 2;
- if (qemuDomainPCIAddressCheckSlot(addrs, &dev) < 0) {
+ if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
VIR_DEBUG("PCI address 0:0:2.0 in use, future addition of a
video"
" device will not be possible without manual"
" intervention");
virResetLastError();
- } else if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) {
+ } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) {
goto error;
}
}
@@ -1609,6 +1574,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
/* USB2 needs special handling to put all companions in the same slot */
if (IS_USB2_CONTROLLER(def->controllers[i])) {
virDevicePCIAddress addr = { 0, 0, 0, 0, false };
+ memset(&tmp_addr, 0, sizeof(tmp_addr));
for (j = 0 ; j < i ; j++) {
if (IS_USB2_CONTROLLER(def->controllers[j]) &&
def->controllers[j]->idx == def->controllers[i]->idx) {
@@ -1636,19 +1602,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
if (addr.slot == 0) {
/* This is the first part of the controller, so need
* to find a free slot & then reserve a function */
- int slot = qemuDomainPCIAddressGetNextSlot(addrs);
- if (slot < 0)
+ if (qemuDomainPCIAddressGetNextSlot(addrs, &tmp_addr) < 0)
goto error;
- addr.slot = slot;
- addrs->nextslot = addr.slot + 1;
- if (QEMU_PCI_ADDRESS_LAST_SLOT < addrs->nextslot)
- addrs->nextslot = 0;
+ addr.bus = tmp_addr.bus;
+ addr.slot = tmp_addr.slot;
}
/* Finally we can reserve the slot+function */
- if (qemuDomainPCIAddressReserveFunction(addrs,
- addr.slot,
- addr.function) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &addr) < 0)
goto error;
def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 7e52c5d..e4db000 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -194,21 +194,18 @@ int qemuDomainAssignPCIAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainObjPtr obj);
qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);
-int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function);
int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
- int slot);
+ virDevicePCIAddressPtr addr);
int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev);
+ virDevicePCIAddressPtr addr);
int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev);
int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev);
int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
- virDomainDeviceInfoPtr dev);
-int qemuDomainPCIAddressReleaseFunction(qemuDomainPCIAddressSetPtr addrs,
- int slot, int function);
-int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot);
+ virDevicePCIAddressPtr addr);
+int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr);
void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs);
int qemuAssignDevicePCISlots(virDomainDefPtr def,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 488a440..4b34122 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -325,7 +325,7 @@ error:
(disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- disk->info.addr.pci.slot) < 0)
+ &disk->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on %s", disk->src);
if (virSecurityManagerRestoreImageLabel(driver->securityManager,
@@ -402,7 +402,7 @@ cleanup:
(controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- controller->info.addr.pci.slot) < 0)
+ &controller->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on controller");
VIR_FREE(devstr);
@@ -899,7 +899,7 @@ cleanup:
(net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- net->info.addr.pci.slot) < 0)
+ &net->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on NIC");
if (iface_connected) {
@@ -1042,7 +1042,7 @@ error:
(hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
releaseaddr &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- hostdev->info->addr.pci.slot) < 0)
+ &hostdev->info->addr.pci) < 0)
VIR_WARN("Unable to release PCI address on host device");
qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1);
@@ -2084,7 +2084,7 @@ int qemuDomainDetachPciDiskDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info.addr.pci.slot) < 0)
+ &detach->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on %s",
dev->data.disk->src);
virDomainDiskRemove(vm->def, i);
@@ -2321,7 +2321,7 @@ int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info.addr.pci.slot) < 0)
+ &detach->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on controller");
ret = 0;
@@ -2397,7 +2397,7 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info->addr.pci.slot) < 0)
+ &detach->info->addr.pci) < 0)
VIR_WARN("Unable to release PCI address on host device");
cleanup:
@@ -2649,7 +2649,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
qemuDomainPCIAddressReleaseSlot(priv->pciaddrs,
- detach->info.addr.pci.slot) < 0)
+ &detach->info.addr.pci) < 0)
VIR_WARN("Unable to release PCI address on NIC");
virDomainConfNWFilterTeardown(detach);
There were a lot of mechanical changes in this. Assuming that you've run
make syntax-check, and tried out several cases of defining new domains
with devices that have no pci address specified (with satisfactory
results!), and that you fix the two small nits I found above, then ACK.
9:10 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: switch PCI address alocation to use virDevicePCIAddress
On 02/19/13 04:02, Laine Stump wrote:
There were a lot of mechanical changes in this. Assuming that
you've run
make syntax-check, and tried out several cases of defining new domains
with devices that have no pci address specified (with satisfactory
results!), and that you fix the two small nits I found above, then ACK.
Thanks and pushed now.
I've tried a few different combinations as well as hotplugging to see if
the cleanup cycle for qemuDomainPCIAddressReserveSlot works.
This is the Thunderbird-mangled diff of the fixes for those two nits:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b50e779..dee493f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1197,7 +1197,7 @@ int
qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
{
virDevicePCIAddress tmp_addr = *addr;
unsigned int *func = &tmp_addr.function;
- int i;
+ unsigned int last;
for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0)
@@ -1207,10 +1207,8 @@ int
qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
return 0;
cleanup:
- for (i = *func; i >= 0; i--) {
- *func = i;
+ for (last = *func, *func = 0; *func < last; (*func)++)
qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr);
- }
return -1;
}
@@ -1479,7 +1477,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
if (qemuDomainPCIAddressCheckSlot(addrs, addrptr) < 0) {
if (qemuDeviceVideoUsable) {
virResetLastError();
- if
(qemuDomainPCIAddressSetNextAddr(addrs,&primaryVideo->info) < 0)
+ if (qemuDomainPCIAddressSetNextAddr(addrs,
&primaryVideo->info) < 0)
goto error;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
4:22 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: allow multiple buses in PCI address alocation
Allow allocating addresses with non-zero bus numbers.
---
src/qemu/qemu_command.c | 41 ++++++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b50e779..f747cfb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -954,17 +954,25 @@ cleanup:
struct _qemuDomainPCIAddressSet {
virHashTablePtr used;
virDevicePCIAddress lastaddr;
+ unsigned int maxbus;
};
-static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
+static char *qemuPCIAddressAsString(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
{
char *str;
- if (addr->domain != 0 ||
- addr->bus != 0) {
+ if (addr->domain != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Only PCI domain 0 and bus 0 are available"));
+ _("Only PCI domain 0 is available"));
+ return NULL;
+ }
+
+ if (addr->bus > addrs->maxbus) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Only PCI buses up to %u are available"),
+ addrs->maxbus);
return NULL;
}
@@ -999,7 +1007,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
}
- addr = qemuPCIAddressAsString(&info->addr.pci);
+ addr = qemuPCIAddressAsString(addrs, &info->addr.pci);
if (!addr)
goto cleanup;
@@ -1028,7 +1036,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
unsigned int *func = &tmp_addr.function;
for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
- addr = qemuPCIAddressAsString(&tmp_addr);
+ addr = qemuPCIAddressAsString(addrs, &tmp_addr);
if (!addr)
goto cleanup;
@@ -1148,7 +1156,7 @@ static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr
addrs,
unsigned int *func = &(tmp_addr.function);
for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
- str = qemuPCIAddressAsString(&tmp_addr);
+ str = qemuPCIAddressAsString(addrs, &tmp_addr);
if (!str)
return -1;
@@ -1168,7 +1176,7 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr
addrs,
{
char *str;
- str = qemuPCIAddressAsString(addr);
+ str = qemuPCIAddressAsString(addrs, addr);
if (!str)
return -1;
@@ -1243,7 +1251,7 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr
addrs,
char *str;
int ret;
- str = qemuPCIAddressAsString(addr);
+ str = qemuPCIAddressAsString(addrs, addr);
if (!str)
return -1;
@@ -1263,7 +1271,7 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr
addrs,
unsigned int *func = &tmp_addr.function;
for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
- str = qemuPCIAddressAsString(&tmp_addr);
+ str = qemuPCIAddressAsString(addrs, &tmp_addr);
if (!str)
return -1;
@@ -1298,19 +1306,30 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
virDevicePCIAddress tmp_addr = addrs->lastaddr;
int i;
char *addr;
+ bool next_bus = false;
tmp_addr.slot++;
for (i = 0; i <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, tmp_addr.slot++) {
if (QEMU_PCI_ADDRESS_LAST_SLOT < tmp_addr.slot) {
+ /* Check all the slots in this bus first */
tmp_addr.slot = 0;
+ next_bus = !next_bus;
}
- if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
+ if (!(addr = qemuPCIAddressAsString(addrs, &tmp_addr)))
return -1;
if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
VIR_DEBUG("PCI addr %s already in use", addr);
VIR_FREE(addr);
+ if (i == QEMU_PCI_ADDRESS_LAST_SLOT && next_bus &&
+ tmp_addr.bus < addrs->maxbus) {
+ /* Move on to the next bus if this one's full */
+ i = 0;
+ tmp_addr.bus++;
+ tmp_addr.slot = 0;
+ next_bus = !next_bus;
+ }
continue;
}
--
1.7.12.4
12:12 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: allow multiple buses in PCI address alocation
On 02/15/2013 03:22 AM, Ján Tomko wrote:
Allow allocating addresses with non-zero bus numbers.
while not actually allowing it :-) (since maxbus is never set to
anything > 0, as you say in Patch 0/2). How/when do you envision that
being changed? liguang's patches allow devices with any bus number you
want, and just add pci-bridge devices as necessary, so I don't know if
any explicit setting/checking of a max is necessary (although it might
be something checked for in a virCaps callback function, as certain
machine types might support fewer buses than others or something).
BTW, I think that this entire model of the guest PCI address space
allocation could be useful if moved to its own library in util - it
seems like something that other hypervisors could benefit from.
---
src/qemu/qemu_command.c | 41 ++++++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b50e779..f747cfb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -954,17 +954,25 @@ cleanup:
struct _qemuDomainPCIAddressSet {
virHashTablePtr used;
virDevicePCIAddress lastaddr;
+ unsigned int maxbus;
};
-static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
+static char *qemuPCIAddressAsString(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
{
char *str;
- if (addr->domain != 0 ||
- addr->bus != 0) {
+ if (addr->domain != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Only PCI domain 0 and bus 0 are available"));
+ _("Only PCI domain 0 is available"));
+ return NULL;
+ }
+
+ if (addr->bus > addrs->maxbus) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Only PCI buses up to %u are available"),
+ addrs->maxbus);
return NULL;
}
I realize that this check was pre-existing, but to me it seems out of
place to have such a check in a function that is merely converting the
PCI address from its components into a string; a marriage of convenience
perhaps?
I think that instead of putting the extra burden of checking limits on
this formatting function, there should instead be a separate function
that checks to make sure all elements are within bounds (and why limit
it to domain and bus? May as well check slot and function as well), and
that function should probably be caps-related (i.e. a callback function
provided in virCaps)
@@ -999,7 +1007,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
}
- addr = qemuPCIAddressAsString(&info->addr.pci);
+ addr = qemuPCIAddressAsString(addrs, &info->addr.pci);
if (!addr)
goto cleanup;
@@ -1028,7 +1036,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
unsigned int *func = &tmp_addr.function;
for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
- addr = qemuPCIAddressAsString(&tmp_addr);
+ addr = qemuPCIAddressAsString(addrs, &tmp_addr);
if (!addr)
goto cleanup;
@@ -1148,7 +1156,7 @@ static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr
addrs,
unsigned int *func = &(tmp_addr.function);
for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
- str = qemuPCIAddressAsString(&tmp_addr);
+ str = qemuPCIAddressAsString(addrs, &tmp_addr);
if (!str)
return -1;
@@ -1168,7 +1176,7 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr
addrs,
{
char *str;
- str = qemuPCIAddressAsString(addr);
+ str = qemuPCIAddressAsString(addrs, addr);
if (!str)
return -1;
@@ -1243,7 +1251,7 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr
addrs,
char *str;
int ret;
- str = qemuPCIAddressAsString(addr);
+ str = qemuPCIAddressAsString(addrs, addr);
if (!str)
return -1;
@@ -1263,7 +1271,7 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr
addrs,
unsigned int *func = &tmp_addr.function;
for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
- str = qemuPCIAddressAsString(&tmp_addr);
+ str = qemuPCIAddressAsString(addrs, &tmp_addr);
if (!str)
return -1;
@@ -1298,19 +1306,30 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr
addrs,
virDevicePCIAddress tmp_addr = addrs->lastaddr;
int i;
char *addr;
+ bool next_bus = false;
tmp_addr.slot++;
for (i = 0; i <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, tmp_addr.slot++) {
if (QEMU_PCI_ADDRESS_LAST_SLOT < tmp_addr.slot) {
+ /* Check all the slots in this bus first */
tmp_addr.slot = 0;
+ next_bus = !next_bus;
}
- if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
+ if (!(addr = qemuPCIAddressAsString(addrs, &tmp_addr)))
return -1;
if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
VIR_DEBUG("PCI addr %s already in use", addr);
VIR_FREE(addr);
+ if (i == QEMU_PCI_ADDRESS_LAST_SLOT && next_bus &&
+ tmp_addr.bus < addrs->maxbus) {
+ /* Move on to the next bus if this one's full */
+ i = 0;
+ tmp_addr.bus++;
+ tmp_addr.slot = 0;
+ next_bus = !next_bus;
+ }
continue;
}
This function is more confused than necessary. Rather than having the
"next_bus" boolean, wouldn't it be simpler to follow if you just had a
nested loop - inner for slot and outer for bus?
Beyond that (and I guess this is a discussion that's beyond the bounds
of this patch, since the data structure is pre-existing), is a hashtable
really the best choice for representing this? What we really have is a
collection of buses, each with 32 slots x 8 functions; Why not have a
list of 32 byte arrays - each byte is a slot, and each bit within a byte
is a function - just set the bit for any slot/function that is in use. A
new 32 byte array could be added to the list as new buses are put in
use. If there was any extra information stored for each entry then a
hashtable might make sense, but there isn't - the only info stored is
the string used to compute the hash.
9:55 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: allow multiple buses in PCI address alocation
On 02/21/13 17:12, Laine Stump wrote:
On 02/15/2013 03:22 AM, Ján Tomko wrote:
> Allow allocating addresses with non-zero bus numbers.
while not actually allowing it :-) (since maxbus is never set to
anything > 0, as you say in Patch 0/2). How/when do you envision that
being changed? liguang's patches allow devices with any bus number you
want, and just add pci-bridge devices as necessary, so I don't know if
any explicit setting/checking of a max is necessary (although it might
be something checked for in a virCaps callback function, as certain
machine types might support fewer buses than others or something).
The idea was to change it when creating the PCI address set based on the
number of bridges in the domain definition (or machine type, if it
implies more buses).
liguang's patch only adds bridges for explicitly specified bus numbers,
we could still run out of buses if there are enough devices without a
specified address or if we use hotplug. Both of these sound convenient,
but I'm not sure if there is a pretty solution.
> -static char *qemuPCIAddressAsString(virDevicePCIAddressPtr
addr)
> +static char *qemuPCIAddressAsString(qemuDomainPCIAddressSetPtr addrs,
> + virDevicePCIAddressPtr addr)
> {
> char *str;
>
> - if (addr->domain != 0 ||
> - addr->bus != 0) {
> + if (addr->domain != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Only PCI domain 0 and bus 0 are available"));
> + _("Only PCI domain 0 is available"));
> + return NULL;
> + }
> +
> + if (addr->bus > addrs->maxbus) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Only PCI buses up to %u are available"),
> + addrs->maxbus);
> return NULL;
> }
I realize that this check was pre-existing, but to me it seems out of
place to have such a check in a function that is merely converting the
PCI address from its components into a string; a marriage of convenience
perhaps?
I think that instead of putting the extra burden of checking limits on
this formatting function, there should instead be a separate function
that checks to make sure all elements are within bounds (and why limit
it to domain and bus? May as well check slot and function as well), and
that function should probably be caps-related (i.e. a callback function
provided in virCaps)
Slot and device values are checked by the XML parsing code right now,
and looking at all the callers of qemuPCIAddressAsString, only a few of
them can have wrong domain or bus value.
Maybe we should print the values in hexadecimal instead of decimal, to
match the XML, as the strings are also used in error messages.
> @@ -1298,19 +1306,30 @@
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddress tmp_addr = addrs->lastaddr;
> int i;
> char *addr;
> + bool next_bus = false;
>
> tmp_addr.slot++;
> for (i = 0; i <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, tmp_addr.slot++) {
> if (QEMU_PCI_ADDRESS_LAST_SLOT < tmp_addr.slot) {
> + /* Check all the slots in this bus first */
> tmp_addr.slot = 0;
> + next_bus = !next_bus;
> }
>
> - if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
> + if (!(addr = qemuPCIAddressAsString(addrs, &tmp_addr)))
> return -1;
>
> if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
> VIR_DEBUG("PCI addr %s already in use", addr);
> VIR_FREE(addr);
> + if (i == QEMU_PCI_ADDRESS_LAST_SLOT && next_bus &&
> + tmp_addr.bus < addrs->maxbus) {
> + /* Move on to the next bus if this one's full */
> + i = 0;
> + tmp_addr.bus++;
> + tmp_addr.slot = 0;
> + next_bus = !next_bus;
> + }
> continue;
> }
This function is more confused than necessary. Rather than having the
"next_bus" boolean, wouldn't it be simpler to follow if you just had a
nested loop - inner for slot and outer for bus?
And now I realize it's not even able to use the previous buses.
Beyond that (and I guess this is a discussion that's beyond the bounds
of this patch, since the data structure is pre-existing), is a hashtable
really the best choice for representing this? What we really have is a
collection of buses, each with 32 slots x 8 functions; Why not have a
list of 32 byte arrays - each byte is a slot, and each bit within a byte
is a function - just set the bit for any slot/function that is in use. A
new 32 byte array could be added to the list as new buses are put in
use. If there was any extra information stored for each entry then a
hashtable might make sense, but there isn't - the only info stored is
the string used to compute the hash.
That sounds much better than the hash table.
Jan
4268
days inactive
4275
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Laine Stump