[libvirt] [PATCH v3 20/30] qemu: Create NVMe disk in domain namespace

[Michal Privoznik]

On 12/10/19 9:16 PM, Cole Robinson wrote:
> On 12/2/19 9:26 AM, Michal Privoznik wrote:
>> If a domain has an NVMe disk configured, then we need to create
>> /dev/vfio/* paths in domain's namespace so that qemu can open
>> them.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_domain.c | 67 ++++++++++++++++++++++++++++++++----------
>>   1 file changed, 51 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 7b515b9520..70c4ee8e65 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -13430,16 +13430,29 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
>>   {
>>       virStorageSourcePtr next;
>>       char *dst = NULL;
>> +    bool hasNVMe = false;
>>       int ret = -1;
>>   
>>       for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {
>> -        if (!next->path || !virStorageSourceIsLocalStorage(next)) {
>> -            /* Not creating device. Just continue. */
>> -            continue;
>> -        }
>> +        if (next->type == VIR_STORAGE_TYPE_NVME) {
>> +            g_autofree char *nvmePath = NULL;
>>   
>> -        if (qemuDomainCreateDevice(next->path, data, false) < 0)
>> -            goto cleanup;
>> +            hasNVMe = true;
>> +
>> +            if (!(nvmePath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr)))
>> +                goto cleanup;
>> +
>> +            if (qemuDomainCreateDevice(nvmePath, data, false) < 0)
>> +                goto cleanup;
>> +        } else {
>> +            if (!next->path || !virStorageSourceIsLocalStorage(next)) {
>> +                /* Not creating device. Just continue. */
>> +                continue;
>> +            }
>> +
>> +            if (qemuDomainCreateDevice(next->path, data, false) < 0)
>> +                goto cleanup;
>> +        }
>>       }
>>
> 
> I don't see anything wrong with this patch. In the interest of getting
> this series into git, here is my:
> 
> Reviewed-by: Cole Robinson <crobinso at redhat.com>
> 
> 
> But, I'm finding a lot of the 'next->type == VIR_STORAGE_TYPE_NVME'
> sprinkled around unfortunate, here and in later patches. It seems like
> there's two StorageSource cases here involving local filesystem paths:
> 
> 1) Does src->path contain the <source> storage location? Answered by
> virStorageSourceIsLocalStorage
> 2) Does src have a local path that the VM needs to access? Until now
> this was always the same as #1 && src->path, but not any more.
> 
> So maybe add helpers like:
> 
> bool
> virStorageSourceHasLocalResource(virStorageSourcePtr src)
> {
>      return next->path || next->type == VIR_STORAGE_TYPE_NVME;
> }
> 
> const char *
> virStorageSourceGetLocalResource(virStorageSourcePtr src)
> {
>      if (!virStorageSourceHasLocalResource(src))
>          <error>
> 
>      if (next->type == NVME)
>          <return VFIO path or error>
> 
>      return strdup(next->path);
> }
> 
> I think virStorageSourceIsLocalStorage should be renamed too because now
> it is ambiguous. Maybe something more explicit like SourceUsesPath, or
> SourceIsLocalPath. Naming sucks obviously so suggestions welcome
> 
> With those funtions, the loop above becomes:
> 
> for (...) {
>      g_autofree char *path = NULL;
>      if (!HasLocalResource)
>          continue
>      if (!(path = GetLocalResource(...))
>          error
>      <do stuff with path>
> }
> 
> The hasNVMe bit can then be replaced with virStorageSourceChainHasNVMe
> 
> security driver usage will be simpler too I think. But maybe I'm missing
> some complication

I think you're right. I will save that to a follow up series.

Michal