[libvirt] [PATCH 0/2] fix cgroup setup for vfio hostdevs
The first patch is required for vfio hostdev to work. The 2nd is just a refactor to eliminate duplicated code for usb hostdev cgroup setup. Laine Stump (2): qemu: add vfio devices to cgroup ACL when appropriate qemu: put usb cgroup setup in common function src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 182 ++++++++++++++++++++++++++++++++----- src/qemu/qemu_cgroup.h | 9 +- src/qemu/qemu_hotplug.c | 26 ++---- src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 174 insertions(+), 46 deletions(-) -- 1.7.11.7
PCIO device assignment using VFIO requires read/write access by the qemu process to /dev/vfio/vfio, and /dev/vfio/nn, where "nn" is the VFIO group number that the assigned device belongs to (and can be found with the function virPCIDeviceGetVFIOGroupDev) /dev/vfio/vfio can be accessible to any guest without danger (according to vfio developers), so it is added to the static ACL. The group device must be dynamically added to the cgroup ACL for each vfio hostdev in two places: 1) for any devices in the persistent config when the domain is started (done during qemuSetupCgroup()) 2) at device attach time for any hotplug devices (done in qemuDomainAttachHostDevice) The group device must be removed from the ACL when a device it "hot-unplugged" (in qemuDomainDetachHostDevice()) Note that USB devices are already doing their own cgroup setup and teardown in the hostdev-usb specific function. I chose to make the new functions generic and call them in a common location though. We can then move the USB-specific code (which is duplicated in two locations) to this single location. I'll be posting a followup patch to do that. --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 133 ++++++++++++++++++++++++++++++++++++- src/qemu/qemu_cgroup.h | 6 +- src/qemu/qemu_hotplug.c | 10 ++- src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 148 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 87bdf70..0f0a24c 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -241,7 +241,7 @@ # "/dev/null", "/dev/full", "/dev/zero", # "/dev/random", "/dev/urandom", # "/dev/ptmx", "/dev/kvm", "/dev/kqemu", -# "/dev/rtc","/dev/hpet" +# "/dev/rtc","/dev/hpet", "/dev/vfio/vfio" #] diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 891984a..92c53d9 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -39,7 +39,7 @@ static const char *const defaultDeviceACL[] = { "/dev/null", "/dev/full", "/dev/zero", "/dev/random", "/dev/urandom", "/dev/ptmx", "/dev/kvm", "/dev/kqemu", - "/dev/rtc", "/dev/hpet", + "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio", NULL, }; #define DEVICE_PTY_MAJOR 136 @@ -214,6 +214,131 @@ int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, } +int +qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virPCIDevicePtr pci = NULL; + char *path = NULL; + + /* currently this only does something for PCI devices using vfio + * for device assignment, but it is called for *all* hostdev + * devices. + */ + + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + if (dev->source.subsys.u.pci.backend + != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { + int rc; + + pci = virPCIDeviceNew(dev->source.subsys.u.pci.addr.domain, + dev->source.subsys.u.pci.addr.bus, + dev->source.subsys.u.pci.addr.slot, + dev->source.subsys.u.pci.addr.function); + if (!pci) + goto cleanup; + + if (!(path = virPCIDeviceGetVFIOGroupDev(pci))) + goto cleanup; + + VIR_DEBUG("Cgroup allow %s for PCI device assignment", path); + rc = virCgroupAllowDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RW); + virDomainAuditCgroupPath(vm, priv->cgroup, + "allow", path, "rw", rc); + if (rc < 0) { + virReportSystemError(-rc, + _("Unable to allow access " + "for device path %s"), + path); + goto cleanup; + } + } + break; + default: + break; + } + } + + ret = 0; +cleanup: + virPCIDeviceFree(pci); + VIR_FREE(path); + return ret; +} + + + +int +qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virPCIDevicePtr pci = NULL; + char *path = NULL; + + /* currently this only does something for PCI devices using vfio + * for device assignment, but it is called for *all* hostdev + * devices. + */ + + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + if (dev->source.subsys.u.pci.backend + != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { + int rc; + + pci = virPCIDeviceNew(dev->source.subsys.u.pci.addr.domain, + dev->source.subsys.u.pci.addr.bus, + dev->source.subsys.u.pci.addr.slot, + dev->source.subsys.u.pci.addr.function); + if (!pci) + goto cleanup; + + if (!(path = virPCIDeviceGetVFIOGroupDev(pci))) + goto cleanup; + + VIR_DEBUG("Cgroup deny %s for PCI device assignment", path); + rc = virCgroupDenyDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RWM); + virDomainAuditCgroupPath(vm, priv->cgroup, + "deny", path, "rwm", rc); + if (rc < 0) { + virReportSystemError(-rc, + _("Unable to deny access " + "for device path %s"), + path); + goto cleanup; + } + } + break; + default: + break; + } + } + + ret = 0; +cleanup: + virPCIDeviceFree(pci); + VIR_FREE(path); + return ret; +} + + int qemuInitCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm, bool startup) @@ -423,6 +548,12 @@ int qemuSetupCgroup(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i]; virUSBDevicePtr usb; + if (qemuSetupHostdevCGroup(vm, hostdev) < 0) + goto cleanup; + + /* NB: the code below here should be moved into + * qemuSetupHostdevCGroup() + */ if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) continue; if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index e63f443..64d71a5 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -1,7 +1,7 @@ /* * qemu_cgroup.h: QEMU cgroup management * - * Copyright (C) 2006-2007, 2009-2012 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -36,6 +36,10 @@ int qemuTeardownDiskCgroup(virDomainObjPtr vm, int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev, const char *path, void *opaque); +int qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev); int qemuInitCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm, bool startup); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f5fa1c4..eeee507 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1225,9 +1225,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virUSBDeviceListSteal(list, usb); } + if (qemuSetupHostdevCGroup(vm, hostdev) < 0) + goto cleanup; + if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) - goto cleanup; + goto teardown_cgroup; switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1257,6 +1260,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); +teardown_cgroup: + qemuTeardownHostdevCgroup(vm, hostdev); + cleanup: virObjectUnref(list); if (usb) @@ -2499,6 +2505,8 @@ int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, } if (!ret) { + qemuTeardownHostdevCgroup(vm, detach); + if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, detach, NULL) < 0) { VIR_WARN("Failed to restore host device labelling"); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 0aec997..26ca068 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -42,6 +42,7 @@ module Test_libvirtd_qemu = { "8" = "/dev/kqemu" } { "9" = "/dev/rtc" } { "10" = "/dev/hpet" } + { "11" = "/dev/vfio/vfio" } } { "save_image_format" = "raw" } { "dump_image_format" = "raw" } -- 1.7.11.7
On 04/29/2013 02:28 PM, Laine Stump wrote:
PCIO device assignment using VFIO requires read/write access by the qemu process to /dev/vfio/vfio, and /dev/vfio/nn, where "nn" is the VFIO group number that the assigned device belongs to (and can be found with the function virPCIDeviceGetVFIOGroupDev)
/dev/vfio/vfio can be accessible to any guest without danger (according to vfio developers), so it is added to the static ACL.
The group device must be dynamically added to the cgroup ACL for each vfio hostdev in two places:
1) for any devices in the persistent config when the domain is started (done during qemuSetupCgroup())
2) at device attach time for any hotplug devices (done in qemuDomainAttachHostDevice)
The group device must be removed from the ACL when a device it "hot-unplugged" (in qemuDomainDetachHostDevice())
Note that USB devices are already doing their own cgroup setup and teardown in the hostdev-usb specific function. I chose to make the new functions generic and call them in a common location though. We can then move the USB-specific code (which is duplicated in two locations) to this single location. I'll be posting a followup patch to do that. --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 133 ++++++++++++++++++++++++++++++++++++- src/qemu/qemu_cgroup.h | 6 +- src/qemu/qemu_hotplug.c | 10 ++- src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 148 insertions(+), 4 deletions(-)
@@ -36,6 +36,10 @@ int qemuTeardownDiskCgroup(virDomainObjPtr vm, int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev, const char *path, void *opaque); +int qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev);
A bit odd that setup is ATTRIBUTE_RETURN_CHECK but teardown is not. I'd rather see both require a use...
@@ -1257,6 +1260,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");
+teardown_cgroup: + qemuTeardownHostdevCgroup(vm, hostdev);
...and here, on cleanup paths after an earlier error, stick a VIR_WARN() that logs any failure trying to clean up (as we already did on the line before).
+ cleanup: virObjectUnref(list); if (usb) @@ -2499,6 +2505,8 @@ int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, }
if (!ret) { + qemuTeardownHostdevCgroup(vm, detach); + if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, detach, NULL) < 0) { VIR_WARN("Failed to restore host device labelling");
...here's another place where a VIR_WARN on failure to clean up is appropriate. ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/29/2013 05:46 PM, Eric Blake wrote:
On 04/29/2013 02:28 PM, Laine Stump wrote:
PCIO device assignment using VFIO requires read/write access by the qemu process to /dev/vfio/vfio, and /dev/vfio/nn, where "nn" is the VFIO group number that the assigned device belongs to (and can be found with the function virPCIDeviceGetVFIOGroupDev)
/dev/vfio/vfio can be accessible to any guest without danger (according to vfio developers), so it is added to the static ACL.
The group device must be dynamically added to the cgroup ACL for each vfio hostdev in two places:
1) for any devices in the persistent config when the domain is started (done during qemuSetupCgroup())
2) at device attach time for any hotplug devices (done in qemuDomainAttachHostDevice)
The group device must be removed from the ACL when a device it "hot-unplugged" (in qemuDomainDetachHostDevice())
Note that USB devices are already doing their own cgroup setup and teardown in the hostdev-usb specific function. I chose to make the new functions generic and call them in a common location though. We can then move the USB-specific code (which is duplicated in two locations) to this single location. I'll be posting a followup patch to do that. --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 133 ++++++++++++++++++++++++++++++++++++- src/qemu/qemu_cgroup.h | 6 +- src/qemu/qemu_hotplug.c | 10 ++- src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 148 insertions(+), 4 deletions(-)
@@ -36,6 +36,10 @@ int qemuTeardownDiskCgroup(virDomainObjPtr vm, int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev, const char *path, void *opaque); +int qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev); A bit odd that setup is ATTRIBUTE_RETURN_CHECK but teardown is not. I'd rather see both require a use...
I didn't include it for teardown because I wasn't checking the return (see below), and didn't want the extra line length caused by ignore_value(). (also, I notice that none of the other functions in qemu_cgroup.h have any sort of ATTRIBUTE_* usage at all).
@@ -1257,6 +1260,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");
+teardown_cgroup: + qemuTeardownHostdevCgroup(vm, hostdev); ...and here, on cleanup paths after an earlier error, stick a VIR_WARN() that logs any failure trying to clean up (as we already did on the line before).
But the teardown function itself is already logging an error message (as is the security manager function preceding it), so I don't really see the point of the additional VIR_WARN (I had seen the VIR_WARN on failure of the security manager, and concluded that it was redundant, so didn't replicate it). At any rate, I want to get this in before RC2, so I'll add a VIR_WARN and you can convince me (or I can convince you) later.
+ cleanup: virObjectUnref(list); if (usb) @@ -2499,6 +2505,8 @@ int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, }
if (!ret) { + qemuTeardownHostdevCgroup(vm, detach); + if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, detach, NULL) < 0) { VIR_WARN("Failed to restore host device labelling"); ...here's another place where a VIR_WARN on failure to clean up is appropriate.
ACK with that fixed.
I'm not certain I agree, but what you're requesting won't hurt anything, so in the interest of time I'll modify it that way and push. Thanks for the quick review!
On 04/29/2013 07:46 PM, Laine Stump wrote:
+int qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev); A bit odd that setup is ATTRIBUTE_RETURN_CHECK but teardown is not. I'd rather see both require a use...
I didn't include it for teardown because I wasn't checking the return (see below), and didn't want the extra line length caused by ignore_value(). (also, I notice that none of the other functions in qemu_cgroup.h have any sort of ATTRIBUTE_* usage at all).
Then maybe the simplest fix is to drop the RETURN_CHECK on the setup function?
@@ -1257,6 +1260,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");
+teardown_cgroup: + qemuTeardownHostdevCgroup(vm, hostdev); ...and here, on cleanup paths after an earlier error, stick a VIR_WARN() that logs any failure trying to clean up (as we already did on the line before).
But the teardown function itself is already logging an error message (as is the security manager function preceding it), so I don't really see the point of the additional VIR_WARN (I had seen the VIR_WARN on failure of the security manager, and concluded that it was redundant, so didn't replicate it).
At any rate, I want to get this in before RC2, so I'll add a VIR_WARN and you can convince me (or I can convince you) later.
I just wanted to make sure we have something in the log if cleanup goes wrong - it's unlikely, but that's exactly the case where having more information instead of less in the logs can help in deciphering what happened when things do go wrong, and how badly the system might have been compromised as a result of failure to clean up. It's fine with me if you can show that there was a warning emitted earlier in the call chain, so that a VIR_WARN on the cleanup path is redundant.
I'm not certain I agree, but what you're requesting won't hurt anything, so in the interest of time I'll modify it that way and push.
What you pushed looks fine to me. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/30/2013 10:49 AM, Eric Blake wrote:
On 04/29/2013 07:46 PM, Laine Stump wrote:
+int qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev); A bit odd that setup is ATTRIBUTE_RETURN_CHECK but teardown is not. I'd rather see both require a use...
I didn't include it for teardown because I wasn't checking the return (see below), and didn't want the extra line length caused by ignore_value(). (also, I notice that none of the other functions in qemu_cgroup.h have any sort of ATTRIBUTE_* usage at all). Then maybe the simplest fix is to drop the RETURN_CHECK on the setup function?
@@ -1257,6 +1260,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");
+teardown_cgroup: + qemuTeardownHostdevCgroup(vm, hostdev); ...and here, on cleanup paths after an earlier error, stick a VIR_WARN() that logs any failure trying to clean up (as we already did on the line before).
But the teardown function itself is already logging an error message (as is the security manager function preceding it), so I don't really see the point of the additional VIR_WARN (I had seen the VIR_WARN on failure of the security manager, and concluded that it was redundant, so didn't replicate it).
At any rate, I want to get this in before RC2, so I'll add a VIR_WARN and you can convince me (or I can convince you) later. I just wanted to make sure we have something in the log if cleanup goes wrong - it's unlikely, but that's exactly the case where having more information instead of less in the logs can help in deciphering what happened when things do go wrong, and how badly the system might have been compromised as a result of failure to clean up. It's fine with me if you can show that there was a warning emitted earlier in the call chain, so that a VIR_WARN on the cleanup path is redundant.
I'm not certain I agree, but what you're requesting won't hurt anything, so in the interest of time I'll modify it that way and push. What you pushed looks fine to me.
And something you said up above may have convinced me that putting in the VIR_WARN is useful - many times people will report a problem where they prominently display an error log message that turns out to be a problem inherent in attempting to clean up after the *real* failure occurred, and time is wasted speculating on the cause of this incidental error; with the VIR_WARN in there it would hopefully be immediately obvious that any errors logged were of this immaterial type, and didn't necessarily need further investigation.
The USB-specific cgroup setup had been inserted inline in qemuDomainAttachHostUsbDevice and qemuSetupCgroup, but now there is a common cgroup setup function called for all hostdevs, so it makes sens to put the usb-specific setup there and just rely on that function being called. The one thing I'm uncertain of here (and a reason for not pushing until after release) is that previously hostdev->missing was checked only when starting a domain (and cgroup setup for the device skipped if missing was true), but with this consolidation, it is now checked in the case of hotplug as well. I don't know if this will have any practical effect (does it make sense to hotplug a "missing" usb device?) --- src/qemu/qemu_cgroup.c | 61 ++++++++++++++++++++++++++----------------------- src/qemu/qemu_cgroup.h | 3 --- src/qemu/qemu_hotplug.c | 16 ------------- 3 files changed, 32 insertions(+), 48 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 92c53d9..7a7824d 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -191,9 +191,10 @@ qemuSetupTPMCgroup(virDomainDefPtr def, } -int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, - const char *path, - void *opaque) +static int +qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, + const char *path, + void *opaque) { virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -221,6 +222,7 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; virPCIDevicePtr pci = NULL; + virUSBDevicePtr usb = NULL; char *path = NULL; /* currently this only does something for PCI devices using vfio @@ -263,6 +265,28 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, } } break; + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + /* NB: hostdev->missing wasn't previously checked in the + * case of hotplug, only when starting a domain. Now it is + * always checked, and the cgroup setup skipped if true. + */ + if (dev->missing) + break; + if ((usb = virUSBDeviceNew(dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device, + NULL)) == NULL) { + goto cleanup; + } + + /* oddly, qemuSetupHostUsbDeviceCgroup doesn't ever + * reference the usb object we just created + */ + if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, + vm) < 0) { + goto cleanup; + } + break; default: break; } @@ -271,6 +295,7 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, ret = 0; cleanup: virPCIDeviceFree(pci); + virUSBDeviceFree(usb); VIR_FREE(path); return ret; } @@ -326,6 +351,9 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, } } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + /* nothing to tear down for USB */ + break; default: break; } @@ -545,33 +573,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver, goto cleanup; for (i = 0; i < vm->def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i]; - virUSBDevicePtr usb; - - if (qemuSetupHostdevCGroup(vm, hostdev) < 0) + if (qemuSetupHostdevCGroup(vm, vm->def->hostdevs[i]) < 0) goto cleanup; - - /* NB: the code below here should be moved into - * qemuSetupHostdevCGroup() - */ - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) - continue; - if (hostdev->missing) - continue; - - if ((usb = virUSBDeviceNew(hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device, - NULL)) == NULL) - goto cleanup; - - if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, - vm) < 0) { - virUSBDeviceFree(usb); - goto cleanup; - } - virUSBDeviceFree(usb); } } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 64d71a5..048c2fa 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -33,9 +33,6 @@ int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuTeardownDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk); -int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev, - const char *path, - void *opaque); int qemuSetupHostdevCGroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; int qemuTeardownHostdevCgroup(virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index eeee507..ad46a3d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1151,22 +1151,6 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, goto error; } - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { - virUSBDevicePtr usb; - - if ((usb = virUSBDeviceNew(hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device, - NULL)) == NULL) - goto error; - - if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, - vm) < 0) { - virUSBDeviceFree(usb); - goto error; - } - virUSBDeviceFree(usb); - } - qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) ret = qemuMonitorAddDevice(priv->mon, devstr); -- 1.7.11.7
On 04/29/2013 02:28 PM, Laine Stump wrote:
The USB-specific cgroup setup had been inserted inline in qemuDomainAttachHostUsbDevice and qemuSetupCgroup, but now there is a common cgroup setup function called for all hostdevs, so it makes sens to put the usb-specific setup there and just rely on that function being called.
The one thing I'm uncertain of here (and a reason for not pushing until after release) is that previously hostdev->missing was checked only when starting a domain (and cgroup setup for the device skipped if missing was true), but with this consolidation, it is now checked in the case of hotplug as well. I don't know if this will have any practical effect (does it make sense to hotplug a "missing" usb device?)
Good question - and yeah, that uncertainty makes me also wonder if it makes more sense to delay this patch until after 1.0.5 so we aren't invalidating testing done on rc1. On the other hand, the point of hostdev->missing is to gracefully ignore devices that are called out in XML but might not always exist; you are probably right that no one has tried doing a hotplug while specifying that a missing device do nothing. I could live with this patch as part of 1.0.5 if it makes it into rc2 so that it gets a few more days of testing. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump