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

[Ján Tomko]

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 at redhat.com>
>> ---
>>  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180529/a3cd2573/attachment-0001.sig>