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@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@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