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! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list