So far, qemuDomainGetHostdevPath has no knowledge of the reasong
it is called and thus reports /dev/vfio/vfio for every VFIO
backed device. This is suboptimal, as we want it to:
a) report /dev/vfio/vfio on every addition or domain startup
b) report /dev/vfio/vfio only on last VFIO device being unplugged
If a domain is being stopped then namespace and CGroup die with
it so no need to worry about that. I mean, even when a domain
that's exiting has more than one VFIO devices assigned to it,
this function does not clean /dev/vfio/vfio in CGroup nor in the
namespace. But that doesn't matter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/qemu_cgroup.c | 87 ++++++++++++--------------------------------------
src/qemu/qemu_domain.c | 38 ++++++++++++++++------
src/qemu/qemu_domain.h | 4 ++-
3 files changed, 52 insertions(+), 77 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 944e8dc87..209cbc275 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -52,7 +52,6 @@ const char *const defaultDeviceACL[] = {
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
-#define DEV_VFIO "/dev/vfio/vfio"
static int
qemuSetupImagePathCgroup(virDomainObjPtr vm,
@@ -271,7 +270,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
size_t i, npaths = 0;
int rv, ret = -1;
- if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
+ if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, &perms) < 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -298,11 +297,10 @@ int
qemuTeardownHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
- int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
- virPCIDevicePtr pci = NULL;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
+ int rv, ret = -1;
/* currently this only does something for PCI devices using vfio
* for device assignment, but it is called for *all* hostdev
@@ -312,70 +310,27 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
- if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
-
- switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- int rv;
- size_t i, vfios = 0;
-
- pci = virPCIDeviceNew(pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
- if (!pci)
- goto cleanup;
-
- if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
- goto cleanup;
-
- VIR_DEBUG("Cgroup deny %s for PCI device assignment", path);
- rv = virCgroupDenyDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RWM, false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "deny", path, "rwm", rv == 0);
- if (rv < 0)
- goto cleanup;
-
- /* If this is the last hostdev with VFIO backend deny
- * /dev/vfio/vfio too. */
- for (i = 0; i < vm->def->nhostdevs; i++) {
- virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
- if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
- tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
- tmp->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
- vfios++;
- }
-
- if (vfios == 0) {
- VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device assignment");
- rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
- VIR_CGROUP_DEVICE_RWM, false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "deny", DEV_VFIO, "rwm", rv == 0);
- if (rv < 0)
- goto cleanup;
- }
- }
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- /* nothing to tear down for USB */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
- /* nothing to tear down for SCSI */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
- /* nothing to tear down for scsi_host */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
- break;
- }
+ if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ dev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO &&
+ qemuDomainGetHostdevPath(vm->def, dev, true,
+ &npaths, &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 == 0);
+ if (rv < 0)
+ goto cleanup;
}
ret = 0;
cleanup:
- virPCIDeviceFree(pci);
+ 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 530eced33..515e0052e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6834,7 +6834,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
/**
* 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
@@ -6849,7 +6851,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
* Returns 0 on success, -1 otherwise.
*/
int
-qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+qemuDomainGetHostdevPath(virDomainDefPtr def,
+ virDomainHostdevDefPtr dev,
+ bool teardown,
size_t *npaths,
char ***path,
int **perms)
@@ -6890,7 +6894,21 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
freeTmpPath = true;
perm = VIR_CGROUP_DEVICE_RW;
- includeVFIO = true;
+ if (teardown) {
+ size_t nvfios = 0;
+ for (i = 0; i < def->nhostdevs; i++) {
+ virDomainHostdevDefPtr tmp = def->hostdevs[i];
+ if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ tmp->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+ nvfios++;
+ }
+
+ if (nvfios == 0)
+ includeVFIO = true;
+ } else {
+ includeVFIO = true;
+ }
}
break;
@@ -7389,7 +7407,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
char **path = NULL;
size_t i, npaths = 0;
- if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, NULL) < 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -8024,7 +8042,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &path, NULL) < 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -8055,14 +8073,14 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(vm->def, hostdev, true,
+ &npaths, &path, NULL) < 0)
goto cleanup;
- /* Don't remove other paths than for the @hostdev itself.
- * They might be still in use by other devices. */
- if (npaths > 0 &&
- qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
+ goto cleanup;
+ }
ret = 0;
cleanup:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index e64aa25ba..80de50fbe 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -802,7 +802,9 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
virQEMUCapsPtr qemuCaps);
-int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+int qemuDomainGetHostdevPath(virDomainDefPtr def,
+ virDomainHostdevDefPtr dev,
+ bool teardown,
size_t *npaths,
char ***path,
int **perms);
--
2.11.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@redhat.com>
wrote:
> So far, qemuDomainGetHostdevPath has no knowledge of the reasong
>
reasong/reason
> it is called and thus reports /dev/vfio/vfio for every VFIO
> backed device. This is suboptimal, as we want it to:
>
> a) report /dev/vfio/vfio on every addition or domain startup
> b) report /dev/vfio/vfio only on last VFIO device being unplugged
>
> If a domain is being stopped then namespace and CGroup die with
> it so no need to worry about that. I mean, even when a domain
> that's exiting has more than one VFIO devices assigned to it,
> this function does not clean /dev/vfio/vfio in CGroup nor in the
> namespace. But that doesn't matter.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>
Fine approach to me,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> src/qemu/qemu_cgroup.c | 87
> ++++++++++++--------------------------------------
> src/qemu/qemu_domain.c | 38 ++++++++++++++++------
> src/qemu/qemu_domain.h | 4 ++-
> 3 files changed, 52 insertions(+), 77 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 944e8dc87..209cbc275 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -52,7 +52,6 @@ const char *const defaultDeviceACL[] = {
> #define DEVICE_PTY_MAJOR 136
> #define DEVICE_SND_MAJOR 116
>
> -#define DEV_VFIO "/dev/vfio/vfio"
>
> static int
> qemuSetupImagePathCgroup(virDomainObjPtr vm,
> @@ -271,7 +270,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
> size_t i, npaths = 0;
> int rv, ret = -1;
>
> - if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
> + if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path,
> &perms) < 0)
> goto cleanup;
>
> for (i = 0; i < npaths; i++) {
> @@ -298,11 +297,10 @@ int
> qemuTeardownHostdevCgroup(virDomainObjPtr vm,
> virDomainHostdevDefPtr dev)
> {
> - int ret = -1;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
> - virPCIDevicePtr pci = NULL;
> - char *path = NULL;
> + char **path = NULL;
> + size_t i, npaths = 0;
> + int rv, ret = -1;
>
> /* currently this only does something for PCI devices using vfio
> * for device assignment, but it is called for *all* hostdev
> @@ -312,70 +310,27 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
> if (!virCgroupHasController(priv->cgroup,
> VIR_CGROUP_CONTROLLER_DEVICES))
> return 0;
>
> - if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> -
> - switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> - if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> - int rv;
> - size_t i, vfios = 0;
> -
> - pci = virPCIDeviceNew(pcisrc->addr.domain,
> - pcisrc->addr.bus,
> - pcisrc->addr.slot,
> - pcisrc->addr.function);
> - if (!pci)
> - goto cleanup;
> -
> - if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
> - goto cleanup;
> -
> - VIR_DEBUG("Cgroup deny %s for PCI device assignment",
> path);
> - rv = virCgroupDenyDevicePath(priv->cgroup, path,
> - VIR_CGROUP_DEVICE_RWM,
> false);
> - virDomainAuditCgroupPath(vm, priv->cgroup,
> - "deny", path, "rwm", rv == 0);
> - if (rv < 0)
> - goto cleanup;
> -
> - /* If this is the last hostdev with VFIO backend deny
> - * /dev/vfio/vfio too. */
> - for (i = 0; i < vm->def->nhostdevs; i++) {
> - virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
> - if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> - tmp->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> - tmp->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
> - vfios++;
> - }
> -
> - if (vfios == 0) {
> - VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device
> assignment");
> - rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
> - VIR_CGROUP_DEVICE_RWM,
> false);
> - virDomainAuditCgroupPath(vm, priv->cgroup,
> - "deny", DEV_VFIO, "rwm", rv
> == 0);
> - if (rv < 0)
> - goto cleanup;
> - }
> - }
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> - /* nothing to tear down for USB */
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> - /* nothing to tear down for SCSI */
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
> - /* nothing to tear down for scsi_host */
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> - break;
> - }
> + if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> + dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> + dev->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO &&
> + qemuDomainGetHostdevPath(vm->def, dev, true,
> + &npaths, &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 == 0);
> + if (rv < 0)
> + goto cleanup;
> }
>
> ret = 0;
> cleanup:
> - virPCIDeviceFree(pci);
> + 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 530eced33..515e0052e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6834,7 +6834,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
> video,
>
> /**
> * 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
> @@ -6849,7 +6851,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
> video,
> * Returns 0 on success, -1 otherwise.
> */
> int
> -qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
> +qemuDomainGetHostdevPath(virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + bool teardown,
> size_t *npaths,
> char ***path,
> int **perms)
> @@ -6890,7 +6894,21 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
> freeTmpPath = true;
>
> perm = VIR_CGROUP_DEVICE_RW;
> - includeVFIO = true;
> + if (teardown) {
> + size_t nvfios = 0;
> + for (i = 0; i < def->nhostdevs; i++) {
> + virDomainHostdevDefPtr tmp = def->hostdevs[i];
> + if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> + tmp->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> + tmp->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
> + nvfios++;
> + }
> +
> + if (nvfios == 0)
> + includeVFIO = true;
> + } else {
> + includeVFIO = true;
> + }
> }
> break;
>
> @@ -7389,7 +7407,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver
> ATTRIBUTE_UNUSED,
> char **path = NULL;
> size_t i, npaths = 0;
>
> - if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
> + if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, NULL)
> < 0)
> goto cleanup;
>
> for (i = 0; i < npaths; i++) {
> @@ -8024,7 +8042,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr
> driver,
> if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
> return 0;
>
> - if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
> + if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &path,
> NULL) < 0)
> goto cleanup;
>
> for (i = 0; i < npaths; i++) {
> @@ -8055,14 +8073,14 @@
> qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
> if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
> return 0;
>
> - if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
> + if (qemuDomainGetHostdevPath(vm->def, hostdev, true,
> + &npaths, &path, NULL) < 0)
> goto cleanup;
>
> - /* Don't remove other paths than for the @hostdev itself.
> - * They might be still in use by other devices. */
> - if (npaths > 0 &&
> - qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
> - goto cleanup;
> + for (i = 0; i < npaths; i++) {
> + if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
> + goto cleanup;
> + }
>
> ret = 0;
> cleanup:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index e64aa25ba..80de50fbe 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -802,7 +802,9 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
> bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
> virQEMUCapsPtr qemuCaps);
>
> -int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
> +int qemuDomainGetHostdevPath(virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + bool teardown,
> size_t *npaths,
> char ***path,
> int **perms);
> --
> 2.11.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Marc-André Lureau
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.