Re: [PATCH 1/2] qemu: Add ccw support for vhost-vsock

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Wed, Jul 04, 2018 at 01:21:44PM +0200, Boris Fiuczynski wrote:
> Add support and tests for vhost-vsock-ccw.
>
> Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> ---
> src/qemu/qemu_command.c                       | 15 +++++++--
> src/qemu/qemu_domain_address.c                |  7 +++-
> .../vhost-vsock-ccw-auto.args                 | 27 ++++++++++++++++
> .../qemuxml2argvdata/vhost-vsock-ccw-auto.xml | 25 +++++++++++++++
> tests/qemuxml2argvdata/vhost-vsock-ccw.args   | 27 ++++++++++++++++
> tests/qemuxml2argvdata/vhost-vsock-ccw.xml    | 32 +++++++++++++++++++
> tests/qemuxml2argvtest.c                      |  4 +++
> .../vhost-vsock-ccw-auto.xml                  | 32 +++++++++++++++++++
> tests/qemuxml2xmloutdata/vhost-vsock-ccw.xml  |  1 +
> tests/qemuxml2xmltest.c                       |  5 +++
> 10 files changed, 172 insertions(+), 3 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw-auto.args
> create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw-auto.xml
> create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw.args
> create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw.xml
> create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-ccw-auto.xml
> create mode 120000 tests/qemuxml2xmloutdata/vhost-vsock-ccw.xml
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 04c5c28438..ab6944f68e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10089,10 +10089,21 @@ qemuBuildVsockDevStr(virDomainDefPtr def,
> {
>     qemuDomainVsockPrivatePtr priv = (qemuDomainVsockPrivatePtr)vsock->privateData;
>     virBuffer buf = VIR_BUFFER_INITIALIZER;
> -    const char *device = "vhost-vsock-pci";
>     char *ret = NULL;
> +    const char *devsuffix;
>
> -    virBufferAsprintf(&buf, "%s", device);
> +    if (vsock->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +        devsuffix = "pci";
> +    } else if (vsock->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> +        devsuffix = "ccw";
> +    } else {

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unsupported address type %s for vsock device"),
> +                       virDomainDeviceAddressTypeToString(vsock->info.type));
> +        goto cleanup;
> +    }

This check belongs in qemuDomainDeviceDefValidateVsock

> +
> +    virBufferAsprintf(&buf, "vhost-vsock-%s", devsuffix);

Having the whole device name in one string would be nicer than dealing
with the device suffix, e.g.:

if (type == _CCW)
   device = "vhost-vsock-ccw"
else
   device = "vhost-vsock-pci"

(or even virBufferAddLit, without the temporary variable - I forgot what
led me to use it)

>     virBufferAsprintf(&buf, ",id=%s", vsock->info.alias);
>     virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid);
>     virBufferAsprintf(&buf, ",vhostfd=%s%u", fdprefix, priv->vhostfd);
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index eb11a660d7..9639595175 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -306,7 +306,8 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>        declare address-less virtio devices to be of address type 'type'
>        disks, networks, videos, consoles, controllers, memballoon and rng
>        in this order
> -       if type is ccw filesystem devices are declared to be of address type ccw
> +       if type is ccw filesystem and vsock devices are declared to be of
> +       address type ccw
>     */

This whole comment is pointless - it's repeating what the function does,
not why.

>     size_t i;

[...]

> +  <devices>
> +    <emulator>/usr/bin/qemu-system-s390x</emulator>
> +    <disk type='block' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='virtio'/>
> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
> +    </disk>
> +    <memballoon model='virtio'>
> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/>
> +    </memballoon>
> +    <panic model='s390'/>
> +    <vsock model='virtio'>
> +      <cid auto='no' address='4'/>
> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0003'/>
> +    </vsock>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d6911f9344..60bd9585e5 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2914,6 +2914,10 @@ mymain(void)
>
>     DO_TEST_CAPS_LATEST("vhost-vsock");
>     DO_TEST_CAPS_LATEST("vhost-vsock-auto");
> +    DO_TEST("vhost-vsock-ccw", QEMU_CAPS_DEVICE_VHOST_VSOCK,
> +            QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
> +    DO_TEST("vhost-vsock-ccw-auto", QEMU_CAPS_DEVICE_VHOST_VSOCK,
> +            QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);

It would be nice to extend the LATEST caps coverage to s390 as well, to
make sure that adding a new capability won't affect the vsock device.

Jano

Attachment: signature.asc
Description: Digital signature

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