[libvirt] [PATCH v2 4/4] qemu: assign vfio devices to PCIe addresses when appropriate

[Andrea Bolognani]

On Mon, 2016-11-21 at 00:01 -0500, Laine Stump wrote:
> Although nearly all host devices that are assigned to guests using
> vfio ("<hostdev>" devices in libvirt) are physically PCI Express

s/vfio/VFIO/

Same for the subject.

[...]
> @@ -558,8 +558,88 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>              return 0;
>          }
>  
> -    case VIR_DOMAIN_DEVICE_HOSTDEV:
> -        return pciFlags;
> +    case VIR_DOMAIN_DEVICE_HOSTDEV: {
> +        virDomainHostdevDefPtr hostdev = dev->data.hostdev;
> +
> +        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> +            hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {

How about inverting this condition and returning 0 immediately
unless if the wrong mode or type is used? You could get rid
of one indentation level that way, and the function already
has a gazillion return statements anyway.

[...]
> +            if (virDeviceInfoPCIAddressPresent(hostdev->info)) {
> +                /* A guest-side address has already been assigned, so
> +                 * we can avoid reading the PCI config, and just use
> +                 * pcieFlags, since the pciConnectFlags checking is
> +                 * more relaxed when an address is already assigned
> +                 * than it is when we're looking for a new address (so
> +                 * validation will pass regardless of whether we set
> +                 * the flags to PCI or PCIE).

s/PCIE/PCIe/

[...]
> +            /* If we are running with privileges, we can examine the
> +             * PCI config contents with virPCIDeviceIsPCIExpress() for
> +             * a definitive answer.
> +             */
> +            isExpress = virPCIDeviceIsPCIExpress(pciDev);
> +            virPCIDeviceFree(pciDev);
> +
> +            if (isExpress)
> +                return pcieFlags;
> +
> +            return pciFlags;
> +        }

Empty line here, please.

> +        return 0;
> +    }
>  
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>          switch ((virDomainMemballoonModel) dev->data.memballoon->model) {

ACK (with the nits fixed) regardless of whether you decide to
go with my suggestion of reducing the indentation level.

-- 
Andrea Bolognani / Red Hat / Virtualization