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

[Laine Stump]

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!