In near future, the decision what to do with /dev/vfio/vfio with respect to domain namespace and CGroup is going to be moved out of qemuDomainGetHostdevPath() because there will be some other types of devices than hostdevs that need access to VFIO. All functions that I'm changing (except qemuSetupHostdevCgroup()) assume that hostdev we are adding/removing to VM is not in the definition yet (because of how qemuDomainNeedsVFIO() is written). Fortunately, this assumption is true. For qemuSetupHostdevCgroup(), the worst thing that may happen is that we allow /dev/vfio/vfio which was already allowed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 47 +++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.c | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 31cf71146b..65b148cd83 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -25,6 +25,7 @@ #include "qemu_domain.h" #include "qemu_process.h" #include "qemu_extdevice.h" +#include "qemu_hostdev.h" #include "virlog.h" #include "viralloc.h" #include "virerror.h" @@ -359,6 +360,17 @@ qemuTeardownInputCgroup(virDomainObjPtr vm, } +/** + * qemuSetupHostdevCgroup: + * vm: domain object + * @dev: device to allow + * + * For given host device @dev allow access to in Cgroups. + * Note, @dev must not be in @vm's definition. + * + * Returns: 0 on success, + * -1 otherwise. + */ int qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) @@ -385,6 +397,16 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, goto cleanup; } + if (qemuHostdevNeedsVFIO(dev)) { + VIR_DEBUG("Cgroup allow %s perms=%d", QEMU_DEV_VFIO, VIR_CGROUP_DEVICE_RW); + rv = virCgroupAllowDevicePath(priv->cgroup, QEMU_DEV_VFIO, + VIR_CGROUP_DEVICE_RW, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", + QEMU_DEV_VFIO, "rw", rv); + if (rv < 0) + goto cleanup; + } + ret = 0; cleanup: @@ -395,9 +417,21 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, return ret; } + +/** + * qemuTeardownHostdevCgroup: + * @vm: doamin object + * @dev: device to tear down + * + * For given host device @dev deny access to it in CGroups. + * Note, @dev must not be in @vm's definition. + * + * Returns: 0 on success, + * -1 otherwise. + */ int qemuTeardownHostdevCgroup(virDomainObjPtr vm, - virDomainHostdevDefPtr dev) + virDomainHostdevDefPtr dev) { qemuDomainObjPrivatePtr priv = vm->privateData; char **path = NULL; @@ -421,6 +455,17 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, goto cleanup; } + if (qemuHostdevNeedsVFIO(dev) && + !qemuDomainNeedsVFIO(vm->def)) { + VIR_DEBUG("Cgroup deny " QEMU_DEV_VFIO); + rv = virCgroupDenyDevicePath(priv->cgroup, QEMU_DEV_VFIO, + VIR_CGROUP_DEVICE_RWM, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + QEMU_DEV_VFIO, "rwm", rv); + if (rv < 0) + goto cleanup; + } + ret = 0; cleanup: for (i = 0; i < npaths; i++) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d1596a28ca..550ae848df 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13542,6 +13542,10 @@ qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, goto cleanup; } + if (qemuHostdevNeedsVFIO(dev) && + qemuDomainCreateDevice(QEMU_DEV_VFIO, data, false) < 0) + goto cleanup; + ret = 0; cleanup: for (i = 0; i < npaths; i++) @@ -14565,6 +14569,17 @@ qemuDomainNamespaceTeardownDisk(virDomainObjPtr vm G_GNUC_UNUSED, } +/** + * qemuDomainNamespaceSetupHostdev: + * @vm: domain object + * @hostdev: hostdev to create in @vm's namespace + * + * For given @hostdev, create its devfs representation (if it has one) in + * domain namespace. Note, @hostdev must not be in @vm's definition. + * + * Returns: 0 on success, + * -1 otherwise. + */ int qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -14579,6 +14594,11 @@ qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, if (qemuDomainNamespaceMknodPaths(vm, (const char **)paths, npaths) < 0) goto cleanup; + if (qemuHostdevNeedsVFIO(hostdev) && + !qemuDomainNeedsVFIO(vm->def) && + qemuDomainNamespaceMknodPath(vm, QEMU_DEV_VFIO) < 0) + goto cleanup; + ret = 0; cleanup: for (i = 0; i < npaths; i++) @@ -14588,6 +14608,17 @@ qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, } +/** + * qemuDomainNamespaceTeardownHostdev: + * @vm: domain object + * @hostdev: hostdev to remove in @vm's namespace + * + * For given @hostdev, remove its devfs representation (if it has one) in + * domain namespace. Note, @hostdev must not be in @vm's definition. + * + * Returns: 0 on success, + * -1 otherwise. + */ int qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -14603,6 +14634,11 @@ qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, if (qemuDomainNamespaceUnlinkPaths(vm, (const char **)paths, npaths) < 0) goto cleanup; + if (qemuHostdevNeedsVFIO(hostdev) && + !qemuDomainNeedsVFIO(vm->def) && + qemuDomainNamespaceUnlinkPath(vm, QEMU_DEV_VFIO) < 0) + goto cleanup; + ret = 0; cleanup: for (i = 0; i < npaths; i++) -- 2.23.0

There are several variables which could be automatically freed upon return from the function. I'm not changing @tmpPaths (which is a string list) because it is going to be removed in next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 550ae848df..6daae24c3d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12891,14 +12891,14 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; - virPCIDevicePtr pci = NULL; - virUSBDevicePtr usb = NULL; - virSCSIDevicePtr scsi = NULL; - virSCSIVHostDevicePtr host = NULL; - char *tmpPath = NULL; + g_autoptr(virPCIDevice) pci = NULL; + g_autoptr(virUSBDevice) usb = NULL; + g_autoptr(virSCSIDevice) scsi = NULL; + g_autoptr(virSCSIVHostDevice) host = NULL; + g_autofree char *tmpPath = NULL; bool includeVFIO = false; char **tmpPaths = NULL; - int *tmpPerms = NULL; + g_autofree int *tmpPerms = NULL; size_t tmpNpaths = 0; int perm = 0; @@ -13025,12 +13025,6 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, ret = 0; cleanup: virStringListFreeCount(tmpPaths, tmpNpaths); - VIR_FREE(tmpPerms); - virPCIDeviceFree(pci); - virUSBDeviceFree(usb); - virSCSIDeviceFree(scsi); - virSCSIVHostDeviceFree(host); - VIR_FREE(tmpPath); return ret; } -- 2.23.0

Now that all callers of qemuDomainGetHostdevPath() handle /dev/vfio/vfio on their own, we can safely drop handling in this function. In near future the decision whether domain needs VFIO file is going to include more device types than just virDomainHostdev. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 52 ++++++++-------------- src/qemu/qemu_domain.c | 98 +++++++++--------------------------------- src/qemu/qemu_domain.h | 9 ++-- 3 files changed, 42 insertions(+), 117 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 65b148cd83..d54d01d4c0 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -376,26 +376,23 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) { qemuDomainObjPrivatePtr priv = vm->privateData; - char **path = NULL; - int *perms = NULL; - size_t i, npaths = 0; + g_autofree char *path = NULL; + int perms; int rv, ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, &perms) < 0) + if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0) goto cleanup; - for (i = 0; i < npaths; i++) { - VIR_DEBUG("Cgroup allow %s perms=%d", path[i], perms[i]); - rv = virCgroupAllowDevicePath(priv->cgroup, path[i], perms[i], false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path[i], - virCgroupGetDevicePermsString(perms[i]), - rv); - if (rv < 0) - goto cleanup; - } + VIR_DEBUG("Cgroup allow %s perms=%d", path, perms); + rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, + virCgroupGetDevicePermsString(perms), + rv); + if (rv < 0) + goto cleanup; if (qemuHostdevNeedsVFIO(dev)) { VIR_DEBUG("Cgroup allow %s perms=%d", QEMU_DEV_VFIO, VIR_CGROUP_DEVICE_RW); @@ -410,10 +407,6 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, ret = 0; cleanup: - for (i = 0; i < npaths; i++) - VIR_FREE(path[i]); - VIR_FREE(path); - VIR_FREE(perms); return ret; } @@ -434,26 +427,22 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) { qemuDomainObjPrivatePtr priv = vm->privateData; - char **path = NULL; - size_t i, npaths = 0; + g_autofree char *path = NULL; int rv, ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - if (qemuDomainGetHostdevPath(vm->def, dev, true, - &npaths, &path, NULL) < 0) + if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) goto cleanup; - for (i = 0; i < npaths; i++) { - VIR_DEBUG("Cgroup deny %s", path[i]); - rv = virCgroupDenyDevicePath(priv->cgroup, path[i], - VIR_CGROUP_DEVICE_RWM, false); - virDomainAuditCgroupPath(vm, priv->cgroup, - "deny", path[i], "rwm", rv); - if (rv < 0) - goto cleanup; - } + VIR_DEBUG("Cgroup deny %s", path); + rv = virCgroupDenyDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RWM, false); + virDomainAuditCgroupPath(vm, priv->cgroup, + "deny", path, "rwm", rv); + if (rv < 0) + goto cleanup; if (qemuHostdevNeedsVFIO(dev) && !qemuDomainNeedsVFIO(vm->def)) { @@ -468,9 +457,6 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, ret = 0; cleanup: - for (i = 0; i < npaths; i++) - VIR_FREE(path[i]); - VIR_FREE(path); return ret; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6daae24c3d..be769cb3ff 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12861,29 +12861,23 @@ qemuDomainNeedsVFIO(const virDomainDef *def) /** * qemuDomainGetHostdevPath: - * @def: domain definition * @dev: host device definition - * @teardown: true if device will be removed - * @npaths: number of items in @path and @perms arrays * @path: resulting path to @dev * @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms * * For given device @dev fetch its host path and store it at - * @path. If a device requires other paths to be present/allowed - * they are stored in the @path array after the actual path. - * Optionally, caller can get @perms on the path (e.g. rw/ro). + * @path. Optionally, caller can get @perms on the path (e.g. + * rw/ro). * - * The caller is responsible for freeing the memory. + * The caller is responsible for freeing the @path when no longer + * needed. * * Returns 0 on success, -1 otherwise. */ int -qemuDomainGetHostdevPath(virDomainDefPtr def, - virDomainHostdevDefPtr dev, - bool teardown, - size_t *npaths, - char ***path, - int **perms) +qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, + char **path, + int *perms) { int ret = -1; virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; @@ -12896,14 +12890,9 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, g_autoptr(virSCSIDevice) scsi = NULL; g_autoptr(virSCSIVHostDevice) host = NULL; g_autofree char *tmpPath = NULL; - bool includeVFIO = false; - char **tmpPaths = NULL; g_autofree int *tmpPerms = NULL; - size_t tmpNpaths = 0; int perm = 0; - *npaths = 0; - switch ((virDomainHostdevMode) dev->mode) { case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: switch ((virDomainHostdevSubsysType)dev->source.subsys.type) { @@ -12920,12 +12909,6 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, goto cleanup; perm = VIR_CGROUP_DEVICE_RW; - if (teardown) { - if (!virDomainDefHasVFIOHostdev(def)) - includeVFIO = true; - } else { - includeVFIO = true; - } } break; @@ -12981,7 +12964,6 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, if (!(tmpPath = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) goto cleanup; - includeVFIO = true; perm = VIR_CGROUP_DEVICE_RW; break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: @@ -12995,36 +12977,11 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, break; } - if (tmpPath) { - size_t toAlloc = 1; - - if (includeVFIO) - toAlloc = 2; - - if (VIR_ALLOC_N(tmpPaths, toAlloc) < 0 || - VIR_ALLOC_N(tmpPerms, toAlloc) < 0) - goto cleanup; - tmpPaths[0] = g_strdup(tmpPath); - tmpNpaths = toAlloc; - tmpPerms[0] = perm; - - if (includeVFIO) { - tmpPaths[1] = g_strdup(QEMU_DEV_VFIO); - tmpPerms[1] = VIR_CGROUP_DEVICE_RW; - } - } - - *npaths = tmpNpaths; - tmpNpaths = 0; - *path = tmpPaths; - tmpPaths = NULL; - if (perms) { - *perms = tmpPerms; - tmpPerms = NULL; - } + *path = g_steal_pointer(&tmpPath); + if (perms) + *perms = perm; ret = 0; cleanup: - virStringListFreeCount(tmpPaths, tmpNpaths); return ret; } @@ -13525,16 +13482,13 @@ qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, const struct qemuDomainCreateDeviceData *data) { int ret = -1; - char **path = NULL; - size_t i, npaths = 0; + g_autofree char *path = NULL; - if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, NULL) < 0) + if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) goto cleanup; - for (i = 0; i < npaths; i++) { - if (qemuDomainCreateDevice(path[i], data, false) < 0) - goto cleanup; - } + if (qemuDomainCreateDevice(path, data, false) < 0) + goto cleanup; if (qemuHostdevNeedsVFIO(dev) && qemuDomainCreateDevice(QEMU_DEV_VFIO, data, false) < 0) @@ -13542,9 +13496,6 @@ qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, ret = 0; cleanup: - for (i = 0; i < npaths; i++) - VIR_FREE(path[i]); - VIR_FREE(path); return ret; } @@ -14579,13 +14530,12 @@ qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { int ret = -1; - char **paths = NULL; - size_t i, npaths = 0; + g_autofree char *path = NULL; - if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &paths, NULL) < 0) + if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) goto cleanup; - if (qemuDomainNamespaceMknodPaths(vm, (const char **)paths, npaths) < 0) + if (qemuDomainNamespaceMknodPath(vm, path) < 0) goto cleanup; if (qemuHostdevNeedsVFIO(hostdev) && @@ -14595,9 +14545,6 @@ qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, ret = 0; cleanup: - for (i = 0; i < npaths; i++) - VIR_FREE(paths[i]); - VIR_FREE(paths); return ret; } @@ -14618,14 +14565,12 @@ qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { int ret = -1; - char **paths = NULL; - size_t i, npaths = 0; + g_autofree char *path = NULL; - if (qemuDomainGetHostdevPath(vm->def, hostdev, true, - &npaths, &paths, NULL) < 0) + if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) goto cleanup; - if (qemuDomainNamespaceUnlinkPaths(vm, (const char **)paths, npaths) < 0) + if (qemuDomainNamespaceUnlinkPath(vm, path) < 0) goto cleanup; if (qemuHostdevNeedsVFIO(hostdev) && @@ -14635,9 +14580,6 @@ qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, ret = 0; cleanup: - for (i = 0; i < npaths; i++) - VIR_FREE(paths[i]); - VIR_FREE(paths); return ret; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index db45a932dc..65bd83aece 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1079,12 +1079,9 @@ bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, bool qemuDomainNeedsVFIO(const virDomainDef *def); -int qemuDomainGetHostdevPath(virDomainDefPtr def, - virDomainHostdevDefPtr dev, - bool teardown, - size_t *npaths, - char ***path, - int **perms); +int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, + char **path, + int *perms); int qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, virSecurityManagerPtr mgr, -- 2.23.0

Previous patches rendered some of 'cleanup' labels needless. Drop them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 25 +++++++++------------ src/qemu/qemu_domain.c | 50 ++++++++++++++++-------------------------- 2 files changed, 29 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d54d01d4c0..d23318db8c 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -378,13 +378,13 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; g_autofree char *path = NULL; int perms; - int rv, ret = -1; + int rv; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0) - goto cleanup; + return -1; VIR_DEBUG("Cgroup allow %s perms=%d", path, perms); rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, false); @@ -392,7 +392,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, virCgroupGetDevicePermsString(perms), rv); if (rv < 0) - goto cleanup; + return -1; if (qemuHostdevNeedsVFIO(dev)) { VIR_DEBUG("Cgroup allow %s perms=%d", QEMU_DEV_VFIO, VIR_CGROUP_DEVICE_RW); @@ -401,13 +401,10 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainAuditCgroupPath(vm, priv->cgroup, "allow", QEMU_DEV_VFIO, "rw", rv); if (rv < 0) - goto cleanup; + return -1; } - ret = 0; - - cleanup: - return ret; + return 0; } @@ -428,13 +425,13 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; g_autofree char *path = NULL; - int rv, ret = -1; + int rv; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) - goto cleanup; + return -1; VIR_DEBUG("Cgroup deny %s", path); rv = virCgroupDenyDevicePath(priv->cgroup, path, @@ -442,7 +439,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rv); if (rv < 0) - goto cleanup; + return -1; if (qemuHostdevNeedsVFIO(dev) && !qemuDomainNeedsVFIO(vm->def)) { @@ -452,12 +449,10 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, virDomainAuditCgroupPath(vm, priv->cgroup, "deny", QEMU_DEV_VFIO, "rwm", rv); if (rv < 0) - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be769cb3ff..21a4dad37d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12879,7 +12879,6 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, char **path, int *perms) { - int ret = -1; virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; @@ -12903,10 +12902,10 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, pcisrc->addr.slot, pcisrc->addr.function); if (!pci) - goto cleanup; + return -1; if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci))) - goto cleanup; + return -1; perm = VIR_CGROUP_DEVICE_RW; } @@ -12919,7 +12918,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, usbsrc->device, NULL); if (!usb) - goto cleanup; + return -1; tmpPath = g_strdup(virUSBDeviceGetPath(usb)); perm = VIR_CGROUP_DEVICE_RW; @@ -12940,7 +12939,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, dev->shareable); if (!scsi) - goto cleanup; + return -1; tmpPath = g_strdup(virSCSIDeviceGetPath(scsi)); perm = virSCSIDeviceGetReadonly(scsi) ? @@ -12952,7 +12951,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, if (hostsrc->protocol == VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) { if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn))) - goto cleanup; + return -1; tmpPath = g_strdup(virSCSIVHostDeviceGetPath(host)); perm = VIR_CGROUP_DEVICE_RW; @@ -12962,7 +12961,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: if (!(tmpPath = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) - goto cleanup; + return -1; perm = VIR_CGROUP_DEVICE_RW; break; @@ -12980,9 +12979,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, *path = g_steal_pointer(&tmpPath); if (perms) *perms = perm; - ret = 0; - cleanup: - return ret; + return 0; } @@ -13481,22 +13478,19 @@ qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, virDomainHostdevDefPtr dev, const struct qemuDomainCreateDeviceData *data) { - int ret = -1; g_autofree char *path = NULL; if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) - goto cleanup; + return -1; if (qemuDomainCreateDevice(path, data, false) < 0) - goto cleanup; + return -1; if (qemuHostdevNeedsVFIO(dev) && qemuDomainCreateDevice(QEMU_DEV_VFIO, data, false) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -14529,23 +14523,20 @@ int qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - int ret = -1; g_autofree char *path = NULL; if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) - goto cleanup; + return -1; if (qemuDomainNamespaceMknodPath(vm, path) < 0) - goto cleanup; + return -1; if (qemuHostdevNeedsVFIO(hostdev) && !qemuDomainNeedsVFIO(vm->def) && qemuDomainNamespaceMknodPath(vm, QEMU_DEV_VFIO) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -14564,23 +14555,20 @@ int qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - int ret = -1; g_autofree char *path = NULL; if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) - goto cleanup; + return -1; if (qemuDomainNamespaceUnlinkPath(vm, path) < 0) - goto cleanup; + return -1; if (qemuHostdevNeedsVFIO(hostdev) && !qemuDomainNeedsVFIO(vm->def) && qemuDomainNamespaceUnlinkPath(vm, QEMU_DEV_VFIO) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } -- 2.23.0

Sometimes, we have a PCI address and not fully allocated virPCIDevice and yet we still want to know its /dev/vfio/N path. Introduce virPCIDeviceAddressGetIOMMUGroupDev() function exactly for that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 11 +---------- src/util/virpci.c | 15 +++++++++++++++ src/util/virpci.h | 1 + 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8fe0bf9365..0988f36c20 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2692,6 +2692,7 @@ virPCIDeviceAddressAsString; virPCIDeviceAddressEqual; virPCIDeviceAddressFree; virPCIDeviceAddressGetIOMMUGroupAddresses; +virPCIDeviceAddressGetIOMMUGroupDev; virPCIDeviceAddressGetIOMMUGroupNum; virPCIDeviceAddressGetSysfsFile; virPCIDeviceAddressIOMMUGroupIterate; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 21a4dad37d..59e98de2d2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12884,12 +12884,10 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; - g_autoptr(virPCIDevice) pci = NULL; g_autoptr(virUSBDevice) usb = NULL; g_autoptr(virSCSIDevice) scsi = NULL; g_autoptr(virSCSIVHostDevice) host = NULL; g_autofree char *tmpPath = NULL; - g_autofree int *tmpPerms = NULL; int perm = 0; switch ((virDomainHostdevMode) dev->mode) { @@ -12897,14 +12895,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, switch ((virDomainHostdevSubsysType)dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - pci = virPCIDeviceNew(pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); - if (!pci) - return -1; - - if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci))) + if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&pcisrc->addr))) return -1; perm = VIR_CGROUP_DEVICE_RW; diff --git a/src/util/virpci.c b/src/util/virpci.c index 9bea5a20d0..b0a4107551 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1942,6 +1942,21 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) } +char * +virPCIDeviceAddressGetIOMMUGroupDev(const virPCIDeviceAddress *devAddr) +{ + g_autoptr(virPCIDevice) pci = NULL; + + if (!(pci = virPCIDeviceNew(devAddr->domain, + devAddr->bus, + devAddr->slot, + devAddr->function))) + return NULL; + + return virPCIDeviceGetIOMMUGroupDev(pci); +} + + /* virPCIDeviceGetIOMMUGroupDev - return the name of the device used * to control this PCI device's group (e.g. "/dev/vfio/15") */ diff --git a/src/util/virpci.h b/src/util/virpci.h index cfb4581edf..1c94dc307c 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -195,6 +195,7 @@ int virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr, virPCIDeviceAddressPtr **iommuGroupDevices, size_t *nIommuGroupDevices); int virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr); +char *virPCIDeviceAddressGetIOMMUGroupDev(const virPCIDeviceAddress *devAddr); char *virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev); int virPCIDeviceIsAssignable(virPCIDevicePtr dev, -- 2.23.0

In near future we will have a list of PCI devices we want to detach (held in virPCIDeviceListPtr) but we don't have virDomainHostdevDefPtr. That's okay because virHostdevPreparePCIDevices() works with virPCIDeviceListPtr mostly anyway. And in very few places where it needs virDomainHostdevDefPtr are not interesting for our case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ACKed-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhostdev.c | 48 +++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 39e6b8f49f..b8e6e2bc36 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -714,27 +714,22 @@ virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr, } } -int -virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, - const char *drv_name, - const char *dom_name, - const unsigned char *uuid, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs, - unsigned int flags) + +static int +virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + const unsigned char *uuid, + virPCIDeviceListPtr pcidevs, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + unsigned int flags) { - g_autoptr(virPCIDeviceList) pcidevs = NULL; int last_processed_hostdev_vf = -1; size_t i; int ret = -1; virPCIDeviceAddressPtr devAddr = NULL; - if (!nhostdevs) - return 0; - - if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) - return -1; - virObjectLock(mgr->activePCIHostdevs); virObjectLock(mgr->inactivePCIHostdevs); @@ -984,6 +979,29 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, return ret; } + +int +virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + const unsigned char *uuid, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + unsigned int flags) +{ + g_autoptr(virPCIDeviceList) pcidevs = NULL; + + if (!nhostdevs) + return 0; + + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) + return -1; + + return virHostdevPreparePCIDevicesImpl(mgr, drv_name, dom_name, uuid, + pcidevs, hostdevs, nhostdevs, flags); +} + + /* @oldStateDir: * For upgrade purpose: see virHostdevRestoreNetConfig */ -- 2.23.0

In near future we will have a list of PCI devices we want to re-attach to the host (held in virPCIDeviceListPtr) but we don't have virDomainHostdevDefPtr. That's okay because virHostdevReAttachPCIDevices() works with virPCIDeviceListPtr mostly anyway. And in very few places where it needs virDomainHostdevDefPtr are not interesting for our case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ACKed-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhostdev.c | 58 +++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index b8e6e2bc36..94a0185597 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1002,30 +1002,17 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } -/* @oldStateDir: - * For upgrade purpose: see virHostdevRestoreNetConfig - */ -void -virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, - const char *drv_name, - const char *dom_name, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs, - const char *oldStateDir) +static void +virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + virPCIDeviceListPtr pcidevs, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + const char *oldStateDir) { - g_autoptr(virPCIDeviceList) pcidevs = NULL; size_t i; - if (!nhostdevs) - return; - - if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { - VIR_ERROR(_("Failed to allocate PCI device list: %s"), - virGetLastErrorMessage()); - virResetLastError(); - return; - } - virObjectLock(mgr->activePCIHostdevs); virObjectLock(mgr->inactivePCIHostdevs); @@ -1121,6 +1108,35 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, virObjectUnlock(mgr->inactivePCIHostdevs); } + +/* @oldStateDir: + * For upgrade purpose: see virHostdevRestoreNetConfig + */ +void +virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + const char *oldStateDir) +{ + g_autoptr(virPCIDeviceList) pcidevs = NULL; + + if (!nhostdevs) + return; + + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { + VIR_ERROR(_("Failed to allocate PCI device list: %s"), + virGetLastErrorMessage()); + virResetLastError(); + return; + } + + virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs, + hostdevs, nhostdevs, oldStateDir); +} + + int virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, virDomainHostdevDefPtr *hostdevs, -- 2.23.0

This helper is cleaner than plain memcpy() because one doesn't have to look into virPCIDeviceAddress struct to see if it contains any strings / pointers. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ACKed-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virpci.c | 14 ++++++++++++++ src/util/virpci.h | 4 ++++ 3 files changed, 19 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0988f36c20..6e4ef0413b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2689,6 +2689,7 @@ virObjectUnref; # util/virpci.h virPCIDeviceAddressAsString; +virPCIDeviceAddressCopy; virPCIDeviceAddressEqual; virPCIDeviceAddressFree; virPCIDeviceAddressGetIOMMUGroupAddresses; diff --git a/src/util/virpci.c b/src/util/virpci.c index b0a4107551..99a8002743 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1340,6 +1340,20 @@ virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, return false; } +/** + * virPCIDeviceAddressCopy: + * @dst: where to store address + * @src: source address to copy + * + * Creates a deep copy of given @src address and stores it into + * @dst which has to be pre-allocated by caller. + */ +void virPCIDeviceAddressCopy(virPCIDeviceAddressPtr dst, + const virPCIDeviceAddress *src) +{ + memcpy(dst, src, sizeof(*src)); +} + char * virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 1c94dc307c..f6796fc422 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -44,6 +44,7 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; struct _virZPCIDeviceAddress { unsigned int uid; /* exempt from syntax-check */ unsigned int fid; + /* Don't forget to update virPCIDeviceAddressCopy if needed. */ }; #define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d" @@ -56,6 +57,7 @@ struct _virPCIDeviceAddress { int multi; /* virTristateSwitch */ int extFlags; /* enum virPCIDeviceAddressExtensionFlags */ virZPCIDeviceAddress zpci; + /* Don't forget to update virPCIDeviceAddressCopy if needed. */ }; typedef enum { @@ -236,6 +238,8 @@ bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); bool virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, const virPCIDeviceAddress *addr2); +void virPCIDeviceAddressCopy(virPCIDeviceAddressPtr dst, + const virPCIDeviceAddress *src); char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) ATTRIBUTE_NONNULL(1); -- 2.23.0

There are going to be more disk types that are considered unsafe with respect to migration. Therefore, move the error reporting call outside of if() body and rework if-else combo to switch(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c38bf342d7..e4760c6ad3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1228,6 +1228,8 @@ qemuMigrationSrcIsSafe(virDomainDefPtr def, for (i = 0; i < def->ndisks; i++) { virDomainDiskDefPtr disk = def->disks[i]; const char *src = virDomainDiskGetSource(disk); + int actualType = virStorageSourceGetActualType(disk->src); + bool unsafe = false; /* Disks without any source (i.e. floppies and CD-ROMs) * OR readonly are safe. */ @@ -1241,21 +1243,34 @@ qemuMigrationSrcIsSafe(virDomainDefPtr def, continue; /* However, disks on local FS (e.g. ext4) are not safe. */ - if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_FILE) { + switch ((virStorageType) actualType) { + case VIR_STORAGE_TYPE_FILE: if ((rc = virFileIsSharedFS(src)) < 0) { return false; } else if (rc == 0) { - virReportError(VIR_ERR_MIGRATE_UNSAFE, "%s", - _("Migration without shared storage is unsafe")); - return false; + unsafe = true; } if ((rc = virStorageFileIsClusterFS(src)) < 0) return false; else if (rc == 1) continue; - } else if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_NETWORK) { + break; + case VIR_STORAGE_TYPE_NETWORK: /* But network disks are safe again. */ continue; + + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_LAST: + break; + } + + if (unsafe) { + virReportError(VIR_ERR_MIGRATE_UNSAFE, "%s", + _("Migration without shared storage is unsafe")); + return false; } /* Our code elsewhere guarantees shared disks are either readonly (in -- 2.23.0

There is this class of PCI devices that act like disks: NVMe. Therefore, they are both PCI devices and disks. While we already have <hostdev/> (and can assign a NVMe device to a domain successfully) we don't have disk representation. There are three problems with PCI assignment in case of a NVMe device: 1) domains with <hostdev/> can't be migrated 2) NVMe device is assigned whole, there's no way to assign only a namespace 3) Because hypervisors see <hostdev/> they don't put block layer on top of it - users don't get all the fancy features like snapshots NVMe namespaces are way of splitting one continuous NVDIMM memory into smaller ones, effectively creating smaller NVMe-s (which can then be partitioned, LVMed, etc.) Because of all of this the following XML was chosen to model a NVMe device: <disk type='nvme' device='disk'> <driver name='qemu' type='raw'/> <source type='pci' managed='yes' namespace='1'> <address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </source> <target dev='vda' bus='virtio'/> </disk> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 57 +++++++++++++++++++++++-- docs/schemas/domaincommon.rng | 32 ++++++++++++++ tests/qemuxml2argvdata/disk-nvme.xml | 63 ++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-nvme.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6df4a8b26e..fe871d933f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2944,6 +2944,13 @@ </backingStore> <target dev='vdd' bus='virtio'/> </disk> + <disk type='nvme' device='disk'> + <driver name='qemu' type='raw'/> + <source type='pci' managed='yes' namespace='1'> + <address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </source> + <target dev='vde' bus='virtio'/> + </disk> </devices> ...</pre> @@ -2957,7 +2964,8 @@ Valid values are "file", "block", "dir" (<span class="since">since 0.7.5</span>), "network" (<span class="since">since 0.8.7</span>), or - "volume" (<span class="since">since 1.0.5</span>) + "volume" (<span class="since">since 1.0.5</span>), or + "nvme" (<span class="since">since 5.6.0</span>) and refer to the underlying source for the disk. <span class="since">Since 0.0.3</span> </dd> @@ -3140,6 +3148,43 @@ <span class="since">Since 1.0.5</span> </p> </dd> + <dt><code>nvme</code></dt> + <dd> + To specify disk source for NVMe disk the <code>source</code> + element has the following attributes: + <dl> + <dt><code>type</code></dt> + <dd>The type of address specified in <code>address</code> + sub-element. Currently, only <code>pci</code> value is + accepted. + </dd> + + <dt><code>managed</code></dt> + <dd>This attribute instructs libvirt to detach NVMe + controller automatically on domain startup (<code>yes</code>) + or expect the controller to be detached by system + administrator (<code>no</code>). + </dd> + + <dt><code>namespace</code></dt> + <dd>The namespace ID which should be assigned to the domain. + According to NVMe standard, namespace numbers start from 1, + including. + </dd> + </dl> + + The difference between <code><disk type='nvme'></code> + and <code><hostdev/></code> is that the latter is plain + host device assignment with all its limitations (e.g. no live + migration), while the former makes hypervisor to run the NVMe + disk through hypervisor's block layer thus enabling all + features provided by the layer (e.g. snapshots, domain + migration, etc.). Moreover, since the NVMe disk is unbinded + from its PCI driver, the host kernel storage stack is not + involved (compared to passing say <code>/dev/nvme0n1</code> via + <code><disk type='block'></code> and therefore lower + latencies can be achieved. + </dd> </dl> With "file", "block", and "volume", one or more optional sub-elements <code>seclabel</code>, <a href="#seclabel">described @@ -3302,11 +3347,17 @@ initiator IQN needed to access the source via mandatory attribute <code>name</code>. </dd> + <dt><code>address</code></dt> + <dd>For disk of type <code>nvme</code> this element + specifies the PCI address of the host NVMe + controller. + <span class="since">Since 5.6.0</span> + </dd> </dl> <p> - For a "file" or "volume" disk type which represents a cdrom or floppy - (the <code>device</code> attribute), it is possible to define + For a "file" or "volume" disk type which represents a cdrom or + floppy (the <code>device</code> attribute), it is possible to define policy what to do with the disk if the source file is not accessible. (NB, <code>startupPolicy</code> is not valid for "volume" disk unless the specified storage volume is of "file" type). This is done by the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a83c9ae7a5..4b0638a836 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1603,6 +1603,7 @@ <ref name="diskSourceDir"/> <ref name="diskSourceNetwork"/> <ref name="diskSourceVolume"/> + <ref name="diskSourceNvme"/> </choice> </define> @@ -1918,6 +1919,37 @@ </optional> </define> + <define name="diskSourceNvme"> + <attribute name="type"> + <value>nvme</value> + </attribute> + <optional> + <element name="source"> + <attribute name="type"> + <value>pci</value> + </attribute> + <attribute name="namespace"> + <ref name="uint32"/> + </attribute> + <optional> + <attribute name="managed"> + <ref name="virYesNo"/> + </attribute> + </optional> + <element name="address"> + <ref name="pciaddress"/> + </element> + <ref name="diskSourceCommon"/> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name="encryption"/> + </optional> + </element> + </optional> + </define> + <define name="diskTarget"> <data type="string"> <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> diff --git a/tests/qemuxml2argvdata/disk-nvme.xml b/tests/qemuxml2argvdata/disk-nvme.xml new file mode 100644 index 0000000000..66b2d8450e --- /dev/null +++ b/tests/qemuxml2argvdata/disk-nvme.xml @@ -0,0 +1,63 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='nvme' device='disk'> + <driver name='qemu' type='raw'/> + <source type='pci' managed='yes' namespace='1'> + <address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='nvme' device='disk'> + <driver name='qemu' type='raw'/> + <source type='pci' managed='yes' namespace='2'> + <address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </source> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='nvme' device='disk'> + <driver name='qemu' type='raw'/> + <source type='pci' managed='no' namespace='1'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </source> + <target dev='vdc' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </disk> + <disk type='nvme' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source type='pci' managed='no' namespace='2'> + <address domain='0x0001' bus='0x02' slot='0x00' function='0x0'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vdd' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> -- 2.23.0

To simplify implementation, some restrictions are added. For instance, an NVMe disk can't go to any bus but virtio and has to be type of 'disk' and can't have startupPolicy set. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 95 ++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libxl/xen_xl.c | 1 + src/qemu/qemu_block.c | 2 + src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 4 ++ src/qemu/qemu_migration.c | 5 ++ src/util/virstoragefile.c | 57 ++++++++++++++++ src/util/virstoragefile.h | 17 +++++ tests/qemuxml2xmloutdata/disk-nvme.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 185 insertions(+) create mode 120000 tests/qemuxml2xmloutdata/disk-nvme.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9580884747..58879a66ba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5135,6 +5135,11 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, return -1; } + if (disk->src->type == VIR_STORAGE_TYPE_NVME) { + if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT) + disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES; + } + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) { return -1; @@ -9271,6 +9276,75 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, } +static int +virDomainDiskSourceNVMeParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStorageSourcePtr src) +{ + g_autoptr(virStorageSourceNVMeDef) nvme = NULL; + g_autofree char *type = NULL; + g_autofree char *namespace = NULL; + g_autofree char *managed = NULL; + xmlNodePtr address; + + nvme = g_new0(virStorageSourceNVMeDef, 1); + + if (!(type = virXMLPropString(node, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing 'type' attribute to disk source")); + return -1; + } + + if (STRNEQ(type, "pci")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported source type '%s'"), + type); + return -1; + } + + if (!(namespace = virXMLPropString(node, "namespace"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing 'namespace' attribute to disk source")); + return -1; + } + + if (virStrToLong_ull(namespace, NULL, 10, &nvme->namespace) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("malformed namespace '%s'"), + namespace); + return -1; + } + + /* NVMe namespaces start from 1 */ + if (nvme->namespace == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVMe namespace can't be zero")); + return -1; + } + + if ((managed = virXMLPropString(node, "managed"))) { + if ((nvme->managed = virTristateBoolTypeFromString(managed)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("malformed managed value '%s'"), + managed); + return -1; + } + } + + if (!(address = virXPathNode("./address", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVMe disk source is missing address")); + return -1; + } + + if (virPCIDeviceAddressParseXML(address, &nvme->pciAddr) < 0) + return -1; + + src->nvme = g_steal_pointer(&nvme); + return 0; +} + + static int virDomainDiskSourcePRParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -9369,6 +9443,10 @@ virDomainStorageSourceParse(xmlNodePtr node, if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) return -1; break; + case VIR_STORAGE_TYPE_NVME: + if (virDomainDiskSourceNVMeParse(node, ctxt, src) < 0) + return -1; + break; case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -24102,6 +24180,19 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf, } +static void +virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf, + virBufferPtr childBuf, + const virStorageSourceNVMeDef *nvme) +{ + virBufferAddLit(attrBuf, " type='pci'"); + virBufferAsprintf(attrBuf, " managed='%s'", + virTristateBoolTypeToString(nvme->managed)); + virBufferAsprintf(attrBuf, " namespace='%llu'", nvme->namespace); + virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false); +} + + static int virDomainDiskSourceFormatPrivateData(virBufferPtr buf, virStorageSourcePtr src, @@ -24178,6 +24269,10 @@ virDomainDiskSourceFormat(virBufferPtr buf, break; + case VIR_STORAGE_TYPE_NVME: + virDomainDiskSourceNVMeFormat(&attrBuf, &childBuf, src->nvme); + break; + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6e4ef0413b..d37d3867a6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3078,6 +3078,7 @@ virStorageSourceNetworkAssignDefaultPorts; virStorageSourceNew; virStorageSourceNewFromBacking; virStorageSourceNewFromBackingAbsolute; +virStorageSourceNVMeDefFree; virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 396adf6dac..8e9770c367 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1649,6 +1649,7 @@ xenFormatXLDiskSrc(virStorageSourcePtr src, char **srcstr) break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: break; diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index ada2c52947..8eeb9c3c57 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1060,6 +1060,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return NULL; @@ -2331,6 +2332,7 @@ qemuBlockStorageSourceCreateGetStorageProps(virStorageSourcePtr src, case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: return 0; case VIR_STORAGE_TYPE_NONE: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3465d28b84..c61e38492a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1129,6 +1129,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src, break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: break; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b5300241a8..72ee89db1f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14776,6 +14776,7 @@ qemuDomainSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDefPtr snapdi case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -14792,6 +14793,7 @@ qemuDomainSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDefPtr snapdi case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -14859,6 +14861,7 @@ qemuDomainSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDefPtr snapdisk case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -14986,6 +14989,7 @@ qemuDomainSnapshotPrepareDiskInternal(virDomainDiskDefPtr disk, case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e4760c6ad3..3705b263a6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -227,6 +227,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn, case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1259,6 +1260,10 @@ qemuMigrationSrcIsSafe(virDomainDefPtr def, /* But network disks are safe again. */ continue; + case VIR_STORAGE_TYPE_NVME: + unsafe = true; + break; + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 77f885ef1d..b4ec9624be 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -57,6 +57,7 @@ VIR_ENUM_IMPL(virStorage, "dir", "network", "volume", + "nvme", ); VIR_ENUM_IMPL(virStorageFileFormat, @@ -2094,6 +2095,49 @@ virStoragePRDefCopy(virStoragePRDefPtr src) } +static virStorageSourceNVMeDefPtr +virStorageSourceNVMeDefCopy(const virStorageSourceNVMeDef *src) +{ + virStorageSourceNVMeDefPtr ret = NULL; + + ret = g_new0(virStorageSourceNVMeDef, 1); + + ret->namespace = src->namespace; + ret->managed = src->managed; + virPCIDeviceAddressCopy(&ret->pciAddr, &src->pciAddr); + return ret; +} + + +static bool +virStorageSourceNVMeDefIsEqual(const virStorageSourceNVMeDef *a, + const virStorageSourceNVMeDef *b) +{ + if (!a && !b) + return true; + + if (!a || !b) + return false; + + if (a->namespace != b->namespace || + a->managed != b->managed || + !virPCIDeviceAddressEqual(&a->pciAddr, &b->pciAddr)) + return false; + + return true; +} + + +void +virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def); +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) @@ -2292,6 +2336,9 @@ virStorageSourceCopy(const virStorageSource *src, !(def->pr = virStoragePRDefCopy(src->pr))) return NULL; + if (src->nvme) + def->nvme = virStorageSourceNVMeDefCopy(src->nvme); + if (virStorageSourceInitiatorCopy(&def->initiator, &src->initiator) < 0) return NULL; @@ -2350,6 +2397,10 @@ virStorageSourceIsSameLocation(virStorageSourcePtr a, } } + if (a->type == VIR_STORAGE_TYPE_NVME && + !virStorageSourceNVMeDefIsEqual(a->nvme, b->nvme)) + return false; + return true; } @@ -2432,6 +2483,9 @@ virStorageSourceIsLocalStorage(const virStorageSource *src) case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_VOLUME: + /* While NVMe disks are local, they are not accessible via src->path. + * Therefore, we have to return false here. */ + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_LAST: case VIR_STORAGE_TYPE_NONE: return false; @@ -2517,6 +2571,7 @@ virStorageSourceClear(virStorageSourcePtr def) VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); virStoragePRDefFree(def->pr); + virStorageSourceNVMeDefFree(def->nvme); virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); @@ -3773,6 +3828,7 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, /* We shouldn't get VOLUME, but the switch requires all cases */ case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return -1; @@ -4229,6 +4285,7 @@ virStorageSourceIsRelative(virStorageSourcePtr src) case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return false; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c357803e50..42ca49bc64 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -30,6 +30,7 @@ #include "virutil.h" #include "virsecret.h" #include "virenum.h" +#include "virpci.h" /* Minimum header size required to probe all known formats with * virStorageFileProbeFormat, or obtain metadata from a known format. @@ -51,6 +52,7 @@ typedef enum { VIR_STORAGE_TYPE_DIR, VIR_STORAGE_TYPE_NETWORK, VIR_STORAGE_TYPE_VOLUME, + VIR_STORAGE_TYPE_NVME, VIR_STORAGE_TYPE_LAST } virStorageType; @@ -230,6 +232,16 @@ struct _virStorageSourceInitiatorDef { char *iqn; /* Initiator IQN */ }; +typedef struct _virStorageSourceNVMeDef virStorageSourceNVMeDef; +typedef virStorageSourceNVMeDef *virStorageSourceNVMeDefPtr; +struct _virStorageSourceNVMeDef { + unsigned long long namespace; + int managed; /* enum virTristateBool */ + virPCIDeviceAddress pciAddr; + + /* Don't forget to update virStorageSourceNVMeDefCopy */ +}; + typedef struct _virStorageDriverData virStorageDriverData; typedef virStorageDriverData *virStorageDriverDataPtr; @@ -261,6 +273,8 @@ struct _virStorageSource { bool encryptionInherited; virStoragePRDefPtr pr; + virStorageSourceNVMeDefPtr nvme; /* type == VIR_STORAGE_TYPE_NVME */ + virStorageSourceInitiatorDef initiator; virObjectPtr privateData; @@ -416,6 +430,9 @@ bool virStoragePRDefIsManaged(virStoragePRDefPtr prd); bool virStorageSourceChainHasManagedPR(virStorageSourcePtr src); +void virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSourceNVMeDef, virStorageSourceNVMeDefFree); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); diff --git a/tests/qemuxml2xmloutdata/disk-nvme.xml b/tests/qemuxml2xmloutdata/disk-nvme.xml new file mode 120000 index 0000000000..ea9eb267ac --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-nvme.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/disk-nvme.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8b43f35f06..b11f086016 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -329,6 +329,7 @@ mymain(void) DO_TEST("disk-network-sheepdog", NONE); DO_TEST("disk-network-vxhs", NONE); DO_TEST("disk-network-tlsx509", NONE); + DO_TEST("disk-nvme", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_QCOW2_LUKS); DO_TEST("disk-scsi", QEMU_CAPS_SCSI_LSI, QEMU_CAPS_SCSI_MEGASAS, QEMU_CAPS_SCSI_MPTSAS1068, QEMU_CAPS_SCSI_DISK_WWN); DO_TEST("disk-virtio-scsi-reservations", -- 2.23.0

There are couple of places where a domain with a VFIO device gets special treatment: in CGroups when enabling/disabling access to /dev/vfio/vfio, and when creating/removing nodes in domain mount namespace. Well, a NVMe disk is a VFIO device too. Fortunately, we have this qemuDomainNeedsVFIO() function which is the only place that needs adjustment. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 70c4ee8e65..e81d986e9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12865,7 +12865,8 @@ bool qemuDomainNeedsVFIO(const virDomainDef *def) { return virDomainDefHasVFIOHostdev(def) || - virDomainDefHasMdevHostdev(def); + virDomainDefHasMdevHostdev(def) || + virDomainDefHasNVMeDisk(def); } -- 2.23.0

If a domain has an NVMe disk configured, then we need to allow it on devices CGroup so that qemu can access it. There is one caveat though - if an NVMe disk is read only we need CGroup to allow write too. This is because when opening the device, qemu does couple of ioctl()-s which are considered as write. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 97 ++++++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d23318db8c..1d3994ecc9 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -118,10 +118,29 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, virStorageSourcePtr src, bool forceReadonly) { - if (!src->path || !virStorageSourceIsLocalStorage(src)) { - VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", - NULLSTR(src->path), virStorageTypeToString(src->type)); - return 0; + g_autofree char *path = NULL; + bool readonly = src->readonly || forceReadonly; + + if (src->type == VIR_STORAGE_TYPE_NVME) { + /* Even though disk is R/O we can't make it so in + * CGroups. QEMU will try to do some ioctl()-s over the + * device and such operations are considered R/W by the + * kernel */ + readonly = false; + + if (!(path = virPCIDeviceAddressGetIOMMUGroupDev(&src->nvme->pciAddr))) + return -1; + + if (qemuSetupImagePathCgroup(vm, QEMU_DEV_VFIO, false) < 0) + return -1; + } else { + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + path = g_strdup(src->path); } if (virStoragePRDefIsManaged(src->pr) && @@ -129,7 +148,7 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, qemuSetupImagePathCgroup(vm, QEMU_DEVICE_MAPPER_CONTROL_PATH, false) < 0) return -1; - return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); + return qemuSetupImagePathCgroup(vm, path, readonly); } @@ -146,7 +165,10 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; + g_autofree char *path = NULL; int perms = VIR_CGROUP_DEVICE_RWM; + bool hasPR = false; + bool hasNVMe = false; size_t i; int ret; @@ -154,41 +176,60 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - if (!src->path || !virStorageSourceIsLocalStorage(src)) { - VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", - NULLSTR(src->path), virStorageTypeToString(src->type)); - return 0; - } + for (i = 0; i < vm->def->ndisks; i++) { + virStorageSourcePtr diskSrc = vm->def->disks[i]->src; - if (virFileExists(QEMU_DEVICE_MAPPER_CONTROL_PATH)) { - for (i = 0; i < vm->def->ndisks; i++) { - virStorageSourcePtr diskSrc = vm->def->disks[i]->src; + if (src == diskSrc) + continue; - if (src == diskSrc) - continue; + if (virStoragePRDefIsManaged(diskSrc->pr)) + hasPR = true; - if (virStoragePRDefIsManaged(diskSrc->pr)) - break; - } + if (virStorageSourceChainHasNVMe(diskSrc)) + hasNVMe = true; + } - if (i == vm->def->ndisks) { - VIR_DEBUG("Disabling device mapper control"); - ret = virCgroupDenyDevicePath(priv->cgroup, - QEMU_DEVICE_MAPPER_CONTROL_PATH, - perms, true); + if (src->type == VIR_STORAGE_TYPE_NVME) { + if (!(path = virPCIDeviceAddressGetIOMMUGroupDev(&src->nvme->pciAddr))) + return -1; + + if (!hasNVMe && + !qemuDomainNeedsVFIO(vm->def)) { + ret = virCgroupDenyDevicePath(priv->cgroup, QEMU_DEV_VFIO, perms, true); virDomainAuditCgroupPath(vm, priv->cgroup, "deny", - QEMU_DEVICE_MAPPER_CONTROL_PATH, + QEMU_DEV_VFIO, virCgroupGetDevicePermsString(perms), ret); if (ret < 0) - return ret; + return -1; } + } else { + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + path = g_strdup(src->path); + } + + if (!hasPR && + virFileExists(QEMU_DEVICE_MAPPER_CONTROL_PATH)) { + VIR_DEBUG("Disabling device mapper control"); + ret = virCgroupDenyDevicePath(priv->cgroup, + QEMU_DEVICE_MAPPER_CONTROL_PATH, + perms, true); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + QEMU_DEVICE_MAPPER_CONTROL_PATH, + virCgroupGetDevicePermsString(perms), ret); + if (ret < 0) + return ret; } - VIR_DEBUG("Deny path %s", src->path); + VIR_DEBUG("Deny path %s", path); - ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); + ret = virCgroupDenyDevicePath(priv->cgroup, path, perms, true); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, virCgroupGetDevicePermsString(perms), ret); /* If you're looking for a counter part to -- 2.23.0

This function is currently not called for any type of storage source that is not considered 'local' (as defined by virStorageSourceIsLocalStorage()). Well, NVMe disks are not 'local' from that point of view and therefore we will need to call this function more frequently. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_apparmor.c | 25 ++++++++++++---- src/security/security_dac.c | 30 +++++++++++++++++++ src/security/security_selinux.c | 51 +++++++++++++++++++++++++++----- 3 files changed, 92 insertions(+), 14 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 21560b2330..7cea788931 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -779,9 +779,8 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { virSecurityLabelDefPtr secdef; - - if (!src->path || !virStorageSourceIsLocalStorage(src)) - return 0; + g_autofree char *vfioGroupDev = NULL; + const char *path; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); if (!secdef || !secdef->relabel) @@ -790,15 +789,29 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!secdef->imagelabel) return 0; + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr))) + return -1; + + path = vfioGroupDev; + } else { + if (!src->path || !virStorageSourceIsLocalStorage(src)) + return 0; + + path = src->path; + } + /* if the device doesn't exist, error out */ - if (!virFileExists(src->path)) { + if (!virFileExists(path)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("\'%s\' does not exist"), - src->path); + path); return -1; } - return reload_profile(mgr, def, src->path, true); + return reload_profile(mgr, def, path, true); } static int diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a9a1fad5d7..411aa1b159 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -911,6 +911,19 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + g_autofree char *vfioGroupDev = NULL; + + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr))) + return -1; + + return virSecurityDACSetOwnership(mgr, NULL, vfioGroupDev, user, group, false); + } + /* We can't do restore on shared resources safely. Not even * with refcounting implemented in XATTRs because if there * was a domain running with the feature turned off the @@ -1017,6 +1030,23 @@ virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr, } } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + g_autofree char *vfioGroupDev = NULL; + + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr))) + return -1; + + /* Ideally, we would check if there is not another PCI + * device within domain def that is in the same IOMMU + * group. But we're not doing that for hostdevs yet. */ + + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, vfioGroupDev, false); + } + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e05ef7593e..26bbe55ce7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1729,9 +1729,8 @@ virSecuritySELinuxRestoreImageLabelSingle(virSecurityManagerPtr mgr, { virSecurityLabelDefPtr seclabel; virSecurityDeviceLabelDefPtr disk_seclabel; - - if (!src->path || !virStorageSourceIsLocalStorage(src)) - return 0; + g_autofree char *vfioGroupDev = NULL; + const char *path = src->path; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) @@ -1763,9 +1762,16 @@ virSecuritySELinuxRestoreImageLabelSingle(virSecurityManagerPtr mgr, * ownership, because that kills access on the destination host which is * sub-optimal for the guest VM's I/O attempts :-) */ if (migrated) { - int rc = virFileIsSharedFS(src->path); - if (rc < 0) - return -1; + int rc = 1; + + if (virStorageSourceIsLocalStorage(src)) { + if (!src->path) + return 0; + + if ((rc = virFileIsSharedFS(src->path)) < 0) + return -1; + } + if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", src->path); @@ -1773,7 +1779,22 @@ virSecuritySELinuxRestoreImageLabelSingle(virSecurityManagerPtr mgr, } } - return virSecuritySELinuxRestoreFileLabel(mgr, src->path, true); + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr))) + return -1; + + /* Ideally, we would check if there is not another PCI + * device within domain def that is in the same IOMMU + * group. But we're not doing that for hostdevs yet. */ + path = vfioGroupDev; + } + + return virSecuritySELinuxRestoreFileLabel(mgr, path, true); } @@ -1820,6 +1841,8 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, char *use_label = NULL; bool remember; bool is_toplevel = parent == src || parent->externalDataStore == src; + g_autofree char *vfioGroupDev = NULL; + const char *path = src->path; int ret; if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -1872,7 +1895,19 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, use_label = data->content_context; } - ret = virSecuritySELinuxSetFilecon(mgr, src->path, use_label, remember); + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr))) + return -1; + + path = vfioGroupDev; + } + + ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember); if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us -- 2.23.0

This capability tracks if qemu is capable of: -drive file.driver=nvme The feature was added in QEMU's commit of v2.12.0-rc0~104^2~2. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + 24 files changed, 25 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f65af5c228..4b3b4f2eff 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -552,6 +552,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "arm-max-cpu", "blockdev-file-dynamic-auto-read-only", "savevm-monitor-nodes", + "drive-nvme", ); @@ -1399,6 +1400,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "blockdev-add/arg-type/+file/drop-cache", QEMU_CAPS_MIGRATION_FILE_DROP_CACHE }, { "blockdev-add/arg-type/+file/$dynamic-auto-read-only", QEMU_CAPS_BLOCK_FILE_AUTO_READONLY_DYNAMIC }, { "human-monitor-command/$savevm-monitor-nodes", QEMU_CAPS_SAVEVM_MONITOR_NODES }, + { "blockdev-add/arg-type/+nvme", QEMU_CAPS_DRIVE_NVME }, }; typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3fd8bebe79..3a9b70c1cf 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -533,6 +533,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_ARM_MAX_CPU, /* max-arm-cpu type exists */ QEMU_CAPS_BLOCK_FILE_AUTO_READONLY_DYNAMIC, /* the auto-read-only property of block backends for files is dynamic */ QEMU_CAPS_SAVEVM_MONITOR_NODES, /* 'savevm' handles monitor-owned nodes properly */ + QEMU_CAPS_DRIVE_NVME, /* -drive file.driver=nvme */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index 5d1bfb798f..72c63533a3 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -154,6 +154,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='arm-max-cpu'/> + <flag name='drive-nvme'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700289</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 84179e8ef6..147c017fe3 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -151,6 +151,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900289</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 4be4018d44..8ff0633724 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -123,6 +123,7 @@ <flag name='memory-backend-file.align'/> <flag name='query-cpu-model-baseline'/> <flag name='query-cpu-model-comparison'/> + <flag name='drive-nvme'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100289</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 81903cd493..85954d352e 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -195,6 +195,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='x86-max-cpu'/> + <flag name='drive-nvme'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100289</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml index 8714c3c1f4..baea300753 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml @@ -153,6 +153,7 @@ <flag name='memory-backend-file.align'/> <flag name='bochs-display'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>2012050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900239</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml index a31fc2a250..3b81ea11c3 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml @@ -93,6 +93,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml index 6bf0e68725..615667d9b2 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml @@ -93,6 +93,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml index 941369003b..34a4999294 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml @@ -126,6 +126,7 @@ <flag name='query-cpu-model-baseline'/> <flag name='query-cpu-model-comparison'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100239</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index 92abf01e4e..3d5535f404 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -200,6 +200,7 @@ <flag name='x86-max-cpu'/> <flag name='bochs-display'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100239</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml index 5667860b44..0f64cfb2ba 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml @@ -157,6 +157,7 @@ <flag name='memory-backend-file.pmem'/> <flag name='overcommit'/> <flag name='bochs-display'/> + <flag name='drive-nvme'/> <version>3000091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml index 5cdba54a11..c468ad0153 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml @@ -203,6 +203,7 @@ <flag name='x86-max-cpu'/> <flag name='bochs-display'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>3000092</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml index dacbadd208..9fc3c7e6f5 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml @@ -167,6 +167,7 @@ <flag name='migration-file-drop-cache'/> <flag name='ramfb'/> <flag name='arm-max-cpu'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml index cd860daa9f..e84dd557a4 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml @@ -171,6 +171,7 @@ <flag name='bochs-display'/> <flag name='migration-file-drop-cache'/> <flag name='machine.pseries.cap-ccf-assist'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml index a1a1a26ece..bc675fc727 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml @@ -168,6 +168,7 @@ <flag name='nbd-bitmap'/> <flag name='bochs-display'/> <flag name='migration-file-drop-cache'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml index 5bdc1e9451..1eeda32b7c 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml @@ -168,6 +168,7 @@ <flag name='nbd-bitmap'/> <flag name='bochs-display'/> <flag name='migration-file-drop-cache'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml index 54f7aba2dc..8b8acd5a33 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml @@ -134,6 +134,7 @@ <flag name='migration-file-drop-cache'/> <flag name='query-cpu-model-baseline'/> <flag name='query-cpu-model-comparison'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index 90f8452710..c29be71658 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -208,6 +208,7 @@ <flag name='bochs-display'/> <flag name='migration-file-drop-cache'/> <flag name='ramfb'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index 9f1675aa55..2bc9672063 100644 --- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml @@ -213,6 +213,7 @@ <flag name='vhost-user-vga'/> <flag name='ramfb'/> <flag name='blockdev-file-dynamic-auto-read-only'/> + <flag name='drive-nvme'/> <version>4001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml index 6ed4829be6..588e682064 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml @@ -173,6 +173,7 @@ <flag name='arm-max-cpu'/> <flag name='blockdev-file-dynamic-auto-read-only'/> <flag name='savevm-monitor-nodes'/> + <flag name='drive-nvme'/> <version>4001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml index bc7042e7e1..9a480c4eb3 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml @@ -174,6 +174,7 @@ <flag name='vhost-user-vga'/> <flag name='machine.pseries.cap-ccf-assist'/> <flag name='blockdev-file-dynamic-auto-read-only'/> + <flag name='drive-nvme'/> <version>4001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml index 714634dcc3..505b3adcb6 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml @@ -133,6 +133,7 @@ <flag name='query-cpu-model-baseline'/> <flag name='query-cpu-model-comparison'/> <flag name='blockdev-file-dynamic-auto-read-only'/> + <flag name='drive-nvme'/> <version>4001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml index c7bac03f44..31302c9a7b 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml @@ -216,6 +216,7 @@ <flag name='ramfb'/> <flag name='blockdev-file-dynamic-auto-read-only'/> <flag name='savevm-monitor-nodes'/> + <flag name='drive-nvme'/> <version>4001092</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> -- 2.23.0

Now, that we have everything prepared, we can generate command line for NVMe disks. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_block.c | 25 +++++++- src/qemu/qemu_command.c | 3 + src/qemu/qemu_process.c | 7 +++ .../disk-nvme.x86_64-latest.args | 63 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 8eeb9c3c57..1ff5a7f226 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -977,6 +977,25 @@ qemuBlockStorageSourceGetVvfatProps(virStorageSourcePtr src, } +static virJSONValuePtr +qemuBlockStorageSourceGetNVMeProps(virStorageSourcePtr src) +{ + const virStorageSourceNVMeDef *nvme = src->nvme; + g_autofree char *pciAddr = NULL; + virJSONValuePtr ret = NULL; + + if (!(pciAddr = virPCIDeviceAddressAsString(&nvme->pciAddr))) + return NULL; + + ignore_value(virJSONValueObjectCreate(&ret, + "s:driver", "nvme", + "s:device", pciAddr, + "U:namespace", nvme->namespace, + NULL)); + return ret; +} + + static int qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src, virJSONValuePtr props) @@ -1059,8 +1078,12 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, return NULL; break; - case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NVME: + if (!(fileprops = qemuBlockStorageSourceGetNVMeProps(src))) + return NULL; + break; + + case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return NULL; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c61e38492a..97d11c1d5f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1527,6 +1527,9 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src, src->haveTLS == VIR_TRISTATE_BOOL_YES) return true; + if (actualType == VIR_STORAGE_TYPE_NVME) + return true; + return false; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a588ee25f8..fa507691ff 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5284,6 +5284,13 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, _("PowerPC pseries machines do not support floppy device")); return -1; } + + if (src->type == VIR_STORAGE_TYPE_NVME && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_NVME)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("NVMe disks are not supported with this QEMU binary")); + return -1; + } } return 0; diff --git a/tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args b/tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args new file mode 100644 index 0000000000..b4486086fa --- /dev/null +++ b/tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args @@ -0,0 +1,63 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-blockdev '{"driver":"nvme","device":"0000:01:00.0","namespace":1,\ +"node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-4-format","read-only":false,"driver":"raw",\ +"file":"libvirt-4-storage"}' \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=libvirt-4-format,\ +id=virtio-disk0,bootindex=1 \ +-blockdev '{"driver":"nvme","device":"0000:01:00.0","namespace":2,\ +"node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"raw",\ +"file":"libvirt-3-storage"}' \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=libvirt-3-format,\ +id=virtio-disk1 \ +-blockdev '{"driver":"nvme","device":"0000:02:00.0","namespace":1,\ +"node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\ +"file":"libvirt-2-storage"}' \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=libvirt-2-format,\ +id=virtio-disk2 \ +-object secret,id=libvirt-1-format-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-blockdev '{"driver":"nvme","device":"0001:02:00.0","namespace":2,\ +"node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},\ +"auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,\ +"cache":{"direct":true,"no-flush":false},"driver":"qcow2",\ +"encrypt":{"format":"luks","key-secret":"libvirt-1-format-luks-secret0"},\ +"file":"libvirt-1-storage"}' \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=libvirt-1-format,\ +id=virtio-disk3,write-cache=on \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 86b8899921..440f47309f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1078,6 +1078,7 @@ mymain(void) driver.config->vxhsTLS = 0; VIR_FREE(driver.config->vxhsTLSx509certdir); DO_TEST("disk-no-boot", NONE); + DO_TEST_CAPS_LATEST("disk-nvme"); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", QEMU_CAPS_VIRTIO_SCSI); DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-attaching-partition-nosupport"); -- 2.23.0

Because this is a HMP we're dealing with, there is nothing like class of reply message, so we have to do some string comparison to guess if the command fails. Well, with NVMe disks whole new class of errors comes to play because qemu needs to initialize IOMMU and VFIO for them. You can see all the messages it may produce in qemu_vfio_init_pci(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_text.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 9054682d60..6948a5bf90 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -75,6 +75,13 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon, goto cleanup; } + if (strstr(reply, "IOMMU") || + strstr(reply, "VFIO")) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + reply); + goto cleanup; + } + ret = 0; cleanup: -- 2.23.0

At the very beginning of the attach function the qemuDomainStorageSourceChainAccessAllow() is called which modifies CGroups, locks and seclabels for new disk and its backing chain. This must be followed by a counterpart which reverts back all the changes if something goes wrong. This boils down to calling qemuDomainStorageSourceChainAccessRevoke() which is done under 'error' label. But not all failure branches jump there. They just jump onto 'cleanup' label where no revoke is done. Such mistake is easy to do because 'cleanup' label does exist. Therefore, dissolve 'error' block in 'cleanup' and have everything jump onto 'cleanup' label. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d4eacaf099..053523f8c5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -683,13 +683,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) - goto cleanup; + return -1; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto error; + goto cleanup; if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) - goto error; + goto cleanup; if (blockdev) { if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON && @@ -706,13 +706,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, } if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) - goto error; + goto cleanup; if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) - goto error; + goto cleanup; if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) - goto error; + goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -748,7 +748,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; - goto error; + goto cleanup; } virDomainAuditDisk(vm, NULL, disk->src, "attach", true); @@ -757,6 +757,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = 0; cleanup: + if (ret < 0) + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); qemuDomainSecretDiskDestroy(disk); return ret; @@ -773,9 +775,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = -2; virDomainAuditDisk(vm, NULL, disk->src, "attach", false); - - error: - ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); goto cleanup; } -- 2.23.0

With NVMe disks, one can start a blockjob with a NVMe disk that is not visible in domain XML (at least right away). Usually, it's fairly easy to override this limitation of qemuDomainGetMemLockLimitBytes() - for instance for hostdevs we temporarily add the device to domain def, let the function calculate the limit and then remove the device. But it's not so easy with virStorageSourcePtr - in some cases they don't necessarily are attached to a disk. And even if they are it's done later in the process and frankly, I find it too complicated to be able to use the simple trick we use with hostdevs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 46 ++++++++++++++++++++++++++--------------- src/qemu/qemu_domain.h | 6 ++++-- src/qemu/qemu_hotplug.c | 12 +++++------ tests/qemumemlocktest.c | 2 +- 5 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 97d11c1d5f..7cc6a55c75 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10463,7 +10463,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, /* In some situations, eg. VFIO passthrough, QEMU might need to lock a * significant amount of memory, so we need to set the limit accordingly */ - virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def)); + virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def, false)); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MSG_TIMESTAMP) && cfg->logTimestamp) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e81d986e9e..be4aef049d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11875,12 +11875,14 @@ ppc64VFIODeviceIsNV2Bridge(const char *device) /** * getPPC64MemLockLimitBytes: * @def: domain definition + * @forceVFIO: force VFIO usage * * A PPC64 helper that calculates the memory locking limit in order for * the guest to operate properly. */ static unsigned long long -getPPC64MemLockLimitBytes(virDomainDefPtr def) +getPPC64MemLockLimitBytes(virDomainDefPtr def, + bool forceVFIO) { unsigned long long memKB = 0; unsigned long long baseLimit = 0; @@ -11971,7 +11973,7 @@ getPPC64MemLockLimitBytes(virDomainDefPtr def) passthroughLimit = maxMemory + 128 * (1ULL<<30) / 512 * nPCIHostBridges + 8192; - } else if (usesVFIO) { + } else if (usesVFIO || forceVFIO) { /* For regular (non-NVLink2 present) VFIO passthrough, the value * of passthroughLimit is: * @@ -12009,16 +12011,20 @@ getPPC64MemLockLimitBytes(virDomainDefPtr def) /** * qemuDomainGetMemLockLimitBytes: * @def: domain definition + * @forceVFIO: force VFIO calculation * * Calculate the memory locking limit that needs to be set in order for * the guest to operate properly. The limit depends on a number of factors, * including certain configuration options and less immediately apparent ones * such as the guest architecture or the use of certain devices. + * The @forceVFIO argument can be used to tell this function will use VFIO even + * though @def doesn't indicates so right now. * * Returns: the memory locking limit, or 0 if setting the limit is not needed */ unsigned long long -qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) +qemuDomainGetMemLockLimitBytes(virDomainDefPtr def, + bool forceVFIO) { unsigned long long memKB = 0; bool usesVFIO = false; @@ -12039,7 +12045,7 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) return VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; if (ARCH_IS_PPC64(def->os.arch) && def->virtType == VIR_DOMAIN_VIRT_KVM) - return getPPC64MemLockLimitBytes(def); + return getPPC64MemLockLimitBytes(def, forceVFIO); /* For device passthrough using VFIO the guest memory and MMIO memory * regions need to be locked persistent in order to allow DMA. @@ -12059,18 +12065,20 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) * * Note that this may not be valid for all platforms. */ - for (i = 0; i < def->nhostdevs; i++) { - if (virHostdevIsVFIODevice(def->hostdevs[i]) || - virHostdevIsMdevDevice(def->hostdevs[i])) { - usesVFIO = true; - break; + if (!forceVFIO) { + for (i = 0; i < def->nhostdevs; i++) { + if (virHostdevIsVFIODevice(def->hostdevs[i]) || + virHostdevIsMdevDevice(def->hostdevs[i])) { + usesVFIO = true; + break; + } } - } - if (virDomainDefHasNVMeDisk(def)) - usesVFIO = true; + if (virDomainDefHasNVMeDisk(def)) + usesVFIO = true; + } - if (usesVFIO) + if (usesVFIO || forceVFIO) memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024; done: @@ -12081,9 +12089,12 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) /** * qemuDomainAdjustMaxMemLock: * @vm: domain + * @forceVFIO: apply VFIO requirements even if vm's def doesn't require it * * Adjust the memory locking limit for the QEMU process associated to @vm, in - * order to comply with VFIO or architecture requirements. + * order to comply with VFIO or architecture requirements. If @forceVFIO is + * true then the limit is changed even if nothing in @vm's definition indicates + * so. * * The limit will not be changed unless doing so is needed; the first time * the limit is changed, the original (default) limit is stored in @vm and @@ -12093,12 +12104,13 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) * Returns: 0 on success, <0 on failure */ int -qemuDomainAdjustMaxMemLock(virDomainObjPtr vm) +qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, + bool forceVFIO) { unsigned long long bytes = 0; int ret = -1; - bytes = qemuDomainGetMemLockLimitBytes(vm->def); + bytes = qemuDomainGetMemLockLimitBytes(vm->def, forceVFIO); if (bytes) { /* If this is the first time adjusting the limit, save the current @@ -12147,7 +12159,7 @@ qemuDomainAdjustMaxMemLockHostdev(virDomainObjPtr vm, int ret = 0; vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; - if (qemuDomainAdjustMaxMemLock(vm) < 0) + if (qemuDomainAdjustMaxMemLock(vm, false) < 0) ret = -1; vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 65bd83aece..65d137462d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -955,8 +955,10 @@ bool qemuDomainSupportsPCI(virDomainDefPtr def, void qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm); -unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDefPtr def); -int qemuDomainAdjustMaxMemLock(virDomainObjPtr vm); +unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDefPtr def, + bool forceVFIO); +int qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, + bool forceVFIO); int qemuDomainAdjustMaxMemLockHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 053523f8c5..61deaffdd9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1646,7 +1646,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, if (teardowndevice && qemuDomainNamespaceTeardownHostdev(vm, hostdev) < 0) VIR_WARN("Unable to remove host device from /dev"); - if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm) < 0) + if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm, false) < 0) VIR_WARN("Unable to reset maximum locked memory on hotplug fail"); if (releaseaddr) @@ -2383,7 +2383,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (virDomainMemoryInsert(vm->def, mem) < 0) goto cleanup; - if (qemuDomainAdjustMaxMemLock(vm) < 0) + if (qemuDomainAdjustMaxMemLock(vm, false) < 0) goto removedef; qemuDomainObjEnterMonitor(driver, vm); @@ -2454,7 +2454,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, /* reset the mlock limit */ virErrorPreserveLast(&orig_err); - ignore_value(qemuDomainAdjustMaxMemLock(vm)); + ignore_value(qemuDomainAdjustMaxMemLock(vm, false)); virErrorRestore(&orig_err); goto audit; @@ -2857,7 +2857,7 @@ qemuDomainAttachMediatedDevice(virQEMUDriverPtr driver, ret = 0; cleanup: if (ret < 0) { - if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm) < 0) + if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm, false) < 0) VIR_WARN("Unable to reset maximum locked memory on hotplug fail"); if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); @@ -4356,7 +4356,7 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, ignore_value(qemuProcessRefreshBalloonState(driver, vm, QEMU_ASYNC_JOB_NONE)); /* decrease the mlock limit after memory unplug if necessary */ - ignore_value(qemuDomainAdjustMaxMemLock(vm)); + ignore_value(qemuDomainAdjustMaxMemLock(vm, false)); return 0; } @@ -4483,7 +4483,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, qemuDomainRemovePCIHostDevice(driver, vm, hostdev); /* QEMU might no longer need to lock as much memory, eg. we just * detached the last VFIO device, so adjust the limit here */ - if (qemuDomainAdjustMaxMemLock(vm) < 0) + if (qemuDomainAdjustMaxMemLock(vm, false) < 0) VIR_WARN("Failed to adjust locked memory limit"); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c index ef3bfa0345..23a144a8ce 100644 --- a/tests/qemumemlocktest.c +++ b/tests/qemumemlocktest.c @@ -42,7 +42,7 @@ testCompareMemLock(const void *data) goto cleanup; } - ret = virTestCompareToULL(info->memlock, qemuDomainGetMemLockLimitBytes(def)); + ret = virTestCompareToULL(info->memlock, qemuDomainGetMemLockLimitBytes(def, false)); cleanup: virDomainDefFree(def); -- 2.23.0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 58 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hostdev.c | 22 ++++++++++++++++ src/qemu/qemu_hostdev.h | 6 +++++ 3 files changed, 86 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be4aef049d..7d3a5d07d7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10570,6 +10570,54 @@ typedef enum { } qemuDomainStorageSourceAccessFlags; +static int +qemuDomainStorageSourceAccessModifyNVMe(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + bool revoke) +{ + bool revoke_maxmemlock = false; + bool revoke_hostdev = false; + int ret = -1; + + if (!virStorageSourceChainHasNVMe(src)) + return 0; + + VIR_DEBUG("Modifying access for a NVMe disk src=%p revoke=%d", + src, revoke); + + if (revoke) { + revoke_maxmemlock = true; + revoke_hostdev = true; + ret = 0; + goto revoke; + } + + if (qemuDomainAdjustMaxMemLock(vm, true) < 0) + goto revoke; + + revoke_maxmemlock = true; + + if (qemuHostdevPrepareOneNVMeDisk(driver, vm->def->name, src) < 0) + goto revoke; + + revoke_hostdev = true; + + return 0; + + revoke: + if (revoke_maxmemlock) { + if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + VIR_WARN("Unable to change max memlock limit"); + } + + if (revoke_hostdev) + qemuHostdevReAttachOneNVMeDisk(driver, vm->def->name, src); + + return ret; +} + + /** * qemuDomainStorageSourceAccessModify: * @driver: qemu driver struct @@ -10601,6 +10649,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, bool revoke_cgroup = false; bool revoke_label = false; bool revoke_namespace = false; + bool revoke_nvme = false; bool revoke_lockspace = false; VIR_DEBUG("src='%s' readonly=%d force_ro=%d force_rw=%d revoke=%d chain=%d", @@ -10618,6 +10667,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, revoke_cgroup = true; revoke_label = true; revoke_namespace = true; + revoke_nvme = true; revoke_lockspace = true; ret = 0; goto revoke; @@ -10628,6 +10678,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, revoke_lockspace = true; + if (qemuDomainStorageSourceAccessModifyNVMe(driver, vm, src, false) < 0) + goto revoke; + + revoke_nvme = true; + /* When modifying access of existing @src namespace does not need update */ if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) { if (qemuDomainNamespaceSetupDisk(vm, src) < 0) @@ -10678,6 +10733,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, VIR_WARN("Unable to remove /dev entry for %s", srcstr); } + if (revoke_nvme) + qemuDomainStorageSourceAccessModifyNVMe(driver, vm, src, true); + if (revoke_lockspace) { if (virDomainLockImageDetach(driver->lockManager, vm, src) < 0) VIR_WARN("Unable to release lock on %s", srcstr); diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 5ab0217858..6ab052724a 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -212,6 +212,17 @@ qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDefPtr *hostdevs, return true; } +int +qemuHostdevPrepareOneNVMeDisk(virQEMUDriverPtr driver, + const char *name, + virStorageSourcePtr src) +{ + return virHostdevPrepareOneNVMeDevice(driver->hostdevMgr, + QEMU_DRIVER_NAME, + name, + src); +} + int qemuHostdevPrepareNVMeDisks(virQEMUDriverPtr driver, const char *name, @@ -369,6 +380,17 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, return 0; } +void +qemuHostdevReAttachOneNVMeDisk(virQEMUDriverPtr driver, + const char *name, + virStorageSourcePtr src) +{ + virHostdevReAttachOneNVMeDevice(driver->hostdevMgr, + QEMU_DRIVER_NAME, + name, + src); +} + void qemuHostdevReAttachNVMeDisks(virQEMUDriverPtr driver, const char *name, diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 735414b6aa..23df925529 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -41,6 +41,9 @@ int qemuHostdevUpdateActiveSCSIDevices(virQEMUDriverPtr driver, int qemuHostdevUpdateActiveDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def); +int qemuHostdevPrepareOneNVMeDisk(virQEMUDriverPtr driver, + const char *name, + virStorageSourcePtr src); int qemuHostdevPrepareNVMeDisks(virQEMUDriverPtr driver, const char *name, virDomainDiskDefPtr *disks, @@ -74,6 +77,9 @@ int qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, unsigned int flags); +void qemuHostdevReAttachOneNVMeDisk(virQEMUDriverPtr driver, + const char *name, + virStorageSourcePtr src); void qemuHostdevReAttachNVMeDisks(virQEMUDriverPtr driver, const char *name, virDomainDiskDefPtr *disks, -- 2.23.0

This is slightly more complicated because NVMe disk source is not a simple attribute to <source/> element. The format in which the PCI address and namespace ID are printed is the same as QEMU accepts them: nvme://XXXX:XX:XX.X/X Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index c2f157ad6b..30b186ffd1 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -631,8 +631,8 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; + type = virXPathString("string(./@type)", ctxt); if (details) { - type = virXPathString("string(./@type)", ctxt); device = virXPathString("string(./@device)", ctxt); if (!type || !device) { vshPrint(ctl, "unable to query block list details"); @@ -645,11 +645,30 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) vshError(ctl, "unable to query block list"); goto cleanup; } - source = virXPathString("string(./source/@file" - "|./source/@dev" - "|./source/@dir" - "|./source/@name" - "|./source/@volume)", ctxt); + + if (STREQ_NULLABLE(type, "nvme")) { + g_autofree char *namespace = NULL; + virPCIDeviceAddress addr = { 0 }; + xmlNodePtr addrNode = NULL; + + if (!(namespace = virXPathString("string(./source/@namespace)", ctxt)) || + !(addrNode = virXPathNode("./source/address", ctxt)) || + virPCIDeviceAddressParseXML(addrNode, &addr) < 0) { + vshError(ctl, "Unable to query NVMe disk address"); + goto cleanup; + } + + source = g_strdup_printf("nvme://%04x:%02x:%02x.%d/%s", + addr.domain, addr.bus, addr.slot, + addr.function, namespace); + } else { + source = virXPathString("string(./source/@file" + "|./source/@dev" + "|./source/@dir" + "|./source/@name" + "|./source/@volume)", ctxt); + } + if (details) { if (vshTableRowAppend(table, type, device, target, NULLSTR_MINUS(source), NULL) < 0) -- 2.23.0