Re: [libvirt] [PATCH v3 20/30] qemu: Create NVMe disk in domain namespace
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
---
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(>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
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 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))
if (next->type == NVME)
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
}
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
Re: [libvirt] [PATCH v3 20/30] qemu: Create NVMe disk in domain namespace
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
> ---
> 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(>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
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 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))
if (next->type == NVME)
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
}
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
[libvirt] [PATCH v3 20/30] qemu: Create NVMe disk in domain namespace
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
---
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(>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(>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
