[libvirt] [PATCH v1 23/31] qemu_domain: Introduce NVMe path getting helpers

Michal Privoznik mprivozn at redhat.com
Fri Aug 2 15:20:09 UTC 2019


On 7/16/19 4:30 PM, Peter Krempa wrote:
> On Thu, Jul 11, 2019 at 17:54:10 +0200, Michal Privoznik wrote:
>> Couple of places in the QEMU driver will want to know what paths
>> are associated with NVMe disks (for instance CGroup code or
>> namespaces code). Introduce helpers which return desired paths
>> (for instance /dev/vfio/vfio and /dev/vfio/N).
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_domain.h |  6 ++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 2a7f09ce24..949bbace88 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -11723,6 +11723,50 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
>>   }
>>   
>>   
>> +char *
>> +qemuDomainGetNVMeDiskPath(const virStorageSourceNVMeDef *nvme)
> 
> Name of this function is VERY misleading. It returns path of the IOMMU
> group associated with the host portion of the NVMe device, but the name
> implies smething completely different.
> 
> Include IOMMUGroup in the name and add a comment.
> 
>> +{
>> +    VIR_AUTOPTR(virPCIDevice) pci = NULL;
>> +
>> +    /* All NVMe devices are VFIO PCI devices */
>> +    if (!(pci = virPCIDeviceNew(nvme->pciAddr.domain,
>> +                                nvme->pciAddr.bus,
>> +                                nvme->pciAddr.slot,
>> +                                nvme->pciAddr.function)))
>> +        return NULL;
>> +
>> +    return virPCIDeviceGetIOMMUGroupDev(pci);
>> +}
>> +
>> +
>> +char **
>> +qemuDomainGetDiskNVMePaths(const virDomainDef *def,
> 
> Also this name looks troubling.
> 
>> +                           const virStorageSource *src,
>> +                           bool teardown)
>> +{
>> +    VIR_AUTOFREE(char *) iommuGroup = NULL;
>> +    VIR_AUTOSTRINGLIST paths = NULL;
>> +    bool includeVFIO = !teardown;
>> +
>> +    if (!(iommuGroup = qemuDomainGetNVMeDiskPath(src->nvme)))
>> +        return NULL;
>> +
>> +    if (virStringListAdd(&paths, iommuGroup) < 0)
>> +        return NULL;
>> +
>> +    if (teardown && def &&
>> +        !virDomainDefHasNVMeDisk(def) &&
>> +        !virDomainDefHasVFIOHostdev(def))
>> +        includeVFIO = true;
>> +
>> +    if (includeVFIO &&
>> +        virStringListAdd(&paths, QEMU_DEV_VFIO) < 0)
>> +        return NULL;
> 
> I don't like this. It's hiding the stuff necessary to detach VFIO groups
> in random function and the hostdev code will require exactly the same
> treatment. Additionally any further possible VFIO based device would
> require 3 places.
> 
> Can't we consolidate that somehow?
> 

Agreed it's ugly. But I don't have any idea, sorry. Do you have any 
suggestion?

Michal




More information about the libvir-list mailing list