By using the new qemu monitor functions to handle passing and removing
file descriptors, we can support hotplug of vdpa devices.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 src/qemu/qemu_hotplug.c                       | 60 +++++++++++++++++--
 tests/qemuhotplugmock.c                       |  9 +++
 tests/qemuhotplugtest.c                       | 16 +++++
 .../qemuhotplug-interface-vdpa.xml            |  4 ++
 .../qemuhotplug-base-live+interface-vdpa.xml  | 57 ++++++++++++++++++
 5 files changed, 142 insertions(+), 4 deletions(-)
 create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml
 create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0582b78f97..3a2aff607c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1152,6 +1152,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
     virErrorPtr originalError = NULL;
     g_autofree char *slirpfdName = NULL;
     int slirpfd = -1;
+    g_autofree char *vdpafdName = NULL;
+    int vdpafd = -1;
     char **tapfdName = NULL;
     int *tapfd = NULL;
     size_t tapfdSize = 0;
@@ -1335,12 +1337,16 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
         /* hostdev interfaces were handled earlier in this function */
         break;
 
+    case VIR_DOMAIN_NET_TYPE_VDPA:
+        if ((vdpafd = qemuInterfaceVDPAConnect(net)) < 0)
+            goto cleanup;
+        break;
+
     case VIR_DOMAIN_NET_TYPE_SERVER:
     case VIR_DOMAIN_NET_TYPE_CLIENT:
     case VIR_DOMAIN_NET_TYPE_MCAST:
     case VIR_DOMAIN_NET_TYPE_INTERNAL:
     case VIR_DOMAIN_NET_TYPE_UDP:
-    case VIR_DOMAIN_NET_TYPE_VDPA:
     case VIR_DOMAIN_NET_TYPE_LAST:
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
                        _("hotplug of interface type of %s is not implemented yet"),
@@ -1386,14 +1392,28 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
     for (i = 0; i < vhostfdSize; i++)
         vhostfdName[i] = g_strdup_printf("vhostfd-%s%zu", net->info.alias, i);
 
+    qemuDomainObjEnterMonitor(driver, vm);
+
+    if (vdpafd > 0) {
+        /* vhost-vdpa only takes a filename for the dev, but we want to pass an
+         * open fd to qemu. Passing -1 as the fdset-id will create a new fdset
+         * and return the id of that fdset */
+        qemuMonitorAddFdInfo fdinfo;
+        if (qemuMonitorAddFileHandleToSet(priv->mon, vdpafd, -1,
+                                          net->data.vdpa.devicepath,
+                                          &fdinfo) < 0) {
+            ignore_value(qemuDomainObjExitMonitor(driver, vm));
+            goto cleanup;
+        }
+        vdpafdName = g_strdup_printf("/dev/fdset/%d", fdinfo.fdset);
+    }
+
     if (!(netprops = qemuBuildHostNetStr(net,
                                          tapfdName, tapfdSize,
                                          vhostfdName, vhostfdSize,
-                                         slirpfdName, NULL)))
+                                         slirpfdName, vdpafdName)))
         goto cleanup;
 
-    qemuDomainObjEnterMonitor(driver, vm);
-
     if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
         if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) {
             ignore_value(qemuDomainObjExitMonitor(driver, vm));
@@ -1518,6 +1538,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
     VIR_FREE(vhostfdName);
     virDomainCCWAddressSetFree(ccwaddrs);
     VIR_FORCE_CLOSE(slirpfd);
+    VIR_FORCE_CLOSE(vdpafd);
 
     return ret;
 
@@ -4586,8 +4607,39 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
              * to just ignore the error and carry on.
              */
         }
+    } else if (actualType == VIR_DOMAIN_NET_TYPE_VDPA) {
+        int vdpafdset = -1;
+        g_autoptr(qemuMonitorFdsets) fdsets = NULL;
+
+        /* query qemu for which fdset is associated with the fd that we passed
+         * to qemu via 'add-fd' for this vdpa device. If we don't remove the
+         * fd, qemu will keep it open */
+        if (qemuMonitorQueryFdsets(priv->mon, &fdsets) == 0) {
+            for (i = 0; i < fdsets->nfdsets && vdpafdset < 0; i++) {
+                size_t j;
+                qemuMonitorFdsetInfoPtr set = &fdsets->fdsets[i];
+
+                for (j = 0; j < set->nfds; j++) {
+                    qemuMonitorFdsetFdInfoPtr fdinfo = &set->fds[j];
+                    if (STREQ_NULLABLE(fdinfo->opaque, net->data.vdpa.devicepath)) {
+                        vdpafdset = set->id;
+                        break;
+                    }
+                }
+            }
+        }
+
+        if (vdpafdset < 0) {
+            VIR_WARN("Cannot determine fdset for vdpa device");
+        } else {
+            if (qemuMonitorRemoveFdset(priv->mon, vdpafdset) < 0) {
+                /* if it fails, there's not much we can do... just carry on */
+                VIR_WARN("failed to close vdpa device");
+            }
+        }
     }
 
+
     if (qemuDomainObjExitMonitor(driver, vm) < 0)
         return -1;
 
diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
index 29fac8a598..d2e32ecf7e 100644
--- a/tests/qemuhotplugmock.c
+++ b/tests/qemuhotplugmock.c
@@ -19,11 +19,13 @@
 #include <config.h>
 
 #include "qemu/qemu_hotplug.h"
+#include "qemu/qemu_interface.h"
 #include "qemu/qemu_process.h"
 #include "conf/domain_conf.h"
 #include "virdevmapper.h"
 #include "virutil.h"
 #include "virmock.h"
+#include <fcntl.h>
 
 static int (*real_virGetDeviceID)(const char *path, int *maj, int *min);
 static bool (*real_virFileExists)(const char *path);
@@ -106,3 +108,10 @@ void
 qemuProcessKillManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
 {
 }
+
+int
+qemuInterfaceVDPAConnect(virDomainNetDefPtr net G_GNUC_UNUSED)
+{
+    /* need a valid fd or sendmsg won't work. Just open /dev/null */
+    return open("/dev/null", O_RDONLY);
+}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 2d12cacf28..b7cebfc0e7 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -89,6 +89,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
     virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
     virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER);
     virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SCSI_BLOCK);
+    virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_NETDEV_VHOST_VDPA);
 
     if (qemuTestCapsCacheInsert(driver.qemuCapsCache, priv->qemuCaps) < 0)
         return -1;
@@ -140,6 +141,9 @@ testQemuHotplugAttach(virDomainObjPtr vm,
     case VIR_DOMAIN_DEVICE_HOSTDEV:
         ret = qemuDomainAttachHostDevice(&driver, vm, dev->data.hostdev);
         break;
+    case VIR_DOMAIN_DEVICE_NET:
+        ret = qemuDomainAttachNetDevice(&driver, vm, dev->data.net);
+        break;
     default:
         VIR_TEST_VERBOSE("device type '%s' cannot be attached",
                 virDomainDeviceTypeToString(dev->type));
@@ -162,6 +166,7 @@ testQemuHotplugDetach(virDomainObjPtr vm,
     case VIR_DOMAIN_DEVICE_SHMEM:
     case VIR_DOMAIN_DEVICE_WATCHDOG:
     case VIR_DOMAIN_DEVICE_HOSTDEV:
+    case VIR_DOMAIN_DEVICE_NET:
         ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async);
         break;
     default:
@@ -823,6 +828,17 @@ mymain(void)
     DO_TEST_DETACH("pseries-base-live", "hostdev-pci", false, false,
                    "device_del", QMP_DEVICE_DELETED("hostdev0") QMP_OK);
 
+    DO_TEST_ATTACH("base-live", "interface-vdpa", false, true,
+                   "add-fd", "{ \"return\": { \"fdset-id\": 1, \"fd\": 95 }}",
+                   "netdev_add", QMP_OK, "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "interface-vdpa", false, false,
+                   "device_del", QMP_DEVICE_DELETED("net0") QMP_OK,
+                   "netdev_del", QMP_OK,
+                   "query-fdsets",
+                   "{ \"return\": [{\"fds\": [{\"fd\": 95, \"opaque\": \"/dev/vhost-vdpa-0\"}], \"fdset-id\": 1}]}",
+                   "remove-fd", QMP_OK
+                   );
+
     DO_TEST_ATTACH("base-live", "watchdog", false, true,
                    "watchdog-set-action", QMP_OK,
                    "device_add", QMP_OK);
diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml b/tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml
new file mode 100644
index 0000000000..e42ca08d31
--- /dev/null
+++ b/tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml
@@ -0,0 +1,4 @@
+<interface type='vdpa'>
+    <mac address='52:54:00:39:5f:04'/>
+    <source dev='/dev/vhost-vdpa-0'/>
+</interface>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml
new file mode 100644
index 0000000000..066180bb3c
--- /dev/null
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml
@@ -0,0 +1,57 @@
+<domain type='kvm' id='7'>
+  <name>hotplug</name>
+  <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid>
+  <memory unit='KiB'>4194304</memory>
+  <currentMemory unit='KiB'>4194304</currentMemory>
+  <vcpu placement='static'>4</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0'>
+      <alias name='usb'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='ide' index='0'>
+      <alias name='ide'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+    </controller>
+    <controller type='scsi' index='0' model='virtio-scsi'>
+      <alias name='scsi0'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'>
+      <alias name='pci'/>
+    </controller>
+    <controller type='virtio-serial' index='0'>
+      <alias name='virtio-serial0'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+    </controller>
+    <interface type='vdpa'>
+      <mac address='52:54:00:39:5f:04'/>
+      <source dev='/dev/vhost-vdpa-0'/>
+      <model type='virtio'/>
+      <alias name='net0'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
+    </interface>
+    <input type='mouse' bus='ps2'>
+      <alias name='input0'/>
+    </input>
+    <input type='keyboard' bus='ps2'>
+      <alias name='input1'/>
+    </input>
+    <memballoon model='none'/>
+  </devices>
+  <seclabel type='none' model='none'/>
+</domain>
-- 
2.26.2

On 9/24/20 5:45 PM, Jonathon Jongsma wrote:
> By using the new qemu monitor functions to handle passing and removing
> file descriptors, we can support hotplug of vdpa devices.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   src/qemu/qemu_hotplug.c                       | 60 +++++++++++++++++--
>   tests/qemuhotplugmock.c                       |  9 +++
>   tests/qemuhotplugtest.c                       | 16 +++++
>   .../qemuhotplug-interface-vdpa.xml            |  4 ++
>   .../qemuhotplug-base-live+interface-vdpa.xml  | 57 ++++++++++++++++++
>   5 files changed, 142 insertions(+), 4 deletions(-)
>   create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml
>   create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 0582b78f97..3a2aff607c 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1152,6 +1152,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>       virErrorPtr originalError = NULL;
>       g_autofree char *slirpfdName = NULL;
>       int slirpfd = -1;
> +    g_autofree char *vdpafdName = NULL;
> +    int vdpafd = -1;
>       char **tapfdName = NULL;
>       int *tapfd = NULL;
>       size_t tapfdSize = 0;
> @@ -1335,12 +1337,16 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>           /* hostdev interfaces were handled earlier in this function */
>           break;
>   
> +    case VIR_DOMAIN_NET_TYPE_VDPA:
> +        if ((vdpafd = qemuInterfaceVDPAConnect(net)) < 0)
> +            goto cleanup;
> +        break;
> +
>       case VIR_DOMAIN_NET_TYPE_SERVER:
>       case VIR_DOMAIN_NET_TYPE_CLIENT:
>       case VIR_DOMAIN_NET_TYPE_MCAST:
>       case VIR_DOMAIN_NET_TYPE_INTERNAL:
>       case VIR_DOMAIN_NET_TYPE_UDP:
> -    case VIR_DOMAIN_NET_TYPE_VDPA:
>       case VIR_DOMAIN_NET_TYPE_LAST:
>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                          _("hotplug of interface type of %s is not implemented yet"),
> @@ -1386,14 +1392,28 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>       for (i = 0; i < vhostfdSize; i++)
>           vhostfdName[i] = g_strdup_printf("vhostfd-%s%zu", net->info.alias, i);
>   
> +    qemuDomainObjEnterMonitor(driver, vm);


So this was moved up ahead of the call to qemuBuildHostNetStr()...


> +
> +    if (vdpafd > 0) {
> +        /* vhost-vdpa only takes a filename for the dev, but we want to pass an
> +         * open fd to qemu. Passing -1 as the fdset-id will create a new fdset
> +         * and return the id of that fdset */
> +        qemuMonitorAddFdInfo fdinfo;
> +        if (qemuMonitorAddFileHandleToSet(priv->mon, vdpafd, -1,
> +                                          net->data.vdpa.devicepath,
> +                                          &fdinfo) < 0) {
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +            goto cleanup;


... and here you ExitMonitor prior to goto cleanup on failure...

> +        }
> +        vdpafdName = g_strdup_printf("/dev/fdset/%d", fdinfo.fdset);
> +    }
> +
>       if (!(netprops = qemuBuildHostNetStr(net,
>                                            tapfdName, tapfdSize,
>                                            vhostfdName, vhostfdSize,
> -                                         slirpfdName, NULL)))
> +                                         slirpfdName, vdpafdName)))
>           goto cleanup;


...but  here you don't. (and should)


(NB: this change does put qemuBuildHostNetStr() inside the Monitor 
section, but it just does a bit of string shuffling/creation, so that's 
not a big deal)


>   
> -    qemuDomainObjEnterMonitor(driver, vm);
> -


(^^ old location of qemuDomainObjEnterMonitor())


>       if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
>           if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) {
>               ignore_value(qemuDomainObjExitMonitor(driver, vm));
> @@ -1518,6 +1538,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>       VIR_FREE(vhostfdName);
>       virDomainCCWAddressSetFree(ccwaddrs);
>       VIR_FORCE_CLOSE(slirpfd);
> +    VIR_FORCE_CLOSE(vdpafd);
>   
>       return ret;
>   
> @@ -4586,8 +4607,39 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>                * to just ignore the error and carry on.
>                */
>           }
> +    } else if (actualType == VIR_DOMAIN_NET_TYPE_VDPA) {
> +        int vdpafdset = -1;
> +        g_autoptr(qemuMonitorFdsets) fdsets = NULL;
> +
> +        /* query qemu for which fdset is associated with the fd that we passed
> +         * to qemu via 'add-fd' for this vdpa device. If we don't remove the
> +         * fd, qemu will keep it open */
> +        if (qemuMonitorQueryFdsets(priv->mon, &fdsets) == 0) {
> +            for (i = 0; i < fdsets->nfdsets && vdpafdset < 0; i++) {
> +                size_t j;
> +                qemuMonitorFdsetInfoPtr set = &fdsets->fdsets[i];
> +
> +                for (j = 0; j < set->nfds; j++) {
> +                    qemuMonitorFdsetFdInfoPtr fdinfo = &set->fds[j];
> +                    if (STREQ_NULLABLE(fdinfo->opaque, net->data.vdpa.devicepath)) {
> +                        vdpafdset = set->id;
> +                        break;
> +                    }
> +                }
> +            }
> +        }
> +
> +        if (vdpafdset < 0) {
> +            VIR_WARN("Cannot determine fdset for vdpa device");
> +        } else {
> +            if (qemuMonitorRemoveFdset(priv->mon, vdpafdset) < 0) {
> +                /* if it fails, there's not much we can do... just carry on */
> +                VIR_WARN("failed to close vdpa device");
> +            }
> +        }


I agree there's not much we can do here to make the situation better, 
but is it really going to be okay to inform the user that the device is 
now free, since it apparently isn't? If we go ahead and send the 
DEVICE_DELETED event up to the application, then it will think that the 
same vdpa device is now available to be re-used elsewhere. Do you have 
an idea what are the odds on that being true? (I don't, that's why I'm 
asking :-)).


It may be safer to return failure, so the device is just stuck shown as 
in-use by this guest; that would be bad, but maybe not as bad as if it 
was still actually being used by this guest somehow (possible, since the 
fd couldn't be deleted), and a 2nd guest started using it too. (I really 
don't know what the consequences of any of this might be; just trying to 
inject my sunny disposition into the mix; truthfully I'd be willing to 
accept either way, just wanted to make sure it's considered).

>       }
>   
> +
>       if (qemuDomainObjExitMonitor(driver, vm) < 0)
>           return -1;
>   
> diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
> index 29fac8a598..d2e32ecf7e 100644
> --- a/tests/qemuhotplugmock.c
> +++ b/tests/qemuhotplugmock.c
> @@ -19,11 +19,13 @@
>   #include <config.h>
>   
>   #include "qemu/qemu_hotplug.h"
> +#include "qemu/qemu_interface.h"
>   #include "qemu/qemu_process.h"
>   #include "conf/domain_conf.h"
>   #include "virdevmapper.h"
>   #include "virutil.h"
>   #include "virmock.h"
> +#include <fcntl.h>
>   
>   static int (*real_virGetDeviceID)(const char *path, int *maj, int *min);
>   static bool (*real_virFileExists)(const char *path);
> @@ -106,3 +108,10 @@ void
>   qemuProcessKillManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
>   {
>   }
> +
> +int
> +qemuInterfaceVDPAConnect(virDomainNetDefPtr net G_GNUC_UNUSED)
> +{
> +    /* need a valid fd or sendmsg won't work. Just open /dev/null */
> +    return open("/dev/null", O_RDONLY);
> +}
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index 2d12cacf28..b7cebfc0e7 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -89,6 +89,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
>       virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
>       virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER);
>       virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SCSI_BLOCK);
> +    virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_NETDEV_VHOST_VDPA);
>   
>       if (qemuTestCapsCacheInsert(driver.qemuCapsCache, priv->qemuCaps) < 0)
>           return -1;
> @@ -140,6 +141,9 @@ testQemuHotplugAttach(virDomainObjPtr vm,
>       case VIR_DOMAIN_DEVICE_HOSTDEV:
>           ret = qemuDomainAttachHostDevice(&driver, vm, dev->data.hostdev);
>           break;
> +    case VIR_DOMAIN_DEVICE_NET:
> +        ret = qemuDomainAttachNetDevice(&driver, vm, dev->data.net);
> +        break;


Nice attention to detail - nobody before you has bothered with a hotplug 
test for a network device :-)


>       default:
>           VIR_TEST_VERBOSE("device type '%s' cannot be attached",
>                   virDomainDeviceTypeToString(dev->type));
> @@ -162,6 +166,7 @@ testQemuHotplugDetach(virDomainObjPtr vm,
>       case VIR_DOMAIN_DEVICE_SHMEM:
>       case VIR_DOMAIN_DEVICE_WATCHDOG:
>       case VIR_DOMAIN_DEVICE_HOSTDEV:
> +    case VIR_DOMAIN_DEVICE_NET:
>           ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async);
>           break;
>       default:
> @@ -823,6 +828,17 @@ mymain(void)
>       DO_TEST_DETACH("pseries-base-live", "hostdev-pci", false, false,
>                      "device_del", QMP_DEVICE_DELETED("hostdev0") QMP_OK);
>   
> +    DO_TEST_ATTACH("base-live", "interface-vdpa", false, true,
> +                   "add-fd", "{ \"return\": { \"fdset-id\": 1, \"fd\": 95 }}",
> +                   "netdev_add", QMP_OK, "device_add", QMP_OK);
> +    DO_TEST_DETACH("base-live", "interface-vdpa", false, false,
> +                   "device_del", QMP_DEVICE_DELETED("net0") QMP_OK,
> +                   "netdev_del", QMP_OK,
> +                   "query-fdsets",
> +                   "{ \"return\": [{\"fds\": [{\"fd\": 95, \"opaque\": \"/dev/vhost-vdpa-0\"}], \"fdset-id\": 1}]}",
> +                   "remove-fd", QMP_OK
> +                   );
> +
>       DO_TEST_ATTACH("base-live", "watchdog", false, true,
>                      "watchdog-set-action", QMP_OK,
>                      "device_add", QMP_OK);
> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml b/tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml
> new file mode 100644
> index 0000000000..e42ca08d31
> --- /dev/null
> +++ b/tests/qemuhotplugtestdevices/qemuhotplug-interface-vdpa.xml
> @@ -0,0 +1,4 @@
> +<interface type='vdpa'>
> +    <mac address='52:54:00:39:5f:04'/>
> +    <source dev='/dev/vhost-vdpa-0'/>
> +</interface>
> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml
> new file mode 100644
> index 0000000000..066180bb3c
> --- /dev/null
> +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+interface-vdpa.xml
> @@ -0,0 +1,57 @@
> +<domain type='kvm' id='7'>
> +  <name>hotplug</name>
> +  <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid>
> +  <memory unit='KiB'>4194304</memory>
> +  <currentMemory unit='KiB'>4194304</currentMemory>
> +  <vcpu placement='static'>4</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <features>
> +    <acpi/>
> +    <apic/>
> +    <pae/>
> +  </features>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>restart</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-x86_64</emulator>
> +    <controller type='usb' index='0'>
> +      <alias name='usb'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='ide' index='0'>
> +      <alias name='ide'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> +    </controller>
> +    <controller type='scsi' index='0' model='virtio-scsi'>
> +      <alias name='scsi0'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'>
> +      <alias name='pci'/>
> +    </controller>
> +    <controller type='virtio-serial' index='0'>
> +      <alias name='virtio-serial0'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> +    </controller>
> +    <interface type='vdpa'>
> +      <mac address='52:54:00:39:5f:04'/>
> +      <source dev='/dev/vhost-vdpa-0'/>
> +      <model type='virtio'/>
> +      <alias name='net0'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
> +    </interface>
> +    <input type='mouse' bus='ps2'>
> +      <alias name='input0'/>
> +    </input>
> +    <input type='keyboard' bus='ps2'>
> +      <alias name='input1'/>
> +    </input>
> +    <memballoon model='none'/>
> +  </devices>
> +  <seclabel type='none' model='none'/>
> +</domain>


With the ExitMonitor() added where indicated, consideration of possibly 
failing if the fd can't be deleted, and (as with the rest of the series) 
as long as it's been possible to test with real hardware:


Reviewed-by: Laine Stump <laine@redhat.com>

On Tue, 29 Sep 2020 15:53:39 -0400
Laine Stump <laine@redhat.com> wrote:

> > +
> > +        if (vdpafdset < 0) {
> > +            VIR_WARN("Cannot determine fdset for vdpa device");
> > +        } else {
> > +            if (qemuMonitorRemoveFdset(priv->mon, vdpafdset) < 0) {
> > +                /* if it fails, there's not much we can do... just
> > carry on */
> > +                VIR_WARN("failed to close vdpa device");
> > +            }
> > +        }  
> 
> 
> I agree there's not much we can do here to make the situation better, 
> but is it really going to be okay to inform the user that the device
> is now free, since it apparently isn't? If we go ahead and send the 
> DEVICE_DELETED event up to the application, then it will think that
> the same vdpa device is now available to be re-used elsewhere. Do you
> have an idea what are the odds on that being true? (I don't, that's
> why I'm asking :-)).

I don't either ;)


> It may be safer to return failure, so the device is just stuck shown
> as in-use by this guest; that would be bad, but maybe not as bad as
> if it was still actually being used by this guest somehow (possible,
> since the fd couldn't be deleted), and a 2nd guest started using it
> too. (I really don't know what the consequences of any of this might
> be; just trying to inject my sunny disposition into the mix;
> truthfully I'd be willing to accept either way, just wanted to make
> sure it's considered).

Well, that's a good point. The reason that I printed a warning rather
than returning an error is because I was influenced by some of the
nearby code. 

In order to remove a network device, this function has to do a couple
things (depending on the type of network device). First It removes the
netdev (netdev_del), and then it may need to do some additional cleanup.
For TYPE_VHOSTUSER, it needs to detach a chardev. For TYPE_VDPA, it
needs to close the fd that we passed to qemu. So what do we do if
'netdev_del' succeeds, but 'remove-fd' does not?

If we return an error from this function, the caller will interpret that
as if we failed to remove the network device. But qemu has already
removed the netdev. So things are in an inconsistent state.

TYPE_VHOSTUSER just carries on without even printing a warning if the
chardev can't be removed. So I did something similar here for vDPA, but
added a warning. I'm not sure that there's really a "good" solution
here. 

Regarding the possibility of a second guest attempting to use the vdpa
device that was unsuccessfully removed: I have only tested with the
vdpa_sim kernel module, but if the fd is not closed, attempting to
re-use it with a different guest fails like this:

error: Failed to attach device from vdpa.xml
error: Unable to open '/dev/vhost-vdpa-0' for vdpa device: Device or
resource busy

Jonathon

On 10/1/20 5:08 PM, Jonathon Jongsma wrote:
> On Tue, 29 Sep 2020 15:53:39 -0400
> Laine Stump <laine@redhat.com> wrote:
>
>>> +
>>> +        if (vdpafdset < 0) {
>>> +            VIR_WARN("Cannot determine fdset for vdpa device");
>>> +        } else {
>>> +            if (qemuMonitorRemoveFdset(priv->mon, vdpafdset) < 0) {
>>> +                /* if it fails, there's not much we can do... just
>>> carry on */
>>> +                VIR_WARN("failed to close vdpa device");
>>> +            }
>>> +        }
>>
>> I agree there's not much we can do here to make the situation better,
>> but is it really going to be okay to inform the user that the device
>> is now free, since it apparently isn't? If we go ahead and send the
>> DEVICE_DELETED event up to the application, then it will think that
>> the same vdpa device is now available to be re-used elsewhere. Do you
>> have an idea what are the odds on that being true? (I don't, that's
>> why I'm asking :-)).
> I don't either ;)
>
>
>> It may be safer to return failure, so the device is just stuck shown
>> as in-use by this guest; that would be bad, but maybe not as bad as
>> if it was still actually being used by this guest somehow (possible,
>> since the fd couldn't be deleted), and a 2nd guest started using it
>> too. (I really don't know what the consequences of any of this might
>> be; just trying to inject my sunny disposition into the mix;
>> truthfully I'd be willing to accept either way, just wanted to make
>> sure it's considered).
> Well, that's a good point. The reason that I printed a warning rather
> than returning an error is because I was influenced by some of the
> nearby code.
>
> In order to remove a network device, this function has to do a couple
> things (depending on the type of network device). First It removes the
> netdev (netdev_del), and then it may need to do some additional cleanup.
> For TYPE_VHOSTUSER, it needs to detach a chardev. For TYPE_VDPA, it
> needs to close the fd that we passed to qemu. So what do we do if
> 'netdev_del' succeeds, but 'remove-fd' does not?
>
> If we return an error from this function, the caller will interpret that
> as if we failed to remove the network device. But qemu has already
> removed the netdev. So things are in an inconsistent state.
>
> TYPE_VHOSTUSER just carries on without even printing a warning if the
> chardev can't be removed. So I did something similar here for vDPA, but
> added a warning. I'm not sure that there's really a "good" solution
> here.
>
> Regarding the possibility of a second guest attempting to use the vdpa
> device that was unsuccessfully removed: I have only tested with the
> vdpa_sim kernel module, but if the fd is not closed, attempting to
> re-use it with a different guest fails like this:
>
> error: Failed to attach device from vdpa.xml
> error: Unable to open '/dev/vhost-vdpa-0' for vdpa device: Device or
> resource busy


Okay, based on that explanation, I think your solution is as good as, or 
better than, any other.