[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