On 01/21/2019 10:48 AM, Andrea Bolognani wrote: > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > [...] >> @@ -893,7 +893,9 @@ VIR_ENUM_IMPL(virDomainIOMMUModel, VIR_DOMAIN_IOMMU_MODEL_LAST, >> >> VIR_ENUM_IMPL(virDomainVsockModel, VIR_DOMAIN_VSOCK_MODEL_LAST, >> "default", >> - "virtio") >> + "virtio", >> + "virtio-transitional", >> + "virtio-non-transitional") > > Same comment as always for VIR_ENUM_IMPL(). > > [...] >> @@ -1136,6 +1140,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { >> {"virtio-9p-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_9P_NON_TRANSITIONAL}, >> {"virtio-balloon-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BALLOON_TRANSITIONAL}, >> {"virtio-balloon-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BALLOON_NON_TRANSITIONAL}, >> + {"vhost-vsock-pci-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL}, >> + {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL}, >> }; > > Same comment as always for capabilities. > > [...] >> @@ -495,6 +495,12 @@ qemuBuildVirtioTransitional(virBufferPtr buf, >> tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BALLOON_TRANSITIONAL; >> ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BALLOON_NON_TRANSITIONAL; >> break; >> + case VIR_DOMAIN_DEVICE_VSOCK: >> + has_tmodel = model == VIR_DOMAIN_VSOCK_MODEL_VIRTIO_TRANSITIONAL; >> + has_ntmodel = model == VIR_DOMAIN_VSOCK_MODEL_VIRTIO_NON_TRANSITIONAL; >> + tmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL; >> + ntmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL; >> + break; >> >> case VIR_DOMAIN_DEVICE_LEASE: >> case VIR_DOMAIN_DEVICE_INPUT: > > Sorry I didn't point this out until now, but having an empty line > after each 'break;' would be nice from the visual point of view... > Will do > [...] >> @@ -10471,11 +10476,11 @@ qemuBuildVsockDevStr(virDomainDefPtr def, >> virBuffer buf = VIR_BUFFER_INITIALIZER; >> char *ret = NULL; >> >> - if (vsock->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { >> - virBufferAddLit(&buf, "vhost-vsock-ccw"); >> - } else { >> - virBufferAddLit(&buf, "vhost-vsock-pci"); >> - } >> + if (qemuBuildVirtioTransitional(&buf, "vhost-vsock", qemuCaps, >> + vsock->info.type, >> + vsock->model, NULL, >> + VIR_DOMAIN_DEVICE_VSOCK) < 0) >> + goto cleanup; > > Similar comment as vhost-scsi in 7/18: you should make sure this is > actually safe, and either way move to qemuBuildVirtioDevStr() first > and only then to qemuBuildVirtioTransitional(). > I'll split this out too. > [...] >> +++ b/tests/qemuxml2xmltest.c >> @@ -1269,12 +1269,14 @@ mymain(void) >> QEMU_CAPS_DEVICE_VIDEO_PRIMARY, >> QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, >> QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, >> - QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY); >> + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, >> + QEMU_CAPS_DEVICE_VHOST_VSOCK); >> DO_TEST("virtio-non-transitional", >> QEMU_CAPS_DEVICE_VIDEO_PRIMARY, >> QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, >> QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, >> - QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY); >> + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, >> + QEMU_CAPS_DEVICE_VHOST_VSOCK); > > This could have gone into patch 2/18. > Seems a little strange since it only became relevant with this patch, but I'll change it - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list