Re: [libvirt] [PATCH 1/2] qemu: add vfio devices to cgroup ACL when appropriate
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!