[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 4/5] qemu: auto-add pci-root controller for pc machine types



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]