On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mpriv...@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 <mpriv...@redhat.com> > Fine approach to me, Reviewed-by: Marc-André Lureau <marcandre.lur...@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