[libvirt] [PATCH v6 17/17] qemu: assign vfio devices to PCIe addresses when appropriate

[Laine Stump]

On 11/07/2016 03:26 PM, Alex Williamson wrote:
> On Mon,  7 Nov 2016 14:50:25 -0500
> Laine Stump <laine at laine.org> wrote:
>
>> Although nearly all host devices that are assigned to guests using
>> vfio ("<hostdev>" devices in libvirt) are physically PCI Express
>> devices, until now libvirt's PCI address assignment has always
>> assigned them addresses on legacy PCI controllers.
>>
>> This patch tries to assign them to an address on a PCIe controller
>> instead. First we do some preliminary checks that might allow setting
>> the flags without doing any extra work, and if those conditions aren't
>> met (and if libvirt is running privileged so that it has proper
>> permissions), we perform the (relatively) time consuming task of
>> reading the device's PCI config to see if it is an Express device. If
>> this is successful, the connect flags are set based on the result, but
>> if we aren't able to read the PCI config (most likely due to the
>> device not being present on the system at the time of the check) we
>> assume it is (or will be) and Express device, since that is almost
>> always the case anyway.
>> ---
>>   src/qemu/qemu_domain_address.c | 66 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>> index 65753c5..86984fb 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -428,7 +428,7 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
>>    */
>>   static virDomainPCIConnectFlags
>>   qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>> -                                         virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>> +                                         virQEMUDriverPtr driver,
>>                                            virDomainPCIConnectFlags pcieFlags,
>>                                            virDomainPCIConnectFlags virtioFlags)
>>   {
>> @@ -558,8 +558,68 @@ 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) {
>> +            bool isExpress = false;
>> +            virPCIDevicePtr pciDev;
>> +            virPCIDeviceAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
>> +
>> +            if (pciFlags == pcieFlags) {
>> +                /* This arch/qemu only supports legacy PCI, so there
>> +                 * is no point in checking if the device is an Express
>> +                 * device.
>> +                 */
>> +                return pciFlags;
>> +            }
>> +
>> +            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).
>> +                 */
>> +                return pcieFlags;
>> +            }
>> +
>> +            if (!driver->privileged) {
>> +                /* unprivileged libvirtd is unable to read a device's
>> +                 * PCI config, so instead of trying and failing, we
>> +                 * will just assume what is by far the most likely
>> +                 * version of reality: this is a PCIE device.
>> +                 */
>> +                return pcieFlags;
>
> Don't you still have permission to stat the config file?  You can tell
> pretty well by whether the config file is 4096 bytes that the device is
> either PCIe or PCIx (and given the rarity of PCIx in most systems
> assume that it's PCIe).  Conventional PCI will be 256 bytes.  Thanks,

Interesting idea. Although still not 100%, it would reduce the 
possibility of false positives to "nearly 0".