[libvirt] [PATCH v5 03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

[Andrea Bolognani]

On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:
> Set pciConnectFlags in each device's DeviceInfo and then use those
> flags later when validating existing addresses in
> qemuDomainCollectPCIAddress() and when assigning new addresses with
> qemuDomainPCIAddressReserveNextAddr() (rather than scattering the
> logic about which devices need which type of slot all over the place).
> 
> Note that the exact flags set by
> qemuDomainDeviceCalculatePCIConnectFlags() are different from the
> flags previously set manually in qemuDomainCollectPCIAddress(), but
> this doesn't matter because all validation of addresses in that case
> ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE
> and PCI_DEVICE the same (this lax checking was done on purpose,
> because there are some things that we want to allow the user to
> specify manually, e.g. assigning a PCIe device to a PCI slot, that we
> *don't* ever want libvirt to do automatically. The flag settings that
> we *really* want to match are 1) the old flag settings in
> qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE
> for everything except PCI controllers) and the new flag settings done

[...] and 2) the new [...]

> by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently
> exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI
> controllers).
> ---
>  src/qemu/qemu_domain_address.c | 205 +++++++++++++++--------------------------
>  1 file changed, 74 insertions(+), 131 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 602179c..d731b19 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
>   * failure would be some internal problem with
>   * virDomainDeviceInfoIterate())
>   */
> -static int ATTRIBUTE_UNUSED
> +static int
>  qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
>                                   virQEMUCapsPtr qemuCaps)
>  {

You should remove ATTRIBUTE_UNUSED from
qemuDomainFillDevicePCIConnectFlags() as well.

[...]
> @@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>      entireSlot = (addr->function == 0 &&
>                    addr->multi != VIR_TRISTATE_SWITCH_ON);
>  
> -    if (virDomainPCIAddressReserveAddr(addrs, addr, flags,
> +    if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
>                                         entireSlot, true) < 0)

Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()
function that takes @info directly?

It would be used only a single time, so it could very well be
argued that it would be overkill... On the other hand, it would
be neat not to use virDomainPCIAddressReserve*() functions at
all in the qemu driver and rely solely on the wrappers instead.

Speaking of which, even with the full series applied there
are still a bunch of uses of virDomainPCIAddressReserveAddr()
and virDomainPCIAddressReserveSlot(), mostly in
qemuDomainValidateDevicePCISlots{PIIX3,Q35}().

The attached patch makes all of them go away, except a few
that actually make sense because they set aside addresses for
potential future devices and as such don't have access to
a virDomainDeviceInfo yet.

I'm not saying we should deal with this right away: I'd
rather go back later to tidy up the whole thing than hold up
the series or go through another round of reviews for what
is ultimately a cosmetic issue.

[...]
> @@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>           */
>          if (!buses_reserved &&
>              !qemuDomainMachineIsVirt(def) &&
> -            qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
> +            qemuDomainPCIAddressReserveNextSlot(addrs, &info) < 0)
>              goto cleanup;
>  
>          if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>              goto cleanup;
>  
>          for (i = 1; i < addrs->nbuses; i++) {
> +            virDomainDeviceDef dev;
> +            int contIndex;
>              virDomainPCIAddressBusPtr bus = &addrs->buses[i];
>  
>              if ((rv = virDomainDefMaybeAddController(
>                       def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
>                       i, bus->model)) < 0)
>                  goto cleanup;
> -            /* If we added a new bridge, we will need one more address */
> -            if (rv > 0 &&
> -                qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
> +
> +            if (rv == 0)
> +                continue; /* no new controller added */

Alternatively, you could turn this into

  if (rv > 0) {
      virDomainDeviceDef dev;
      int contIndex;

      /* The code below */
  }

but I leave up to you whether to go one way or the other.

> +
> +            /* We did add a new controller, so we will need one more
> +             * address (and we need to set the new controller's
> +             * pciConnectFlags)
> +             */
> +            contIndex = virDomainControllerFind(def,
> +                                                VIR_DOMAIN_CONTROLLER_TYPE_PCI,
> +                                                i);
> +            if (contIndex < 0) {
> +                /* this should never happen - we just added it */
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Could not find auto-added %s controller "
> +                                 "with index %zu"),
> +                               virDomainControllerModelPCITypeToString(bus->model),
> +                               i);
> +                goto cleanup;
> +            }
> +            dev.type = VIR_DOMAIN_DEVICE_CONTROLLER;
> +            dev.data.controller = def->controllers[contIndex];
> +            /* set connect flags so it will be properly addressed */
> +            qemuDomainFillDevicePCIConnectFlags(def, &dev, qemuCaps);
> +            if (qemuDomainPCIAddressReserveNextSlot(addrs,
> +                                                    &dev.data.controller->info) < 0)
>                  goto cleanup;
>          }
> +
>          nbuses = addrs->nbuses;
>          virDomainPCIAddressSetFree(addrs);
>          addrs = NULL;

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qemu-Introduce-qemuDomainPCIAddressReserve-Addr-Slot.patch
Type: text/x-patch
Size: 6102 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161025/c5cf1fbe/attachment-0001.bin>