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

Michal Privoznik posted 30 patches 42 weeks ago

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

Posted by Michal Privoznik 42 weeks ago
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@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;
+        }
     }
 
     /* qemu-pr-helper might require access to /dev/mapper/control. */
@@ -13447,6 +13460,10 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
         qemuDomainCreateDevice(QEMU_DEVICE_MAPPER_CONTROL_PATH, data, true) < 0)
         goto cleanup;
 
+    if (hasNVMe &&
+        qemuDomainCreateDevice(QEMU_DEV_VFIO, data, false) < 0)
+        goto cleanup;
+
     ret = 0;
  cleanup:
     VIR_FREE(dst);
@@ -14461,19 +14478,32 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
                              virStorageSourcePtr src)
 {
     virStorageSourcePtr next;
-    const char **paths = NULL;
+    char **paths = NULL;
     size_t npaths = 0;
-    char *dmPath = NULL;
+    bool hasNVMe = false;
+    g_autofree char *dmPath = NULL;
+    g_autofree char *vfioPath = NULL;
     int ret = -1;
 
     for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) {
-        if (virStorageSourceIsEmpty(next) ||
-            !virStorageSourceIsLocalStorage(next)) {
-            /* Not creating device. Just continue. */
-            continue;
+        g_autofree char *tmpPath = NULL;
+
+        if (next->type == VIR_STORAGE_TYPE_NVME) {
+            hasNVMe = true;
+
+            if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr)))
+                goto cleanup;
+        } else {
+            if (virStorageSourceIsEmpty(next) ||
+                !virStorageSourceIsLocalStorage(next)) {
+                /* Not creating device. Just continue. */
+                continue;
+            }
+
+            tmpPath = g_strdup(next->path);
         }
 
-        if (VIR_APPEND_ELEMENT_COPY(paths, npaths, next->path) < 0)
+        if (VIR_APPEND_ELEMENT(paths, npaths, tmpPath) < 0)
             goto cleanup;
     }
 
@@ -14484,13 +14514,18 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
             goto cleanup;
     }
 
-    if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0)
+    if (hasNVMe) {
+        vfioPath = g_strdup(QEMU_DEV_VFIO);
+        if (VIR_APPEND_ELEMENT(paths, npaths, vfioPath) < 0)
+            goto cleanup;
+    }
+
+    if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths, npaths) < 0)
         goto cleanup;
 
     ret = 0;
  cleanup:
-    VIR_FREE(dmPath);
-    VIR_FREE(paths);
+    virStringListFreeCount(paths, npaths);
     return ret;
 }
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Cole Robinson 41 weeks ago
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@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@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

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Privoznik 41 weeks ago
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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list