[libvirt] [PATCH] qemu: fix hot unplug of PCI devices with VFIO

[Michal Privoznik]

On 05.02.2016 20:07, Ludovic Beliveau wrote:
> Currently, on hot unplug of PCI devices with VFIO driver for QEMU, libvirt is
> trying to restore the host devices to it's previous value (basically a chown
> on the previous user/group).
> 
> However for devices with VFIO driver, when the device is unbinded it is
> removed from the /dev/vfio file system causing the restore label to fail.
> 
> The fix is to not restore the label for those PCI devices since they are going
> to be teared down anyway.
> 
> Signed-off-by: Ludovic Beliveau <ludovic.beliveau at windriver.com>
> ---

Sorry for the delay. I had this patch marked for review but haven't been doing much review lately.

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f8db960..f5beabd 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2996,6 +2996,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>      int ret = -1;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      char *drivestr = NULL;
> +    virDomainHostdevSubsysPCIPtr pcisrc = NULL;
> +    bool is_vfio = false;
>  
>      VIR_DEBUG("Removing host device %s from domain %p %s",
>                hostdev->info->alias, vm, vm->def->name);
> @@ -3039,6 +3041,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>  
>      switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> +        pcisrc = &hostdev->source.subsys.u.pci;
> +        is_vfio = pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;

I'd prefer:
int backend = hostdev->source.subsys.u.pci.backend;
is_vfio = backend == ...;

>          qemuDomainRemovePCIHostDevice(driver, vm, hostdev);
>          /* QEMU might no longer need to lock as much memory, eg. we just
>           * detached the last VFIO device, so adjust the limit here */
> @@ -3058,7 +3062,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>      if (qemuTeardownHostdevCgroup(vm, hostdev) < 0)
>          VIR_WARN("Failed to remove host device cgroup ACL");
>  
> -    if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
> +    if (!is_vfio &&
> +        virSecurityManagerRestoreHostdevLabel(driver->securityManager,
>                                                vm->def, hostdev, NULL) < 0) {
>          VIR_WARN("Failed to restore host device labelling");
>      }

This approach is correct. But the same problem exists in qemuDomainAttachHostPCIDevice() under error label.

So ACK with this squashed in:

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3e02c86..afa99f1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1278,7 +1278,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
     if (virSecurityManagerSetHostdevLabel(driver->securityManager,
                                           vm->def, hostdev, NULL) < 0)
         goto error;
-    teardownlabel = true;
+    if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+        teardownlabel = true;
 
     if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
         if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0)
@@ -2985,7 +2986,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
     int ret = -1;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     char *drivestr = NULL;
-    virDomainHostdevSubsysPCIPtr pcisrc = NULL;
+    int backend;
     bool is_vfio = false;
 
     VIR_DEBUG("Removing host device %s from domain %p %s",
@@ -3030,8 +3031,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
 
     switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
-        pcisrc = &hostdev->source.subsys.u.pci;
-        is_vfio = pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
+        backend = hostdev->source.subsys.u.pci.backend;
+        is_vfio = backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
         qemuDomainRemovePCIHostDevice(driver, vm, hostdev);
         /* QEMU might no longer need to lock as much memory, eg. we just
          * detached the last VFIO device, so adjust the limit here */


Michal