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

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Tue, May 29, 2018 at 11:16:40AM +0200, Peter Krempa wrote:
> 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@xxxxxxxxxx>
> > ---
> >  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]

It will be necessary for hotplug, because AFAIK the name of the file
descriptor passed via monitor cannot start with a digit.

It has no place in this patch.

> > +                          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?

While ats= is not that useful for every device, I'm afraid
iommu_platform might be and a followup implementing it will be required.

> > +
> > +    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.

I don't think migrating the device would make sense if you would not be
able to change the CIDs.

Jano

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list