[libvirt] [PATCH v4 03/10] Refuse to reattach from vfio if the iommu group is in use by any domain

[Shivaprasad G Bhat]

On 11/20/2015 08:51 PM, Laine Stump wrote:
> On 11/14/2015 03:36 AM, Shivaprasad G Bhat wrote:
>
> > Refuse to reattach from vfio if the iommu group is in use by any domain
>
> util: refuse to reattach device if any device in the same group is in 
> use by any domain
>
>> It is incorrect to attempt the device reattach of a function,
>
> s/function/device/
>
>> when some other domain is using some functions belonging to the same 
>> iommu
>> group.
>
> s/some other domain/any other domain/
> s/some functions/any device/
>
>>
>> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
>> ---
>>   src/util/virhostdev.c |   21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index de029a0..f24ccd8 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
>> @@ -1590,18 +1590,35 @@ 
>> virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
>>                                   virPCIDevicePtr pci)
>>   {
>>       virPCIDeviceAddressPtr devAddr = NULL;
>> +    bool usesVfio = false;
>> +    char *drvPath = NULL;
>> +    char *drvName = NULL;
>>       struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, 
>> NULL,
>>                                                        false};
>>       int ret = -1;
>>   +    if (virPCIDeviceGetDriverPathAndName(pci, &drvPath, &drvName) 
>> < 0)
>> +        goto out;
>> +
>> +    if (STREQ_NULLABLE(drvName, "vfio-pci"))
>> +        usesVfio = true;
>> +
>>       virObjectLock(hostdev_mgr->activePCIHostdevs);
>>       virObjectLock(hostdev_mgr->inactivePCIHostdevs);
>>         if (!(devAddr = virPCIDeviceGetAddress(pci)))
>>           goto out;
>>   -    if (virHostdevIsPCINodeDeviceUsed(devAddr, &data))
>> +    if (usesVfio) {
>> +    /* Doesn't matter which device. If any domain is actively using the
>> +     * iommu group, refuse to reattach */
>> +        if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
>> + virHostdevIsPCINodeDeviceUsed,
>> +                                                 &data) < 0)
>> +            goto out;
>
> Calling the iterator seems very inefficient to me (which is why I 
> suggested in the original BZ comments that we save the iommuGroup of 
> each device to the activelist):
>
> 1) you know the iommu group of the current PCI device (it is in 
> pci->iommuGroup)
>
> 2) all that virHostdevIsPCINodeDeviceUsed does is check through 
> activePCIHostdevs for the device, and this iterator is just calling 
> virHostdevIsPCINodeDeviceUsed for each device in the same iommuGroup 
> as the current device.
>
> Therefore, all you really need to do is look for any devices in 
> acticePCIHostdevs that have dev->iommuGroup == pci->iommuGroup.
>
> Calling the iterator results in a lot of accesses to sysfs, opening 
> and closing files, printfs, etc. Unless that is necessary to catch 
> some extra case (and I don't see that), it's much better to do it the 
> simpler way - more efficient and easier to understand a year from now 
> when we come back to this.
>
I always felt relying on the activelist is not possible as its lost 
during libvirt restart. Now that you point it out, I realise the 
activelist is populated back during libvirt domain discovery so
activelist is actually reliable. I'll change this to use activelist instead.

Thanks,
Shivaprasad

>> +    } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) {
>>           goto out;
>> +    }
>>         virPCIDeviceReattachInit(pci);
>>   @@ -1614,6 +1631,8 @@ 
>> virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
>>       virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
>>       virObjectUnlock(hostdev_mgr->activePCIHostdevs);
>>       VIR_FREE(devAddr);
>> +    VIR_FREE(drvPath);
>> +    VIR_FREE(drvName);
>>       return ret;
>>   }
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>