[libvirt PATCH v4 5/6] qemu: support hotplug of vdpa devices
Laine Stump
laine at redhat.com
Tue Sep 29 19:53:39 UTC 2020
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 at 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 at redhat.com>
More information about the libvir-list
mailing list