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

[Boris Fiuczynski]

On 07/10/2018 01:37 PM, Ján Tomko wrote:
> 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 at linux.ibm.com>
>> ---
>> 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
Changed it.

> 
>> +
>> +    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)
Changed 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.
Well I did not create the original comment but adjusted it for 
completeness. Since the function name is not really speaking for itself 
I kind of understand the intend. Do you suggest that I should delete the 
comment completely?

> 
>>     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.
I agree and John already made me aware of that. Have not yet gotten to 
it but soon hope to. Can we leave it as is for now and change it when 
LATEST caps has been extended to s390?

> 
> Jano
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294