On 04/22/2013 02:43 PM, Ján Tomko wrote:
<controller type='pci' index='0' model='pci-root'/> is auto-added to pc* machine types. Without this controller PCI bus 0 is not available and no PCI addresses are assigned by default.
Since older libvirt supported PCI bus 0 even without this controller, it is removed from the XML when migrating. --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 6 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 57 ++++++++++++------ src/qemu/qemu_command.h | 3 +- src/qemu/qemu_domain.c | 67 +++++++++++++++++++++- [ + tons of test xml files] 162 files changed, 269 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1e7de52..5740009 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9762,7 +9762,7 @@ virDomainLookupVcpuPin(virDomainDefPtr def, return NULL; }
-static int +int virDomainDefMaybeAddController(virDomainDefPtr def, int type, int idx, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3cb626b..565f0f8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2553,6 +2553,12 @@ int virDomainObjListExport(virDomainObjListPtr doms, virDomainVcpuPinDefPtr virDomainLookupVcpuPin(virDomainDefPtr def, int vcpuid);
+int +virDomainDefMaybeAddController(virDomainDefPtr def, + int type, + int idx, + int model); + char *virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps);
#endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 32b4ae8..ca324de 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -116,6 +116,7 @@ virDomainDefFree; virDomainDefGenSecurityLabelDef; virDomainDefGetDefaultEmulator; virDomainDefGetSecurityLabelDef; +virDomainDefMaybeAddController; virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f052a91..3787ff1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1196,6 +1196,7 @@ typedef uint8_t qemuDomainPCIAddressBus[QEMU_PCI_ADDRESS_SLOT_LAST]; struct _qemuDomainPCIAddressSet { qemuDomainPCIAddressBus *used; virDevicePCIAddress lastaddr; + size_t nbuses; /* allocation of 'used' */ };
@@ -1206,6 +1207,10 @@ struct _qemuDomainPCIAddressSet { static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED, virDevicePCIAddressPtr addr) { + if (addrs->nbuses == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); + return false; + } if (addr->domain != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Only PCI domain 0 is available")); @@ -1321,7 +1326,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, qemuDomainObjPrivatePtr priv = NULL;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(addrs = qemuDomainPCIAddressSetCreate(def))) + int nbuses = 0; + int i; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + nbuses++; + } + + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses))) goto cleanup;
if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) @@ -1366,16 +1379,25 @@ int qemuDomainAssignAddresses(virDomainDefPtr def, return qemuDomainAssignPCIAddresses(def, qemuCaps, obj); }
-qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def) +qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, + unsigned int nbuses) { qemuDomainPCIAddressSetPtr addrs; + int i;
if (VIR_ALLOC(addrs) < 0) goto no_memory;
- if (VIR_ALLOC_N(addrs->used, 1) < 0) + if (VIR_ALLOC_N(addrs->used, nbuses) < 0) goto no_memory;
+ addrs->nbuses = nbuses; + + /* reserve slot 0 in every bus - it's used by the host bridge on bus 0 + * and unusable on PCI bridges */ + for (i = 0; i < nbuses; i++) + addrs->used[i][0] = 0xFF; + if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0) goto error;
@@ -1604,12 +1626,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, virDevicePCIAddressPtr addrptr; unsigned int *func = &tmp_addr.function;
- - /* 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 */ for (i = 0; i < def->ncontrollers ; i++) { /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ @@ -1661,16 +1677,18 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) * hardcoded slot=1, multifunction device */ - memset(&tmp_addr, 0, sizeof(tmp_addr)); - tmp_addr.slot = 1; - for (*func = 0; *func < QEMU_PCI_ADDRESS_FUNCTION_LAST; (*func)++) { - if ((*func == 1 && reservedIDE) || - (*func == 2 && reservedUSB)) - /* we have reserved this pci address */ - continue; + if (addrs->nbuses) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + for (*func = 0; *func < QEMU_PCI_ADDRESS_FUNCTION_LAST; (*func)++) { + if ((*func == 1 && reservedIDE) || + (*func == 2 && reservedUSB)) + /* we have reserved this pci address */ + continue;
- if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0) - goto error; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0) + goto error; + } }
if (def->nvideos > 0) { @@ -1762,6 +1780,9 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
/* Device controllers (SCSI, USB, but not IDE, FDC or CCID) */ for (i = 0; i < def->ncontrollers ; i++) { + /* PCI root has no address */ + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + continue; /* FDC lives behind the ISA bridge; CCID is a usb device */ if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_FDC || def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_CCID) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1789c20..b05510b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -196,7 +196,8 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainObjPtr obj); -qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def); +qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, + unsigned int nbuses); int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr addr); int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a7aabdf..ab99538 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -673,6 +673,37 @@ qemuDomainDefPostParse(virDomainDefPtr def, !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) return -1;
+ /* Add implicit PCI root controller if the machine has one */ + switch (def->os.arch) { + case VIR_ARCH_I686: + case VIR_ARCH_X86_64: + if (!def->os.machine) + break; + if (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35") || + STREQ(def->os.machine, "isapc")) + break; + if (!STRPREFIX(def->os.machine, "pc-0.") && + !STRPREFIX(def->os.machine, "pc-1.") && + !STREQ(def->os.machine, "pc") && + !STRPREFIX(def->os.machine, "rhel")) + break;
If you're going to fall through to a different case like this, you should put a comment in the code something like this: /* FALL THROUGH TO NEXT CASE */ just so people don't have to think too hard :-) However, I think it would be more easily expandable in the future if you had a straight switch statement with all the cases, and just set a "needsPCIRoot" boolean for those cases that need it, then an "if (needsPCIRoot)" at the end. In the future when we want to add other implicit devices, each case can be a mix of the appropriate "needsThis" and "needsThat", with the actual additions at the end.
+ + case VIR_ARCH_ALPHA: + case VIR_ARCH_PPC: + case VIR_ARCH_PPC64: + case VIR_ARCH_PPCEMB: + case VIR_ARCH_SH4: + case VIR_ARCH_SH4EB: + if (virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) + return -1; + break; + default: + break; + } +
Wow! Is that what it broke down to? I figured there would be many more than that.
return 0; }
@@ -1255,7 +1286,8 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) { int i; - virDomainControllerDefPtr usb = NULL; + int remove = 0; + virDomainControllerDefPtr usb = NULL, pci = NULL;
/* If only the default USB controller is present, we can remove it * and make the XML compatible with older versions of libvirt which @@ -1274,9 +1306,36 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, if (usb && usb->idx == 0 && usb->model == -1) { VIR_DEBUG("Removing default USB controller from domain '%s'" " for migration compatibility", def->name); + remove++; + } else { + usb = NULL; + } + + /* Remove the default PCI controller if there is only one present + * and its model is pci-root */ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if (pci) { + pci = NULL; + break; + } + pci = def->controllers[i]; + } + } + + if (pci && pci->idx == 0 && + pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { + VIR_DEBUG("Removing default 'pci-root' from domain '%s'" + " for migration compatibility", def->name); + remove++; + } else { + pci = NULL; + } + + if (remove) { controllers = def->controllers; ncontrollers = def->ncontrollers; - if (VIR_ALLOC_N(def->controllers, ncontrollers - 1) < 0) { + if (VIR_ALLOC_N(def->controllers, ncontrollers - remove) < 0) { controllers = NULL; virReportOOMError(); goto cleanup; @@ -1284,10 +1343,12 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
def->ncontrollers = 0; for (i = 0; i < ncontrollers; i++) { - if (controllers[i] != usb) + if (controllers[i] != usb && controllers[i] != pci) def->controllers[def->ncontrollers++] = controllers[i]; } } + + }
ret = virDomainDefFormatInternal(def, flags, buf);
... and the rest is the gigantic amount of test xml changes. ACK, with the addition of the "FALLTHROUGH" comment, or restructuring it is as I suggested.