Re: [libvirt] [PATCHv2 7/7] qemu: add support for vhost-vsock-pci

On Thu, May 24, 2018 at 12:39:15 +0200, Ján Tomko wrote: > Create a new vsock endpoint by opening /dev/vhost-vsock, > set the requested CID via ioctl (or assign a free one if auto='yes'), > pass the file descriptor to QEMU and build the command line. > > https://bugzilla.redhat.com/show_bug.cgi?id=1291851 > Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; > --- > src/qemu/qemu_alias.c | 16 ++++++++ > src/qemu/qemu_command.c | 45 ++++++++++++++++++++++ > src/qemu/qemu_domain.c | 5 +++ > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_process.c | 35 +++++++++++++++++ > .../vhost-vsock-auto.x86_64-latest.args | 32 +++++++++++++++ > .../vhost-vsock.x86_64-latest.args | 32 +++++++++++++++ > tests/qemuxml2argvtest.c | 14 +++++++ > 8 files changed, 180 insertions(+), 1 deletion(-) > create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/vhost-vsock.x86_64-latest.args [...] > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 9da2d609e8..ef0716d683 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -9865,6 +9865,47 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, > } > > > +static int > +qemuBuildVsockCommandLine(virCommandPtr cmd, > + virDomainDefPtr def, > + virDomainVsockDefPtr vsock, > + const char *fdprefix, fdprefix is always empty string, so why is it necessary? [1]

> + virQEMUCapsPtr qemuCaps) > +{ > + qemuDomainVsockPrivatePtr priv = (qemuDomainVsockPrivatePtr)vsock->privateData; > + const char *device = "vhost-vsock-pci"; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + char *devstr = NULL; > + int ret = -1; > + > + virBufferAsprintf(&buf, "%s", device); > + virBufferAsprintf(&buf, ",id=%s", vsock->info.alias); > + virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid); > + virBufferAsprintf(&buf, ",vhostfd=%s%u", fdprefix, priv->vhostfd); > + if (qemuBuildDeviceAddressStr(&buf, def, &vsock->info, qemuCaps) < 0) > + goto cleanup; > +#if 0 > + if (qemuBuildVirtioOptionsStr(&buf, net->virtio, qemuCaps) < 0) > + goto error; > +#endif Leftover stuff from previous version?

> + > + if (virBufferCheckError(&buf) < 0) > + goto cleanup; > + > + devstr = virBufferContentAndReset(&buf); > + > + virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); > + priv->vhostfd = -1; > + virCommandAddArgList(cmd, "-device", devstr, NULL); > + > + ret = 0; > + cleanup: > + virBufferFreeAndReset(&buf); > + VIR_FREE(devstr); > + return ret; > +} > + > + > /* > * Constructs a argv suitable for launching qemu with config defined > * for a given virtual machine. > @@ -10111,6 +10152,10 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, > goto error; > } > > + if (def->vsock && > + qemuBuildVsockCommandLine(cmd, def, def->vsock, "", qemuCaps) < 0) [1] > + goto error; > + > /* In some situations, eg. VFIO passthrough, QEMU might need to lock a > * significant amount of memory, so we need to set the limit accordingly */ > virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def)); [...] > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 174d932ae7..e318639963 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c [...] > @@ -5975,6 +5976,36 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, > } > > > +static int > +qemuProcessOpenVhostVsock(virDomainVsockDefPtr vsock) > +{ > + qemuDomainVsockPrivatePtr priv = (qemuDomainVsockPrivatePtr)vsock->privateData; > + const char *vsock_path = "/dev/vhost-vsock"; > + int fd; > + > + if ((fd = open(vsock_path, O_RDWR)) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + "%s", _("unable to open vhost-vsock device")); > + return -1; > + } > + > + if (vsock->guest_cid) { > + if (virVsockSetGuestCid(fd, vsock->guest_cid) < 0) > + goto error; > + } else { > + if (virVsockAcquireGuestCid(fd, &vsock->guest_cid) < 0) > + goto error; This logic is wrong. You should base the decision on auto_cid, rather than presence of guest_cid. If automatic cid is requested you should always honour it. Specifically this might create problem with migration as the cid that was used on the source was already taken, but the user is okay with autogenerating it. Or if the cid actually can't change, you should make it part of the ABI stability check.