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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list