[libvirt] [PATCH 1/2] qemu: add vfio devices to cgroup ACL when appropriate

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130429/8171453d/attachment-0001.sig>