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

[Cole Robinson]

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

- Cole