* [Qemu-devel] [PATCH for-4.0 v4 1/2] virtio: Helper for registering virtio device types
2018-12-05 19:57 [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
@ 2018-12-05 19:57 ` Eduardo Habkost
2018-12-05 19:57 ` [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Eduardo Habkost @ 2018-12-05 19:57 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin
Cc: Gonglei, Paolo Bonzini, Stefan Hajnoczi, Amit Shah,
Andrea Bolognani, Cleber Rosa, Marcel Apfelbaum, Fam Zheng,
Cornelia Huck, Kevin Wolf, Max Reitz, libvir-list,
Wainer dos Santos Moschetta, Philippe Mathieu-Daudé,
Markus Armbruster, Laine Stump, Jason Wang, Gerd Hoffmann,
Daniel P. Berrangé,
Caio Carrara
Introduce a helper for registering different flavours of virtio
devices. Convert code to use the helper, but keep only the
existing generic types. Transitional and non-transitional device
types will be added by another patch.
Acked-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v3 -> v4:
* Fix typos on comments (Cornelia Huck)
Changes v2 -> v3:
* Split into two separate patches (type registration helper
and introduction of new types)
* Rewrote comments describing each flavour
* Rewrote virtio_pci_types_register() completely:
* Replaced magic generation of type names with explicit fields in
VirtioPCIDeviceTypeInfo
* Removed modern_only field (won't be used anymore)
* Don't register a separate base type unless it's required by
the caller
---
hw/virtio/virtio-pci.h | 54 ++++++++
hw/display/virtio-gpu-pci.c | 7 +-
hw/display/virtio-vga.c | 7 +-
hw/virtio/virtio-crypto-pci.c | 7 +-
hw/virtio/virtio-pci.c | 231 ++++++++++++++++++++++++----------
5 files changed, 228 insertions(+), 78 deletions(-)
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 813082b0d7..8cd546608e 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -417,4 +417,58 @@ struct VirtIOCryptoPCI {
/* Virtio ABI version, if we increment this, we break the guest driver. */
#define VIRTIO_PCI_ABI_VERSION 0
+/* Input for virtio_pci_types_register() */
+typedef struct VirtioPCIDeviceTypeInfo {
+ /*
+ * Common base class for the subclasses below.
+ *
+ * Required only if transitional_name or non_transitional_name is set.
+ *
+ * We need a separate base type instead of making all types
+ * inherit from generic_name for two reasons:
+ * 1) generic_name implements INTERFACE_PCIE_DEVICE, but
+ * transitional_name does not.
+ * 2) generic_name has the "disable-legacy" and "disable-modern"
+ * properties, transitional_name and non_transitional name don't.
+ */
+ const char *base_name;
+ /*
+ * Generic device type. Optional.
+ *
+ * Supports both transitional and non-transitional modes,
+ * using the disable-legacy and disable-modern properties.
+ * If disable-legacy=auto, (non-)transitional mode is selected
+ * depending on the bus where the device is plugged.
+ *
+ * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE,
+ * but PCI Express is supported only in non-transitional mode.
+ *
+ * The only type implemented by QEMU 3.1 and older.
+ */
+ const char *generic_name;
+ /*
+ * The transitional device type. Optional.
+ *
+ * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE.
+ */
+ const char *transitional_name;
+ /*
+ * The non-transitional device type. Optional.
+ *
+ * Implements INTERFACE_CONVENTIONAL_PCI_DEVICE only.
+ */
+ const char *non_transitional_name;
+
+ /* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
+ const char *parent;
+
+ /* Same as TypeInfo fields: */
+ size_t instance_size;
+ void (*instance_init)(Object *obj);
+ void (*class_init)(ObjectClass *klass, void *data);
+} VirtioPCIDeviceTypeInfo;
+
+/* Register virtio-pci type(s). @t must be static. */
+void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
+
#endif
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index cece4aa495..faf76a8bc4 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -69,9 +69,8 @@ static void virtio_gpu_initfn(Object *obj)
TYPE_VIRTIO_GPU);
}
-static const TypeInfo virtio_gpu_pci_info = {
- .name = TYPE_VIRTIO_GPU_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_gpu_pci_info = {
+ .generic_name = TYPE_VIRTIO_GPU_PCI,
.instance_size = sizeof(VirtIOGPUPCI),
.instance_init = virtio_gpu_initfn,
.class_init = virtio_gpu_pci_class_init,
@@ -79,6 +78,6 @@ static const TypeInfo virtio_gpu_pci_info = {
static void virtio_gpu_pci_register_types(void)
{
- type_register_static(&virtio_gpu_pci_info);
+ virtio_pci_types_register(&virtio_gpu_pci_info);
}
type_init(virtio_gpu_pci_register_types)
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index ab2e369b28..8db4d916f2 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -207,9 +207,8 @@ static void virtio_vga_inst_initfn(Object *obj)
TYPE_VIRTIO_GPU);
}
-static TypeInfo virtio_vga_info = {
- .name = TYPE_VIRTIO_VGA,
- .parent = TYPE_VIRTIO_PCI,
+static VirtioPCIDeviceTypeInfo virtio_vga_info = {
+ .generic_name = TYPE_VIRTIO_VGA,
.instance_size = sizeof(struct VirtIOVGA),
.instance_init = virtio_vga_inst_initfn,
.class_init = virtio_vga_class_init,
@@ -217,7 +216,7 @@ static TypeInfo virtio_vga_info = {
static void virtio_vga_register_types(void)
{
- type_register_static(&virtio_vga_info);
+ virtio_pci_types_register(&virtio_vga_info);
}
type_init(virtio_vga_register_types)
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index bf64996e48..8cc3fa3ef7 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -64,9 +64,8 @@ static void virtio_crypto_initfn(Object *obj)
TYPE_VIRTIO_CRYPTO);
}
-static const TypeInfo virtio_crypto_pci_info = {
- .name = TYPE_VIRTIO_CRYPTO_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_crypto_pci_info = {
+ .generic_name = TYPE_VIRTIO_CRYPTO_PCI,
.instance_size = sizeof(VirtIOCryptoPCI),
.instance_init = virtio_crypto_initfn,
.class_init = virtio_crypto_pci_class_init,
@@ -74,6 +73,6 @@ static const TypeInfo virtio_crypto_pci_info = {
static void virtio_crypto_pci_register_types(void)
{
- type_register_static(&virtio_crypto_pci_info);
+ virtio_pci_types_register(&virtio_crypto_pci_info);
}
type_init(virtio_crypto_pci_register_types)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index a954799267..f07ec55c38 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1119,9 +1119,8 @@ static void virtio_9p_pci_instance_init(Object *obj)
TYPE_VIRTIO_9P);
}
-static const TypeInfo virtio_9p_pci_info = {
- .name = TYPE_VIRTIO_9P_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_9p_pci_info = {
+ .generic_name = TYPE_VIRTIO_9P_PCI,
.instance_size = sizeof(V9fsPCIState),
.instance_init = virtio_9p_pci_instance_init,
.class_init = virtio_9p_pci_class_init,
@@ -1877,9 +1876,6 @@ static void virtio_pci_reset(DeviceState *qdev)
static Property virtio_pci_properties[] = {
DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags,
VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
- DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy,
- ON_OFF_AUTO_AUTO),
- DEFINE_PROP_BOOL("disable-modern", VirtIOPCIProxy, disable_modern, false),
DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags,
VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true),
DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags,
@@ -1939,13 +1935,123 @@ static const TypeInfo virtio_pci_info = {
.class_init = virtio_pci_class_init,
.class_size = sizeof(VirtioPCIClass),
.abstract = true,
- .interfaces = (InterfaceInfo[]) {
- { INTERFACE_PCIE_DEVICE },
- { INTERFACE_CONVENTIONAL_PCI_DEVICE },
- { }
- },
};
+static Property virtio_pci_generic_properties[] = {
+ DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy,
+ ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_BOOL("disable-modern", VirtIOPCIProxy, disable_modern, false),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_pci_base_class_init(ObjectClass *klass, void *data)
+{
+ const VirtioPCIDeviceTypeInfo *t = data;
+ if (t->class_init) {
+ t->class_init(klass, NULL);
+ }
+}
+
+static void virtio_pci_generic_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->props = virtio_pci_generic_properties;
+}
+
+/* Used when the generic type and the base type is the same */
+static void virtio_pci_generic_base_class_init(ObjectClass *klass, void *data)
+{
+ virtio_pci_base_class_init(klass, data);
+ virtio_pci_generic_class_init(klass, NULL);
+}
+
+static void virtio_pci_transitional_instance_init(Object *obj)
+{
+ VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
+
+ proxy->disable_legacy = ON_OFF_AUTO_OFF;
+ proxy->disable_modern = false;
+}
+
+static void virtio_pci_non_transitional_instance_init(Object *obj)
+{
+ VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
+
+ proxy->disable_legacy = ON_OFF_AUTO_ON;
+ proxy->disable_modern = false;
+}
+
+void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
+{
+ TypeInfo base_type_info = {
+ .name = t->base_name,
+ .parent = t->parent ? t->parent : TYPE_VIRTIO_PCI,
+ .instance_size = t->instance_size,
+ .instance_init = t->instance_init,
+ .class_init = virtio_pci_base_class_init,
+ .class_data = (void *)t,
+ .abstract = true,
+ };
+ TypeInfo generic_type_info = {
+ .name = t->generic_name,
+ .parent = base_type_info.name,
+ .class_init = virtio_pci_generic_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_PCIE_DEVICE },
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ },
+ };
+
+ if (!base_type_info.name) {
+ /* No base type -> register a single generic device type */
+ base_type_info.name = t->generic_name;
+ base_type_info.class_init = virtio_pci_generic_base_class_init;
+ base_type_info.interfaces = generic_type_info.interfaces;
+ base_type_info.abstract = false;
+ generic_type_info.name = NULL;
+ assert(!t->non_transitional_name);
+ assert(!t->transitional_name);
+ }
+
+ type_register(&base_type_info);
+ if (generic_type_info.name) {
+ type_register(&generic_type_info);
+ }
+
+ if (t->non_transitional_name) {
+ const TypeInfo non_transitional_type_info = {
+ .name = t->non_transitional_name,
+ .parent = base_type_info.name,
+ .instance_init = virtio_pci_non_transitional_instance_init,
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_PCIE_DEVICE },
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ },
+ };
+ type_register(&non_transitional_type_info);
+ }
+
+ if (t->transitional_name) {
+ const TypeInfo transitional_type_info = {
+ .name = t->transitional_name,
+ .parent = base_type_info.name,
+ .instance_init = virtio_pci_transitional_instance_init,
+ .interfaces = (InterfaceInfo[]) {
+ /*
+ * Transitional virtio devices work only as Conventional PCI
+ * devices because they require PIO ports.
+ */
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ },
+ };
+ type_register(&transitional_type_info);
+ }
+}
+
/* virtio-blk-pci */
static Property virtio_blk_pci_properties[] = {
@@ -1995,9 +2101,8 @@ static void virtio_blk_pci_instance_init(Object *obj)
"bootindex", &error_abort);
}
-static const TypeInfo virtio_blk_pci_info = {
- .name = TYPE_VIRTIO_BLK_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_blk_pci_info = {
+ .generic_name = TYPE_VIRTIO_BLK_PCI,
.instance_size = sizeof(VirtIOBlkPCI),
.instance_init = virtio_blk_pci_instance_init,
.class_init = virtio_blk_pci_class_init,
@@ -2051,9 +2156,8 @@ static void vhost_user_blk_pci_instance_init(Object *obj)
"bootindex", &error_abort);
}
-static const TypeInfo vhost_user_blk_pci_info = {
- .name = TYPE_VHOST_USER_BLK_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo vhost_user_blk_pci_info = {
+ .generic_name = TYPE_VHOST_USER_BLK_PCI,
.instance_size = sizeof(VHostUserBlkPCI),
.instance_init = vhost_user_blk_pci_instance_init,
.class_init = vhost_user_blk_pci_class_init,
@@ -2119,9 +2223,8 @@ static void virtio_scsi_pci_instance_init(Object *obj)
TYPE_VIRTIO_SCSI);
}
-static const TypeInfo virtio_scsi_pci_info = {
- .name = TYPE_VIRTIO_SCSI_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_scsi_pci_info = {
+ .generic_name = TYPE_VIRTIO_SCSI_PCI,
.instance_size = sizeof(VirtIOSCSIPCI),
.instance_init = virtio_scsi_pci_instance_init,
.class_init = virtio_scsi_pci_class_init,
@@ -2174,9 +2277,8 @@ static void vhost_scsi_pci_instance_init(Object *obj)
"bootindex", &error_abort);
}
-static const TypeInfo vhost_scsi_pci_info = {
- .name = TYPE_VHOST_SCSI_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo vhost_scsi_pci_info = {
+ .generic_name = TYPE_VHOST_SCSI_PCI,
.instance_size = sizeof(VHostSCSIPCI),
.instance_init = vhost_scsi_pci_instance_init,
.class_init = vhost_scsi_pci_class_init,
@@ -2229,9 +2331,8 @@ static void vhost_user_scsi_pci_instance_init(Object *obj)
"bootindex", &error_abort);
}
-static const TypeInfo vhost_user_scsi_pci_info = {
- .name = TYPE_VHOST_USER_SCSI_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo vhost_user_scsi_pci_info = {
+ .generic_name = TYPE_VHOST_USER_SCSI_PCI,
.instance_size = sizeof(VHostUserSCSIPCI),
.instance_init = vhost_user_scsi_pci_instance_init,
.class_init = vhost_user_scsi_pci_class_init,
@@ -2277,9 +2378,8 @@ static void vhost_vsock_pci_instance_init(Object *obj)
TYPE_VHOST_VSOCK);
}
-static const TypeInfo vhost_vsock_pci_info = {
- .name = TYPE_VHOST_VSOCK_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
+ .generic_name = TYPE_VHOST_VSOCK_PCI,
.instance_size = sizeof(VHostVSockPCI),
.instance_init = vhost_vsock_pci_instance_init,
.class_init = vhost_vsock_pci_class_init,
@@ -2334,9 +2434,8 @@ static void virtio_balloon_pci_instance_init(Object *obj)
"guest-stats-polling-interval", &error_abort);
}
-static const TypeInfo virtio_balloon_pci_info = {
- .name = TYPE_VIRTIO_BALLOON_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_balloon_pci_info = {
+ .generic_name = TYPE_VIRTIO_BALLOON_PCI,
.instance_size = sizeof(VirtIOBalloonPCI),
.instance_init = virtio_balloon_pci_instance_init,
.class_init = virtio_balloon_pci_class_init,
@@ -2407,9 +2506,8 @@ static void virtio_serial_pci_instance_init(Object *obj)
TYPE_VIRTIO_SERIAL);
}
-static const TypeInfo virtio_serial_pci_info = {
- .name = TYPE_VIRTIO_SERIAL_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_serial_pci_info = {
+ .generic_name = TYPE_VIRTIO_SERIAL_PCI,
.instance_size = sizeof(VirtIOSerialPCI),
.instance_init = virtio_serial_pci_instance_init,
.class_init = virtio_serial_pci_class_init,
@@ -2462,9 +2560,8 @@ static void virtio_net_pci_instance_init(Object *obj)
"bootindex", &error_abort);
}
-static const TypeInfo virtio_net_pci_info = {
- .name = TYPE_VIRTIO_NET_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_net_pci_info = {
+ .generic_name = TYPE_VIRTIO_NET_PCI,
.instance_size = sizeof(VirtIONetPCI),
.instance_init = virtio_net_pci_instance_init,
.class_init = virtio_net_pci_class_init,
@@ -2513,9 +2610,8 @@ static void virtio_rng_initfn(Object *obj)
TYPE_VIRTIO_RNG);
}
-static const TypeInfo virtio_rng_pci_info = {
- .name = TYPE_VIRTIO_RNG_PCI,
- .parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_rng_pci_info = {
+ .generic_name = TYPE_VIRTIO_RNG_PCI,
.instance_size = sizeof(VirtIORngPCI),
.instance_init = virtio_rng_initfn,
.class_init = virtio_rng_pci_class_init,
@@ -2605,24 +2701,24 @@ static const TypeInfo virtio_input_hid_pci_info = {
.abstract = true,
};
-static const TypeInfo virtio_keyboard_pci_info = {
- .name = TYPE_VIRTIO_KEYBOARD_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_keyboard_pci_info = {
+ .generic_name = TYPE_VIRTIO_KEYBOARD_PCI,
.parent = TYPE_VIRTIO_INPUT_HID_PCI,
.class_init = virtio_input_hid_kbd_pci_class_init,
.instance_size = sizeof(VirtIOInputHIDPCI),
.instance_init = virtio_keyboard_initfn,
};
-static const TypeInfo virtio_mouse_pci_info = {
- .name = TYPE_VIRTIO_MOUSE_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_mouse_pci_info = {
+ .generic_name = TYPE_VIRTIO_MOUSE_PCI,
.parent = TYPE_VIRTIO_INPUT_HID_PCI,
.class_init = virtio_input_hid_mouse_pci_class_init,
.instance_size = sizeof(VirtIOInputHIDPCI),
.instance_init = virtio_mouse_initfn,
};
-static const TypeInfo virtio_tablet_pci_info = {
- .name = TYPE_VIRTIO_TABLET_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_tablet_pci_info = {
+ .generic_name = TYPE_VIRTIO_TABLET_PCI,
.parent = TYPE_VIRTIO_INPUT_HID_PCI,
.instance_size = sizeof(VirtIOInputHIDPCI),
.instance_init = virtio_tablet_initfn,
@@ -2637,8 +2733,8 @@ static void virtio_host_initfn(Object *obj)
TYPE_VIRTIO_INPUT_HOST);
}
-static const TypeInfo virtio_host_pci_info = {
- .name = TYPE_VIRTIO_INPUT_HOST_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_host_pci_info = {
+ .generic_name = TYPE_VIRTIO_INPUT_HOST_PCI,
.parent = TYPE_VIRTIO_INPUT_PCI,
.instance_size = sizeof(VirtIOInputHostPCI),
.instance_init = virtio_host_initfn,
@@ -2692,36 +2788,39 @@ static const TypeInfo virtio_pci_bus_info = {
static void virtio_pci_register_types(void)
{
- type_register_static(&virtio_rng_pci_info);
+ /* Base types: */
+ type_register_static(&virtio_pci_bus_info);
+ type_register_static(&virtio_pci_info);
type_register_static(&virtio_input_pci_info);
type_register_static(&virtio_input_hid_pci_info);
- type_register_static(&virtio_keyboard_pci_info);
- type_register_static(&virtio_mouse_pci_info);
- type_register_static(&virtio_tablet_pci_info);
+
+ /* Implementations: */
+ virtio_pci_types_register(&virtio_rng_pci_info);
+ virtio_pci_types_register(&virtio_keyboard_pci_info);
+ virtio_pci_types_register(&virtio_mouse_pci_info);
+ virtio_pci_types_register(&virtio_tablet_pci_info);
#ifdef CONFIG_LINUX
- type_register_static(&virtio_host_pci_info);
+ virtio_pci_types_register(&virtio_host_pci_info);
#endif
- type_register_static(&virtio_pci_bus_info);
- type_register_static(&virtio_pci_info);
#ifdef CONFIG_VIRTFS
- type_register_static(&virtio_9p_pci_info);
+ virtio_pci_types_register(&virtio_9p_pci_info);
#endif
- type_register_static(&virtio_blk_pci_info);
+ virtio_pci_types_register(&virtio_blk_pci_info);
#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
- type_register_static(&vhost_user_blk_pci_info);
+ virtio_pci_types_register(&vhost_user_blk_pci_info);
#endif
- type_register_static(&virtio_scsi_pci_info);
- type_register_static(&virtio_balloon_pci_info);
- type_register_static(&virtio_serial_pci_info);
- type_register_static(&virtio_net_pci_info);
+ virtio_pci_types_register(&virtio_scsi_pci_info);
+ virtio_pci_types_register(&virtio_balloon_pci_info);
+ virtio_pci_types_register(&virtio_serial_pci_info);
+ virtio_pci_types_register(&virtio_net_pci_info);
#ifdef CONFIG_VHOST_SCSI
- type_register_static(&vhost_scsi_pci_info);
+ virtio_pci_types_register(&vhost_scsi_pci_info);
#endif
#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
- type_register_static(&vhost_user_scsi_pci_info);
+ virtio_pci_types_register(&vhost_user_scsi_pci_info);
#endif
#ifdef CONFIG_VHOST_VSOCK
- type_register_static(&vhost_vsock_pci_info);
+ virtio_pci_types_register(&vhost_vsock_pci_info);
#endif
}
--
2.18.0.rc1.1.g3f1ff2140
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-05 19:57 [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
2018-12-05 19:57 ` [Qemu-devel] [PATCH for-4.0 v4 1/2] virtio: Helper for registering virtio device types Eduardo Habkost
@ 2018-12-05 19:57 ` Eduardo Habkost
2018-12-07 12:03 ` Caio Carrara
2019-01-03 9:38 ` Thomas Huth
2018-12-05 21:42 ` [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] " no-reply
` (2 subsequent siblings)
4 siblings, 2 replies; 20+ messages in thread
From: Eduardo Habkost @ 2018-12-05 19:57 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin
Cc: Gonglei, Paolo Bonzini, Stefan Hajnoczi, Amit Shah,
Andrea Bolognani, Cleber Rosa, Marcel Apfelbaum, Fam Zheng,
Cornelia Huck, Kevin Wolf, Max Reitz, libvir-list,
Wainer dos Santos Moschetta, Philippe Mathieu-Daudé,
Markus Armbruster, Laine Stump, Jason Wang, Gerd Hoffmann,
Daniel P. Berrangé,
Caio Carrara
Many of the current virtio-*-pci device types actually represent
3 different types of devices:
* virtio 1.0 non-transitional devices
* virtio 1.0 transitional devices
* virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus
compatibility QMP interfaces. With these multi-purpose device
types, there's no way to tell management software that
transitional devices and legacy devices require a Conventional
PCI bus.
The multi-purpose device types would also prevent us from telling
management software what's the PCI vendor/device ID for them,
because their PCI IDs change at runtime depending on the bus
where they were plugged.
This patch adds separate device types for each of those virtio
device flavors:
- virtio-*-pci: the existing multi-purpose device types
- Configurable using `disable-legacy` and `disable-modern`
properties
- Legacy driver support is automatically enabled/disabled
depending on the bus where it is plugged
- Supports Conventional PCI and PCI Express buses
(but Conventional PCI is incompatible with
disable-legacy=off)
- Changes PCI vendor/device IDs at runtime
- virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
- Supports Conventional PCI buses only, because
it has a PIO BAR
- virtio-*-pci-non-transitional: modern-only
- Supports both Conventional PCI and PCI Express buses
The existing TYPE_* macros for these types will point to an
abstract base type, so existing casts in the code will keep
working for all variants.
A simple test script (tests/acceptance/virtio_version.py) is
included, to check if the new device types are equivalent to
using the `disable-legacy` and `disable-modern` options.
Acked-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v3 -> v4:
* Only test code changes (Caio Carrara)
* Rename get_interfaces() to get_pci_interfaces()
* Use tuple instead of list at get_pci_interfaces()
* Spaces after commas
Changes v2 -> v3:
* Split into two separate patches (type registration helper
and introduction of new types)
* Include full type names as literals in the code instead
of generating type names automatically
Changes v1 -> v2:
* Removed *-0.9 devices. Nobody will want to use them, if
transitional devices work with legacy drivers
(Gerd Hoffmann, Michael S. Tsirkin)
* Drop virtio version from name: rename -1.0-transitional to
-transitional (Michael S. Tsirkin)
* Renamed -1.0 to -non-transitional
* Don't add any extra variants to modern-only device types
(they don't need it)
* Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
Cornelia Huck)
* No need to change cast macros for modern-only devices
* Rename virtio_register_types() to virtio_pci_types_register()
---
hw/virtio/virtio-pci.h | 24 ++--
hw/virtio/virtio-pci.c | 60 ++++++++--
tests/acceptance/virtio_version.py | 176 +++++++++++++++++++++++++++++
3 files changed, 236 insertions(+), 24 deletions(-)
create mode 100644 tests/acceptance/virtio_version.py
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 8cd546608e..29b4216107 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -216,7 +216,7 @@ static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
/*
* virtio-scsi-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci"
+#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci-base"
#define VIRTIO_SCSI_PCI(obj) \
OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
@@ -229,7 +229,7 @@ struct VirtIOSCSIPCI {
/*
* vhost-scsi-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
+#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci-base"
#define VHOST_SCSI_PCI(obj) \
OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
@@ -239,7 +239,7 @@ struct VHostSCSIPCI {
};
#endif
-#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
+#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci-base"
#define VHOST_USER_SCSI_PCI(obj) \
OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
@@ -252,7 +252,7 @@ struct VHostUserSCSIPCI {
/*
* vhost-user-blk-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci"
+#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci-base"
#define VHOST_USER_BLK_PCI(obj) \
OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
@@ -265,7 +265,7 @@ struct VHostUserBlkPCI {
/*
* virtio-blk-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
+#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci-base"
#define VIRTIO_BLK_PCI(obj) \
OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
@@ -277,7 +277,7 @@ struct VirtIOBlkPCI {
/*
* virtio-balloon-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci"
+#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci-base"
#define VIRTIO_BALLOON_PCI(obj) \
OBJECT_CHECK(VirtIOBalloonPCI, (obj), TYPE_VIRTIO_BALLOON_PCI)
@@ -289,7 +289,7 @@ struct VirtIOBalloonPCI {
/*
* virtio-serial-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci"
+#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci-base"
#define VIRTIO_SERIAL_PCI(obj) \
OBJECT_CHECK(VirtIOSerialPCI, (obj), TYPE_VIRTIO_SERIAL_PCI)
@@ -301,7 +301,7 @@ struct VirtIOSerialPCI {
/*
* virtio-net-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_NET_PCI "virtio-net-pci"
+#define TYPE_VIRTIO_NET_PCI "virtio-net-pci-base"
#define VIRTIO_NET_PCI(obj) \
OBJECT_CHECK(VirtIONetPCI, (obj), TYPE_VIRTIO_NET_PCI)
@@ -316,7 +316,7 @@ struct VirtIONetPCI {
#ifdef CONFIG_VIRTFS
-#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci"
+#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci-base"
#define VIRTIO_9P_PCI(obj) \
OBJECT_CHECK(V9fsPCIState, (obj), TYPE_VIRTIO_9P_PCI)
@@ -330,7 +330,7 @@ typedef struct V9fsPCIState {
/*
* virtio-rng-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci"
+#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci-base"
#define VIRTIO_RNG_PCI(obj) \
OBJECT_CHECK(VirtIORngPCI, (obj), TYPE_VIRTIO_RNG_PCI)
@@ -365,7 +365,7 @@ struct VirtIOInputHIDPCI {
#ifdef CONFIG_LINUX
-#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci"
+#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci-base"
#define VIRTIO_INPUT_HOST_PCI(obj) \
OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
@@ -392,7 +392,7 @@ struct VirtIOGPUPCI {
/*
* vhost-vsock-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci"
+#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci-base"
#define VHOST_VSOCK_PCI(obj) \
OBJECT_CHECK(VHostVSockPCI, (obj), TYPE_VHOST_VSOCK_PCI)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f07ec55c38..d05066deb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1120,7 +1120,10 @@ static void virtio_9p_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_9p_pci_info = {
- .generic_name = TYPE_VIRTIO_9P_PCI,
+ .base_name = TYPE_VIRTIO_9P_PCI,
+ .generic_name = "virtio-9p-pci",
+ .transitional_name = "virtio-9p-pci-transitional",
+ .non_transitional_name = "virtio-9p-pci-non-transitional",
.instance_size = sizeof(V9fsPCIState),
.instance_init = virtio_9p_pci_instance_init,
.class_init = virtio_9p_pci_class_init,
@@ -2102,7 +2105,10 @@ static void virtio_blk_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_blk_pci_info = {
- .generic_name = TYPE_VIRTIO_BLK_PCI,
+ .base_name = TYPE_VIRTIO_BLK_PCI,
+ .generic_name = "virtio-blk-pci",
+ .transitional_name = "virtio-blk-pci-transitional",
+ .non_transitional_name = "virtio-blk-pci-non-transitional",
.instance_size = sizeof(VirtIOBlkPCI),
.instance_init = virtio_blk_pci_instance_init,
.class_init = virtio_blk_pci_class_init,
@@ -2157,7 +2163,10 @@ static void vhost_user_blk_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo vhost_user_blk_pci_info = {
- .generic_name = TYPE_VHOST_USER_BLK_PCI,
+ .base_name = TYPE_VHOST_USER_BLK_PCI,
+ .generic_name = "vhost-user-blk-pci",
+ .transitional_name = "vhost-user-blk-pci-transitional",
+ .non_transitional_name = "vhost-user-blk-pci-non-transitional",
.instance_size = sizeof(VHostUserBlkPCI),
.instance_init = vhost_user_blk_pci_instance_init,
.class_init = vhost_user_blk_pci_class_init,
@@ -2224,7 +2233,10 @@ static void virtio_scsi_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_scsi_pci_info = {
- .generic_name = TYPE_VIRTIO_SCSI_PCI,
+ .base_name = TYPE_VIRTIO_SCSI_PCI,
+ .generic_name = "virtio-scsi-pci",
+ .transitional_name = "virtio-scsi-pci-transitional",
+ .non_transitional_name = "virtio-scsi-pci-non-transitional",
.instance_size = sizeof(VirtIOSCSIPCI),
.instance_init = virtio_scsi_pci_instance_init,
.class_init = virtio_scsi_pci_class_init,
@@ -2278,7 +2290,10 @@ static void vhost_scsi_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo vhost_scsi_pci_info = {
- .generic_name = TYPE_VHOST_SCSI_PCI,
+ .base_name = TYPE_VHOST_SCSI_PCI,
+ .generic_name = "vhost-scsi-pci",
+ .transitional_name = "vhost-scsi-pci-transitional",
+ .non_transitional_name = "vhost-scsi-pci-non-transitional",
.instance_size = sizeof(VHostSCSIPCI),
.instance_init = vhost_scsi_pci_instance_init,
.class_init = vhost_scsi_pci_class_init,
@@ -2332,7 +2347,10 @@ static void vhost_user_scsi_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo vhost_user_scsi_pci_info = {
- .generic_name = TYPE_VHOST_USER_SCSI_PCI,
+ .base_name = TYPE_VHOST_USER_SCSI_PCI,
+ .generic_name = "vhost-user-scsi-pci",
+ .transitional_name = "vhost-user-scsi-pci-transitional",
+ .non_transitional_name = "vhost-user-scsi-pci-non-transitional",
.instance_size = sizeof(VHostUserSCSIPCI),
.instance_init = vhost_user_scsi_pci_instance_init,
.class_init = vhost_user_scsi_pci_class_init,
@@ -2379,7 +2397,10 @@ static void vhost_vsock_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
- .generic_name = TYPE_VHOST_VSOCK_PCI,
+ .base_name = TYPE_VHOST_VSOCK_PCI,
+ .generic_name = "vhost-vsock-pci",
+ .transitional_name = "vhost-vsock-pci-transitional",
+ .non_transitional_name = "vhost-vsock-pci-non-transitional",
.instance_size = sizeof(VHostVSockPCI),
.instance_init = vhost_vsock_pci_instance_init,
.class_init = vhost_vsock_pci_class_init,
@@ -2435,7 +2456,10 @@ static void virtio_balloon_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_balloon_pci_info = {
- .generic_name = TYPE_VIRTIO_BALLOON_PCI,
+ .base_name = TYPE_VIRTIO_BALLOON_PCI,
+ .generic_name = "virtio-balloon-pci",
+ .transitional_name = "virtio-balloon-pci-transitional",
+ .non_transitional_name = "virtio-balloon-pci-non-transitional",
.instance_size = sizeof(VirtIOBalloonPCI),
.instance_init = virtio_balloon_pci_instance_init,
.class_init = virtio_balloon_pci_class_init,
@@ -2507,7 +2531,10 @@ static void virtio_serial_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_serial_pci_info = {
- .generic_name = TYPE_VIRTIO_SERIAL_PCI,
+ .base_name = TYPE_VIRTIO_SERIAL_PCI,
+ .generic_name = "virtio-serial-pci",
+ .transitional_name = "virtio-serial-pci-transitional",
+ .non_transitional_name = "virtio-serial-pci-non-transitional",
.instance_size = sizeof(VirtIOSerialPCI),
.instance_init = virtio_serial_pci_instance_init,
.class_init = virtio_serial_pci_class_init,
@@ -2561,7 +2588,10 @@ static void virtio_net_pci_instance_init(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_net_pci_info = {
- .generic_name = TYPE_VIRTIO_NET_PCI,
+ .base_name = TYPE_VIRTIO_NET_PCI,
+ .generic_name = "virtio-net-pci",
+ .transitional_name = "virtio-net-pci-transitional",
+ .non_transitional_name = "virtio-net-pci-non-transitional",
.instance_size = sizeof(VirtIONetPCI),
.instance_init = virtio_net_pci_instance_init,
.class_init = virtio_net_pci_class_init,
@@ -2611,7 +2641,10 @@ static void virtio_rng_initfn(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_rng_pci_info = {
- .generic_name = TYPE_VIRTIO_RNG_PCI,
+ .base_name = TYPE_VIRTIO_RNG_PCI,
+ .generic_name = "virtio-rng-pci",
+ .transitional_name = "virtio-rng-pci-transitional",
+ .non_transitional_name = "virtio-rng-pci-non-transitional",
.instance_size = sizeof(VirtIORngPCI),
.instance_init = virtio_rng_initfn,
.class_init = virtio_rng_pci_class_init,
@@ -2734,7 +2767,10 @@ static void virtio_host_initfn(Object *obj)
}
static const VirtioPCIDeviceTypeInfo virtio_host_pci_info = {
- .generic_name = TYPE_VIRTIO_INPUT_HOST_PCI,
+ .base_name = TYPE_VIRTIO_INPUT_HOST_PCI,
+ .generic_name = "virtio-input-host-pci",
+ .transitional_name = "virtio-input-host-pci-transitional",
+ .non_transitional_name = "virtio-input-host-pci-non-transitional",
.parent = TYPE_VIRTIO_INPUT_PCI,
.instance_size = sizeof(VirtIOInputHostPCI),
.instance_init = virtio_host_initfn,
diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py
new file mode 100644
index 0000000000..ce990250d8
--- /dev/null
+++ b/tests/acceptance/virtio_version.py
@@ -0,0 +1,176 @@
+"""
+Check compatibility of virtio device types
+"""
+# Copyright (c) 2018 Red Hat, Inc.
+#
+# Author:
+# Eduardo Habkost <ehabkost@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+import sys
+import os
+
+sys.path.append(os.path.join(os.path.dirname(__file__), "..", "..", "scripts"))
+from qemu import QEMUMachine
+from avocado_qemu import Test
+
+# Virtio Device IDs:
+VIRTIO_NET = 1
+VIRTIO_BLOCK = 2
+VIRTIO_CONSOLE = 3
+VIRTIO_RNG = 4
+VIRTIO_BALLOON = 5
+VIRTIO_RPMSG = 7
+VIRTIO_SCSI = 8
+VIRTIO_9P = 9
+VIRTIO_RPROC_SERIAL = 11
+VIRTIO_CAIF = 12
+VIRTIO_GPU = 16
+VIRTIO_INPUT = 18
+VIRTIO_VSOCK = 19
+VIRTIO_CRYPTO = 20
+
+PCI_VENDOR_ID_REDHAT_QUMRANET = 0x1af4
+
+# Device IDs for legacy/transitional devices:
+PCI_LEGACY_DEVICE_IDS = {
+ VIRTIO_NET: 0x1000,
+ VIRTIO_BLOCK: 0x1001,
+ VIRTIO_BALLOON: 0x1002,
+ VIRTIO_CONSOLE: 0x1003,
+ VIRTIO_SCSI: 0x1004,
+ VIRTIO_RNG: 0x1005,
+ VIRTIO_9P: 0x1009,
+ VIRTIO_VSOCK: 0x1012,
+}
+
+def pci_modern_device_id(virtio_devid):
+ return virtio_devid + 0x1040
+
+def devtype_implements(vm, devtype, implements):
+ return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)]
+
+def get_pci_interfaces(vm, devtype):
+ interfaces = ('pci-express-device', 'conventional-pci-device')
+ return [i for i in interfaces if devtype_implements(vm, devtype, i)]
+
+class VirtioVersionCheck(Test):
+ """
+ Check if virtio-version-specific device types result in the
+ same device tree created by `disable-modern` and
+ `disable-legacy`.
+
+ :avocado: enable
+ :avocado: tags=x86_64
+ """
+
+ # just in case there are failures, show larger diff:
+ maxDiff = 4096
+
+ def run_device(self, devtype, opts=None, machine='pc'):
+ """
+ Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
+ """
+ with QEMUMachine(self.qemu_bin) as vm:
+ vm.set_machine(machine)
+ if opts:
+ devtype += ',' + opts
+ vm.add_args('-device', '%s,id=devfortest' % (devtype))
+ vm.add_args('-S')
+ vm.launch()
+
+ pcibuses = vm.command('query-pci')
+ alldevs = [dev for bus in pcibuses for dev in bus['devices']]
+ devfortest = [dev for dev in alldevs
+ if dev['qdev_id'] == 'devfortest']
+ return devfortest[0], get_pci_interfaces(vm, devtype)
+
+
+ def assert_devids(self, dev, devid, non_transitional=False):
+ self.assertEqual(dev['id']['vendor'], PCI_VENDOR_ID_REDHAT_QUMRANET)
+ self.assertEqual(dev['id']['device'], devid)
+ if non_transitional:
+ self.assertTrue(0x1040 <= dev['id']['device'] <= 0x107f)
+ self.assertGreaterEqual(dev['id']['subsystem'], 0x40)
+
+ def check_all_variants(self, qemu_devtype, virtio_devid):
+ """Check if a virtio device type and its variants behave as expected"""
+ # Force modern mode:
+ dev_modern, _ = self.run_device(qemu_devtype,
+ 'disable-modern=off,disable-legacy=on')
+ self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid),
+ non_transitional=True)
+
+ # <prefix>-non-transitional device types should be 100% equivalent to
+ # <prefix>,disable-modern=off,disable-legacy=on
+ dev_1_0, nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype))
+ self.assertEqual(dev_modern, dev_1_0)
+
+ # Force transitional mode:
+ dev_trans, _ = self.run_device(qemu_devtype,
+ 'disable-modern=off,disable-legacy=off')
+ self.assert_devids(dev_trans, PCI_LEGACY_DEVICE_IDS[virtio_devid])
+
+ # Force legacy mode:
+ dev_legacy, _ = self.run_device(qemu_devtype,
+ 'disable-modern=on,disable-legacy=off')
+ self.assert_devids(dev_legacy, PCI_LEGACY_DEVICE_IDS[virtio_devid])
+
+ # No options: default to transitional on PC machine-type:
+ no_opts_pc, generic_ifaces = self.run_device(qemu_devtype)
+ self.assertEqual(dev_trans, no_opts_pc)
+
+ #TODO: check if plugging on a PCI Express bus will make the
+ # device non-transitional
+ #no_opts_q35 = self.run_device(qemu_devtype, machine='q35')
+ #self.assertEqual(dev_modern, no_opts_q35)
+
+ # <prefix>-transitional device types should be 100% equivalent to
+ # <prefix>,disable-modern=off,disable-legacy=off
+ dev_trans, trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype))
+ self.assertEqual(dev_trans, dev_trans)
+
+ # ensure the interface information is correct:
+ self.assertIn('conventional-pci-device', generic_ifaces)
+ self.assertIn('pci-express-device', generic_ifaces)
+
+ self.assertIn('conventional-pci-device', nt_ifaces)
+ self.assertIn('pci-express-device', nt_ifaces)
+
+ self.assertIn('conventional-pci-device', trans_ifaces)
+ self.assertNotIn('pci-express-device', trans_ifaces)
+
+
+ def test_conventional_devs(self):
+ self.check_all_variants('virtio-net-pci', VIRTIO_NET)
+ # virtio-blk requires 'driver' parameter
+ #self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
+ self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE)
+ self.check_all_variants('virtio-rng-pci', VIRTIO_RNG)
+ self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON)
+ self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI)
+ # virtio-9p requires 'fsdev' parameter
+ #self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
+
+ def check_modern_only(self, qemu_devtype, virtio_devid):
+ """Check if a modern-only virtio device type behaves as expected"""
+ # Force modern mode:
+ dev_modern, _ = self.run_device(qemu_devtype,
+ 'disable-modern=off,disable-legacy=on')
+ self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid),
+ non_transitional=True)
+
+ # No options: should be modern anyway
+ dev_no_opts, ifaces = self.run_device(qemu_devtype)
+ self.assertEqual(dev_modern, dev_no_opts)
+
+ self.assertIn('conventional-pci-device', ifaces)
+ self.assertIn('pci-express-device', ifaces)
+
+ def test_modern_only_devs(self):
+ self.check_modern_only('virtio-vga', VIRTIO_GPU)
+ self.check_modern_only('virtio-gpu-pci', VIRTIO_GPU)
+ self.check_modern_only('virtio-mouse-pci', VIRTIO_INPUT)
+ self.check_modern_only('virtio-tablet-pci', VIRTIO_INPUT)
+ self.check_modern_only('virtio-keyboard-pci', VIRTIO_INPUT)
--
2.18.0.rc1.1.g3f1ff2140
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-05 19:57 ` [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
@ 2018-12-07 12:03 ` Caio Carrara
2019-01-03 9:38 ` Thomas Huth
1 sibling, 0 replies; 20+ messages in thread
From: Caio Carrara @ 2018-12-07 12:03 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Michael S. Tsirkin, Gonglei, Paolo Bonzini,
Stefan Hajnoczi, Amit Shah, Andrea Bolognani, Cleber Rosa,
Marcel Apfelbaum, Fam Zheng, Cornelia Huck, Kevin Wolf,
Max Reitz, libvir-list, Wainer dos Santos Moschetta,
Philippe Mathieu-Daudé,
Markus Armbruster, Laine Stump, Jason Wang, Gerd Hoffmann,
Daniel P. Berrangé
On Wed, Dec 05, 2018 at 05:57:04PM -0200, Eduardo Habkost wrote:
> Many of the current virtio-*-pci device types actually represent
> 3 different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With these multi-purpose device
> types, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> - virtio-*-pci: the existing multi-purpose device types
> - Configurable using `disable-legacy` and `disable-modern`
> properties
> - Legacy driver support is automatically enabled/disabled
> depending on the bus where it is plugged
> - Supports Conventional PCI and PCI Express buses
> (but Conventional PCI is incompatible with
> disable-legacy=off)
> - Changes PCI vendor/device IDs at runtime
> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-non-transitional: modern-only
> - Supports both Conventional PCI and PCI Express buses
>
> The existing TYPE_* macros for these types will point to an
> abstract base type, so existing casts in the code will keep
> working for all variants.
>
> A simple test script (tests/acceptance/virtio_version.py) is
> included, to check if the new device types are equivalent to
> using the `disable-legacy` and `disable-modern` options.
>
> Acked-by: Andrea Bolognani <abologna@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Caio Carrara <ccarrara@redhat.com>
> ---
> Changes v3 -> v4:
> * Only test code changes (Caio Carrara)
> * Rename get_interfaces() to get_pci_interfaces()
> * Use tuple instead of list at get_pci_interfaces()
> * Spaces after commas
>
> Changes v2 -> v3:
> * Split into two separate patches (type registration helper
> and introduction of new types)
> * Include full type names as literals in the code instead
> of generating type names automatically
>
> Changes v1 -> v2:
> * Removed *-0.9 devices. Nobody will want to use them, if
> transitional devices work with legacy drivers
> (Gerd Hoffmann, Michael S. Tsirkin)
> * Drop virtio version from name: rename -1.0-transitional to
> -transitional (Michael S. Tsirkin)
> * Renamed -1.0 to -non-transitional
> * Don't add any extra variants to modern-only device types
> (they don't need it)
> * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
> Cornelia Huck)
> * No need to change cast macros for modern-only devices
> * Rename virtio_register_types() to virtio_pci_types_register()
> ---
> hw/virtio/virtio-pci.h | 24 ++--
> hw/virtio/virtio-pci.c | 60 ++++++++--
> tests/acceptance/virtio_version.py | 176 +++++++++++++++++++++++++++++
> 3 files changed, 236 insertions(+), 24 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 8cd546608e..29b4216107 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -216,7 +216,7 @@ static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
> /*
> * virtio-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci"
> +#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci-base"
> #define VIRTIO_SCSI_PCI(obj) \
> OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
>
> @@ -229,7 +229,7 @@ struct VirtIOSCSIPCI {
> /*
> * vhost-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci-base"
> #define VHOST_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
>
> @@ -239,7 +239,7 @@ struct VHostSCSIPCI {
> };
> #endif
>
> -#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
> +#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci-base"
> #define VHOST_USER_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
>
> @@ -252,7 +252,7 @@ struct VHostUserSCSIPCI {
> /*
> * vhost-user-blk-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci"
> +#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci-base"
> #define VHOST_USER_BLK_PCI(obj) \
> OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
>
> @@ -265,7 +265,7 @@ struct VHostUserBlkPCI {
> /*
> * virtio-blk-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
> +#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci-base"
> #define VIRTIO_BLK_PCI(obj) \
> OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
>
> @@ -277,7 +277,7 @@ struct VirtIOBlkPCI {
> /*
> * virtio-balloon-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci"
> +#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci-base"
> #define VIRTIO_BALLOON_PCI(obj) \
> OBJECT_CHECK(VirtIOBalloonPCI, (obj), TYPE_VIRTIO_BALLOON_PCI)
>
> @@ -289,7 +289,7 @@ struct VirtIOBalloonPCI {
> /*
> * virtio-serial-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci"
> +#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci-base"
> #define VIRTIO_SERIAL_PCI(obj) \
> OBJECT_CHECK(VirtIOSerialPCI, (obj), TYPE_VIRTIO_SERIAL_PCI)
>
> @@ -301,7 +301,7 @@ struct VirtIOSerialPCI {
> /*
> * virtio-net-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_NET_PCI "virtio-net-pci"
> +#define TYPE_VIRTIO_NET_PCI "virtio-net-pci-base"
> #define VIRTIO_NET_PCI(obj) \
> OBJECT_CHECK(VirtIONetPCI, (obj), TYPE_VIRTIO_NET_PCI)
>
> @@ -316,7 +316,7 @@ struct VirtIONetPCI {
>
> #ifdef CONFIG_VIRTFS
>
> -#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci"
> +#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci-base"
> #define VIRTIO_9P_PCI(obj) \
> OBJECT_CHECK(V9fsPCIState, (obj), TYPE_VIRTIO_9P_PCI)
>
> @@ -330,7 +330,7 @@ typedef struct V9fsPCIState {
> /*
> * virtio-rng-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci"
> +#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci-base"
> #define VIRTIO_RNG_PCI(obj) \
> OBJECT_CHECK(VirtIORngPCI, (obj), TYPE_VIRTIO_RNG_PCI)
>
> @@ -365,7 +365,7 @@ struct VirtIOInputHIDPCI {
>
> #ifdef CONFIG_LINUX
>
> -#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci"
> +#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci-base"
> #define VIRTIO_INPUT_HOST_PCI(obj) \
> OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
>
> @@ -392,7 +392,7 @@ struct VirtIOGPUPCI {
> /*
> * vhost-vsock-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci"
> +#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci-base"
> #define VHOST_VSOCK_PCI(obj) \
> OBJECT_CHECK(VHostVSockPCI, (obj), TYPE_VHOST_VSOCK_PCI)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f07ec55c38..d05066deb8 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1120,7 +1120,10 @@ static void virtio_9p_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_9p_pci_info = {
> - .generic_name = TYPE_VIRTIO_9P_PCI,
> + .base_name = TYPE_VIRTIO_9P_PCI,
> + .generic_name = "virtio-9p-pci",
> + .transitional_name = "virtio-9p-pci-transitional",
> + .non_transitional_name = "virtio-9p-pci-non-transitional",
> .instance_size = sizeof(V9fsPCIState),
> .instance_init = virtio_9p_pci_instance_init,
> .class_init = virtio_9p_pci_class_init,
> @@ -2102,7 +2105,10 @@ static void virtio_blk_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_blk_pci_info = {
> - .generic_name = TYPE_VIRTIO_BLK_PCI,
> + .base_name = TYPE_VIRTIO_BLK_PCI,
> + .generic_name = "virtio-blk-pci",
> + .transitional_name = "virtio-blk-pci-transitional",
> + .non_transitional_name = "virtio-blk-pci-non-transitional",
> .instance_size = sizeof(VirtIOBlkPCI),
> .instance_init = virtio_blk_pci_instance_init,
> .class_init = virtio_blk_pci_class_init,
> @@ -2157,7 +2163,10 @@ static void vhost_user_blk_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo vhost_user_blk_pci_info = {
> - .generic_name = TYPE_VHOST_USER_BLK_PCI,
> + .base_name = TYPE_VHOST_USER_BLK_PCI,
> + .generic_name = "vhost-user-blk-pci",
> + .transitional_name = "vhost-user-blk-pci-transitional",
> + .non_transitional_name = "vhost-user-blk-pci-non-transitional",
> .instance_size = sizeof(VHostUserBlkPCI),
> .instance_init = vhost_user_blk_pci_instance_init,
> .class_init = vhost_user_blk_pci_class_init,
> @@ -2224,7 +2233,10 @@ static void virtio_scsi_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_scsi_pci_info = {
> - .generic_name = TYPE_VIRTIO_SCSI_PCI,
> + .base_name = TYPE_VIRTIO_SCSI_PCI,
> + .generic_name = "virtio-scsi-pci",
> + .transitional_name = "virtio-scsi-pci-transitional",
> + .non_transitional_name = "virtio-scsi-pci-non-transitional",
> .instance_size = sizeof(VirtIOSCSIPCI),
> .instance_init = virtio_scsi_pci_instance_init,
> .class_init = virtio_scsi_pci_class_init,
> @@ -2278,7 +2290,10 @@ static void vhost_scsi_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo vhost_scsi_pci_info = {
> - .generic_name = TYPE_VHOST_SCSI_PCI,
> + .base_name = TYPE_VHOST_SCSI_PCI,
> + .generic_name = "vhost-scsi-pci",
> + .transitional_name = "vhost-scsi-pci-transitional",
> + .non_transitional_name = "vhost-scsi-pci-non-transitional",
> .instance_size = sizeof(VHostSCSIPCI),
> .instance_init = vhost_scsi_pci_instance_init,
> .class_init = vhost_scsi_pci_class_init,
> @@ -2332,7 +2347,10 @@ static void vhost_user_scsi_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo vhost_user_scsi_pci_info = {
> - .generic_name = TYPE_VHOST_USER_SCSI_PCI,
> + .base_name = TYPE_VHOST_USER_SCSI_PCI,
> + .generic_name = "vhost-user-scsi-pci",
> + .transitional_name = "vhost-user-scsi-pci-transitional",
> + .non_transitional_name = "vhost-user-scsi-pci-non-transitional",
> .instance_size = sizeof(VHostUserSCSIPCI),
> .instance_init = vhost_user_scsi_pci_instance_init,
> .class_init = vhost_user_scsi_pci_class_init,
> @@ -2379,7 +2397,10 @@ static void vhost_vsock_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
> - .generic_name = TYPE_VHOST_VSOCK_PCI,
> + .base_name = TYPE_VHOST_VSOCK_PCI,
> + .generic_name = "vhost-vsock-pci",
> + .transitional_name = "vhost-vsock-pci-transitional",
> + .non_transitional_name = "vhost-vsock-pci-non-transitional",
> .instance_size = sizeof(VHostVSockPCI),
> .instance_init = vhost_vsock_pci_instance_init,
> .class_init = vhost_vsock_pci_class_init,
> @@ -2435,7 +2456,10 @@ static void virtio_balloon_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_balloon_pci_info = {
> - .generic_name = TYPE_VIRTIO_BALLOON_PCI,
> + .base_name = TYPE_VIRTIO_BALLOON_PCI,
> + .generic_name = "virtio-balloon-pci",
> + .transitional_name = "virtio-balloon-pci-transitional",
> + .non_transitional_name = "virtio-balloon-pci-non-transitional",
> .instance_size = sizeof(VirtIOBalloonPCI),
> .instance_init = virtio_balloon_pci_instance_init,
> .class_init = virtio_balloon_pci_class_init,
> @@ -2507,7 +2531,10 @@ static void virtio_serial_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_serial_pci_info = {
> - .generic_name = TYPE_VIRTIO_SERIAL_PCI,
> + .base_name = TYPE_VIRTIO_SERIAL_PCI,
> + .generic_name = "virtio-serial-pci",
> + .transitional_name = "virtio-serial-pci-transitional",
> + .non_transitional_name = "virtio-serial-pci-non-transitional",
> .instance_size = sizeof(VirtIOSerialPCI),
> .instance_init = virtio_serial_pci_instance_init,
> .class_init = virtio_serial_pci_class_init,
> @@ -2561,7 +2588,10 @@ static void virtio_net_pci_instance_init(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_net_pci_info = {
> - .generic_name = TYPE_VIRTIO_NET_PCI,
> + .base_name = TYPE_VIRTIO_NET_PCI,
> + .generic_name = "virtio-net-pci",
> + .transitional_name = "virtio-net-pci-transitional",
> + .non_transitional_name = "virtio-net-pci-non-transitional",
> .instance_size = sizeof(VirtIONetPCI),
> .instance_init = virtio_net_pci_instance_init,
> .class_init = virtio_net_pci_class_init,
> @@ -2611,7 +2641,10 @@ static void virtio_rng_initfn(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_rng_pci_info = {
> - .generic_name = TYPE_VIRTIO_RNG_PCI,
> + .base_name = TYPE_VIRTIO_RNG_PCI,
> + .generic_name = "virtio-rng-pci",
> + .transitional_name = "virtio-rng-pci-transitional",
> + .non_transitional_name = "virtio-rng-pci-non-transitional",
> .instance_size = sizeof(VirtIORngPCI),
> .instance_init = virtio_rng_initfn,
> .class_init = virtio_rng_pci_class_init,
> @@ -2734,7 +2767,10 @@ static void virtio_host_initfn(Object *obj)
> }
>
> static const VirtioPCIDeviceTypeInfo virtio_host_pci_info = {
> - .generic_name = TYPE_VIRTIO_INPUT_HOST_PCI,
> + .base_name = TYPE_VIRTIO_INPUT_HOST_PCI,
> + .generic_name = "virtio-input-host-pci",
> + .transitional_name = "virtio-input-host-pci-transitional",
> + .non_transitional_name = "virtio-input-host-pci-non-transitional",
> .parent = TYPE_VIRTIO_INPUT_PCI,
> .instance_size = sizeof(VirtIOInputHostPCI),
> .instance_init = virtio_host_initfn,
> diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py
> new file mode 100644
> index 0000000000..ce990250d8
> --- /dev/null
> +++ b/tests/acceptance/virtio_version.py
> @@ -0,0 +1,176 @@
> +"""
> +Check compatibility of virtio device types
> +"""
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +# Eduardo Habkost <ehabkost@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +import sys
> +import os
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "..", "..", "scripts"))
> +from qemu import QEMUMachine
> +from avocado_qemu import Test
> +
> +# Virtio Device IDs:
> +VIRTIO_NET = 1
> +VIRTIO_BLOCK = 2
> +VIRTIO_CONSOLE = 3
> +VIRTIO_RNG = 4
> +VIRTIO_BALLOON = 5
> +VIRTIO_RPMSG = 7
> +VIRTIO_SCSI = 8
> +VIRTIO_9P = 9
> +VIRTIO_RPROC_SERIAL = 11
> +VIRTIO_CAIF = 12
> +VIRTIO_GPU = 16
> +VIRTIO_INPUT = 18
> +VIRTIO_VSOCK = 19
> +VIRTIO_CRYPTO = 20
> +
> +PCI_VENDOR_ID_REDHAT_QUMRANET = 0x1af4
> +
> +# Device IDs for legacy/transitional devices:
> +PCI_LEGACY_DEVICE_IDS = {
> + VIRTIO_NET: 0x1000,
> + VIRTIO_BLOCK: 0x1001,
> + VIRTIO_BALLOON: 0x1002,
> + VIRTIO_CONSOLE: 0x1003,
> + VIRTIO_SCSI: 0x1004,
> + VIRTIO_RNG: 0x1005,
> + VIRTIO_9P: 0x1009,
> + VIRTIO_VSOCK: 0x1012,
> +}
> +
> +def pci_modern_device_id(virtio_devid):
> + return virtio_devid + 0x1040
> +
> +def devtype_implements(vm, devtype, implements):
> + return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)]
> +
> +def get_pci_interfaces(vm, devtype):
> + interfaces = ('pci-express-device', 'conventional-pci-device')
> + return [i for i in interfaces if devtype_implements(vm, devtype, i)]
> +
> +class VirtioVersionCheck(Test):
> + """
> + Check if virtio-version-specific device types result in the
> + same device tree created by `disable-modern` and
> + `disable-legacy`.
> +
> + :avocado: enable
> + :avocado: tags=x86_64
> + """
> +
> + # just in case there are failures, show larger diff:
> + maxDiff = 4096
> +
> + def run_device(self, devtype, opts=None, machine='pc'):
> + """
> + Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
> + """
> + with QEMUMachine(self.qemu_bin) as vm:
> + vm.set_machine(machine)
> + if opts:
> + devtype += ',' + opts
> + vm.add_args('-device', '%s,id=devfortest' % (devtype))
> + vm.add_args('-S')
> + vm.launch()
> +
> + pcibuses = vm.command('query-pci')
> + alldevs = [dev for bus in pcibuses for dev in bus['devices']]
> + devfortest = [dev for dev in alldevs
> + if dev['qdev_id'] == 'devfortest']
> + return devfortest[0], get_pci_interfaces(vm, devtype)
> +
> +
> + def assert_devids(self, dev, devid, non_transitional=False):
> + self.assertEqual(dev['id']['vendor'], PCI_VENDOR_ID_REDHAT_QUMRANET)
> + self.assertEqual(dev['id']['device'], devid)
> + if non_transitional:
> + self.assertTrue(0x1040 <= dev['id']['device'] <= 0x107f)
> + self.assertGreaterEqual(dev['id']['subsystem'], 0x40)
> +
> + def check_all_variants(self, qemu_devtype, virtio_devid):
> + """Check if a virtio device type and its variants behave as expected"""
> + # Force modern mode:
> + dev_modern, _ = self.run_device(qemu_devtype,
> + 'disable-modern=off,disable-legacy=on')
> + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid),
> + non_transitional=True)
> +
> + # <prefix>-non-transitional device types should be 100% equivalent to
> + # <prefix>,disable-modern=off,disable-legacy=on
> + dev_1_0, nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype))
> + self.assertEqual(dev_modern, dev_1_0)
> +
> + # Force transitional mode:
> + dev_trans, _ = self.run_device(qemu_devtype,
> + 'disable-modern=off,disable-legacy=off')
> + self.assert_devids(dev_trans, PCI_LEGACY_DEVICE_IDS[virtio_devid])
> +
> + # Force legacy mode:
> + dev_legacy, _ = self.run_device(qemu_devtype,
> + 'disable-modern=on,disable-legacy=off')
> + self.assert_devids(dev_legacy, PCI_LEGACY_DEVICE_IDS[virtio_devid])
> +
> + # No options: default to transitional on PC machine-type:
> + no_opts_pc, generic_ifaces = self.run_device(qemu_devtype)
> + self.assertEqual(dev_trans, no_opts_pc)
> +
> + #TODO: check if plugging on a PCI Express bus will make the
> + # device non-transitional
> + #no_opts_q35 = self.run_device(qemu_devtype, machine='q35')
> + #self.assertEqual(dev_modern, no_opts_q35)
> +
> + # <prefix>-transitional device types should be 100% equivalent to
> + # <prefix>,disable-modern=off,disable-legacy=off
> + dev_trans, trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype))
> + self.assertEqual(dev_trans, dev_trans)
> +
> + # ensure the interface information is correct:
> + self.assertIn('conventional-pci-device', generic_ifaces)
> + self.assertIn('pci-express-device', generic_ifaces)
> +
> + self.assertIn('conventional-pci-device', nt_ifaces)
> + self.assertIn('pci-express-device', nt_ifaces)
> +
> + self.assertIn('conventional-pci-device', trans_ifaces)
> + self.assertNotIn('pci-express-device', trans_ifaces)
> +
> +
> + def test_conventional_devs(self):
> + self.check_all_variants('virtio-net-pci', VIRTIO_NET)
> + # virtio-blk requires 'driver' parameter
> + #self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
> + self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE)
> + self.check_all_variants('virtio-rng-pci', VIRTIO_RNG)
> + self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON)
> + self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI)
> + # virtio-9p requires 'fsdev' parameter
> + #self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
> +
> + def check_modern_only(self, qemu_devtype, virtio_devid):
> + """Check if a modern-only virtio device type behaves as expected"""
> + # Force modern mode:
> + dev_modern, _ = self.run_device(qemu_devtype,
> + 'disable-modern=off,disable-legacy=on')
> + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid),
> + non_transitional=True)
> +
> + # No options: should be modern anyway
> + dev_no_opts, ifaces = self.run_device(qemu_devtype)
> + self.assertEqual(dev_modern, dev_no_opts)
> +
> + self.assertIn('conventional-pci-device', ifaces)
> + self.assertIn('pci-express-device', ifaces)
> +
> + def test_modern_only_devs(self):
> + self.check_modern_only('virtio-vga', VIRTIO_GPU)
> + self.check_modern_only('virtio-gpu-pci', VIRTIO_GPU)
> + self.check_modern_only('virtio-mouse-pci', VIRTIO_INPUT)
> + self.check_modern_only('virtio-tablet-pci', VIRTIO_INPUT)
> + self.check_modern_only('virtio-keyboard-pci', VIRTIO_INPUT)
> --
> 2.18.0.rc1.1.g3f1ff2140
>
--
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara@redhat.com
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-05 19:57 ` [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
2018-12-07 12:03 ` Caio Carrara
@ 2019-01-03 9:38 ` Thomas Huth
2019-01-03 10:14 ` Thomas Huth
1 sibling, 1 reply; 20+ messages in thread
From: Thomas Huth @ 2019-01-03 9:38 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Michael S. Tsirkin, Kevin Wolf, Fam Zheng, Amit Shah, qemu-s390x,
Markus Armbruster, Jason Wang, Cornelia Huck, Andrea Bolognani,
Wainer dos Santos Moschetta, Max Reitz, Caio Carrara, Gonglei,
Laine Stump, Gerd Hoffmann, Stefan Hajnoczi, Cleber Rosa,
Paolo Bonzini, Philippe Mathieu-Daudé
On 2018-12-05 20:57, Eduardo Habkost wrote:
> Many of the current virtio-*-pci device types actually represent
> 3 different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With these multi-purpose device
> types, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> - virtio-*-pci: the existing multi-purpose device types
> - Configurable using `disable-legacy` and `disable-modern`
> properties
> - Legacy driver support is automatically enabled/disabled
> depending on the bus where it is plugged
> - Supports Conventional PCI and PCI Express buses
> (but Conventional PCI is incompatible with
> disable-legacy=off)
> - Changes PCI vendor/device IDs at runtime
> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-non-transitional: modern-only
> - Supports both Conventional PCI and PCI Express buses
>
> The existing TYPE_* macros for these types will point to an
> abstract base type, so existing casts in the code will keep
> working for all variants.
>
> A simple test script (tests/acceptance/virtio_version.py) is
> included, to check if the new device types are equivalent to
> using the `disable-legacy` and `disable-modern` options.
>
> Acked-by: Andrea Bolognani <abologna@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Hi Eduardo,
with these new devices, I can trigger an abort on s390x:
$ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
QEMU 3.1.50 monitor - type 'help' for more information
(qemu) device_add vhost-scsi-pci-non-transitional
qemu-system-s390x: hw/core/qdev-properties.c:1236:
qdev_prop_set_globals: Assertion `prop->user_provided' failed.
Aborted (core dumped)
Any ideas how to fix that?
Thomas
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2019-01-03 9:38 ` Thomas Huth
@ 2019-01-03 10:14 ` Thomas Huth
2019-01-03 10:48 ` Cornelia Huck
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Huth @ 2019-01-03 10:14 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Michael S. Tsirkin, Kevin Wolf, Fam Zheng, Amit Shah, qemu-s390x,
Markus Armbruster, Jason Wang, Cornelia Huck, Andrea Bolognani,
Wainer dos Santos Moschetta, Max Reitz, Caio Carrara, Gonglei,
Laine Stump, Gerd Hoffmann, Stefan Hajnoczi, Cleber Rosa,
Paolo Bonzini, Philippe Mathieu-Daudé
On 2019-01-03 10:38, Thomas Huth wrote:
> On 2018-12-05 20:57, Eduardo Habkost wrote:
>> Many of the current virtio-*-pci device types actually represent
>> 3 different types of devices:
>> * virtio 1.0 non-transitional devices
>> * virtio 1.0 transitional devices
>> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>>
>> That would be just an annoyance if it didn't break our device/bus
>> compatibility QMP interfaces. With these multi-purpose device
>> types, there's no way to tell management software that
>> transitional devices and legacy devices require a Conventional
>> PCI bus.
>>
>> The multi-purpose device types would also prevent us from telling
>> management software what's the PCI vendor/device ID for them,
>> because their PCI IDs change at runtime depending on the bus
>> where they were plugged.
>>
>> This patch adds separate device types for each of those virtio
>> device flavors:
>>
>> - virtio-*-pci: the existing multi-purpose device types
>> - Configurable using `disable-legacy` and `disable-modern`
>> properties
>> - Legacy driver support is automatically enabled/disabled
>> depending on the bus where it is plugged
>> - Supports Conventional PCI and PCI Express buses
>> (but Conventional PCI is incompatible with
>> disable-legacy=off)
>> - Changes PCI vendor/device IDs at runtime
>> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
>> - Supports Conventional PCI buses only, because
>> it has a PIO BAR
>> - virtio-*-pci-non-transitional: modern-only
>> - Supports both Conventional PCI and PCI Express buses
>>
>> The existing TYPE_* macros for these types will point to an
>> abstract base type, so existing casts in the code will keep
>> working for all variants.
>>
>> A simple test script (tests/acceptance/virtio_version.py) is
>> included, to check if the new device types are equivalent to
>> using the `disable-legacy` and `disable-modern` options.
>>
>> Acked-by: Andrea Bolognani <abologna@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
> Hi Eduardo,
>
> with these new devices, I can trigger an abort on s390x:
>
> $ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
> QEMU 3.1.50 monitor - type 'help' for more information
> (qemu) device_add vhost-scsi-pci-non-transitional
> qemu-system-s390x: hw/core/qdev-properties.c:1236:
> qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> Aborted (core dumped)
FWIW, it happens with x86, too:
$ qemu-system-x86_64 -M pc-i440fx-2.6 -monitor stdio
QEMU 3.1.50 monitor - type 'help' for more information
(qemu) device_add vhost-scsi-pci-non-transitional
qemu-system-x86_64: hw/core/qdev-properties.c:1236:
qdev_prop_set_globals: Assertion `prop->user_provided' failed.
Aborted (core dumped)
Only machine types newer than 2.7 seem to be OK.
Thomas
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2019-01-03 10:14 ` Thomas Huth
@ 2019-01-03 10:48 ` Cornelia Huck
2019-01-03 18:32 ` Eduardo Habkost
0 siblings, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-01-03 10:48 UTC (permalink / raw)
To: Thomas Huth
Cc: Eduardo Habkost, qemu-devel, Michael S. Tsirkin, Kevin Wolf,
Fam Zheng, Amit Shah, qemu-s390x, Markus Armbruster, Jason Wang,
Andrea Bolognani, Wainer dos Santos Moschetta, Max Reitz,
Caio Carrara, Gonglei, Laine Stump, Gerd Hoffmann,
Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini,
Philippe Mathieu-Daudé
On Thu, 3 Jan 2019 11:14:44 +0100
Thomas Huth <thuth@redhat.com> wrote:
> On 2019-01-03 10:38, Thomas Huth wrote:
> > On 2018-12-05 20:57, Eduardo Habkost wrote:
> >> Many of the current virtio-*-pci device types actually represent
> >> 3 different types of devices:
> >> * virtio 1.0 non-transitional devices
> >> * virtio 1.0 transitional devices
> >> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >>
> >> That would be just an annoyance if it didn't break our device/bus
> >> compatibility QMP interfaces. With these multi-purpose device
> >> types, there's no way to tell management software that
> >> transitional devices and legacy devices require a Conventional
> >> PCI bus.
> >>
> >> The multi-purpose device types would also prevent us from telling
> >> management software what's the PCI vendor/device ID for them,
> >> because their PCI IDs change at runtime depending on the bus
> >> where they were plugged.
> >>
> >> This patch adds separate device types for each of those virtio
> >> device flavors:
> >>
> >> - virtio-*-pci: the existing multi-purpose device types
> >> - Configurable using `disable-legacy` and `disable-modern`
> >> properties
> >> - Legacy driver support is automatically enabled/disabled
> >> depending on the bus where it is plugged
> >> - Supports Conventional PCI and PCI Express buses
> >> (but Conventional PCI is incompatible with
> >> disable-legacy=off)
> >> - Changes PCI vendor/device IDs at runtime
> >> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> >> - Supports Conventional PCI buses only, because
> >> it has a PIO BAR
> >> - virtio-*-pci-non-transitional: modern-only
> >> - Supports both Conventional PCI and PCI Express buses
> >>
> >> The existing TYPE_* macros for these types will point to an
> >> abstract base type, so existing casts in the code will keep
> >> working for all variants.
> >>
> >> A simple test script (tests/acceptance/virtio_version.py) is
> >> included, to check if the new device types are equivalent to
> >> using the `disable-legacy` and `disable-modern` options.
> >>
> >> Acked-by: Andrea Bolognani <abologna@redhat.com>
> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > Hi Eduardo,
> >
> > with these new devices, I can trigger an abort on s390x:
> >
> > $ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
> > QEMU 3.1.50 monitor - type 'help' for more information
> > (qemu) device_add vhost-scsi-pci-non-transitional
> > qemu-system-s390x: hw/core/qdev-properties.c:1236:
> > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > Aborted (core dumped)
> FWIW, it happens with x86, too:
>
> $ qemu-system-x86_64 -M pc-i440fx-2.6 -monitor stdio
> QEMU 3.1.50 monitor - type 'help' for more information
> (qemu) device_add vhost-scsi-pci-non-transitional
> qemu-system-x86_64: hw/core/qdev-properties.c:1236:
> qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> Aborted (core dumped)
>
> Only machine types newer than 2.7 seem to be OK.
It also fails for vhost-scsi-pci-transitional on 2.6 and older machines.
The problem is that HW_COMPAT_2_6 tries to set the disable_modern
property, which the version-specific variants of virtio-pci do not have.
Not sure how to fix this. The version-specific devices cannot really
work on the compat machine.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2019-01-03 10:48 ` Cornelia Huck
@ 2019-01-03 18:32 ` Eduardo Habkost
2019-01-04 4:27 ` Michael S. Tsirkin
0 siblings, 1 reply; 20+ messages in thread
From: Eduardo Habkost @ 2019-01-03 18:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: Thomas Huth, qemu-devel, Michael S. Tsirkin, Kevin Wolf,
Fam Zheng, Amit Shah, qemu-s390x, Markus Armbruster, Jason Wang,
Andrea Bolognani, Wainer dos Santos Moschetta, Max Reitz,
Caio Carrara, Gonglei, Laine Stump, Gerd Hoffmann,
Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini,
Philippe Mathieu-Daudé
On Thu, Jan 03, 2019 at 11:48:37AM +0100, Cornelia Huck wrote:
> On Thu, 3 Jan 2019 11:14:44 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
> > On 2019-01-03 10:38, Thomas Huth wrote:
> > > On 2018-12-05 20:57, Eduardo Habkost wrote:
> > >> Many of the current virtio-*-pci device types actually represent
> > >> 3 different types of devices:
> > >> * virtio 1.0 non-transitional devices
> > >> * virtio 1.0 transitional devices
> > >> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > >>
> > >> That would be just an annoyance if it didn't break our device/bus
> > >> compatibility QMP interfaces. With these multi-purpose device
> > >> types, there's no way to tell management software that
> > >> transitional devices and legacy devices require a Conventional
> > >> PCI bus.
> > >>
> > >> The multi-purpose device types would also prevent us from telling
> > >> management software what's the PCI vendor/device ID for them,
> > >> because their PCI IDs change at runtime depending on the bus
> > >> where they were plugged.
> > >>
> > >> This patch adds separate device types for each of those virtio
> > >> device flavors:
> > >>
> > >> - virtio-*-pci: the existing multi-purpose device types
> > >> - Configurable using `disable-legacy` and `disable-modern`
> > >> properties
> > >> - Legacy driver support is automatically enabled/disabled
> > >> depending on the bus where it is plugged
> > >> - Supports Conventional PCI and PCI Express buses
> > >> (but Conventional PCI is incompatible with
> > >> disable-legacy=off)
> > >> - Changes PCI vendor/device IDs at runtime
> > >> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > >> - Supports Conventional PCI buses only, because
> > >> it has a PIO BAR
> > >> - virtio-*-pci-non-transitional: modern-only
> > >> - Supports both Conventional PCI and PCI Express buses
> > >>
> > >> The existing TYPE_* macros for these types will point to an
> > >> abstract base type, so existing casts in the code will keep
> > >> working for all variants.
> > >>
> > >> A simple test script (tests/acceptance/virtio_version.py) is
> > >> included, to check if the new device types are equivalent to
> > >> using the `disable-legacy` and `disable-modern` options.
> > >>
> > >> Acked-by: Andrea Bolognani <abologna@redhat.com>
> > >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > >
> > > Hi Eduardo,
> > >
> > > with these new devices, I can trigger an abort on s390x:
> > >
> > > $ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
> > > QEMU 3.1.50 monitor - type 'help' for more information
> > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > qemu-system-s390x: hw/core/qdev-properties.c:1236:
> > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > Aborted (core dumped)
> > FWIW, it happens with x86, too:
> >
> > $ qemu-system-x86_64 -M pc-i440fx-2.6 -monitor stdio
> > QEMU 3.1.50 monitor - type 'help' for more information
> > (qemu) device_add vhost-scsi-pci-non-transitional
> > qemu-system-x86_64: hw/core/qdev-properties.c:1236:
> > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > Aborted (core dumped)
> >
> > Only machine types newer than 2.7 seem to be OK.
>
> It also fails for vhost-scsi-pci-transitional on 2.6 and older machines.
>
> The problem is that HW_COMPAT_2_6 tries to set the disable_modern
> property, which the version-specific variants of virtio-pci do not have.
>
> Not sure how to fix this. The version-specific devices cannot really
> work on the compat machine.
Oops.
My patch broke the assumption that every virtio-pci subclass is a
generic legacy/modern device. The good news is that HW_COMPAT_2_6
needs to affect only devices that existed in QEMU
2.6 (not every subclass of virtio-pci).
I see 3 possible ways to address this:
1) Making disable-legacy and disable-modern available on all
virtio-pci subclasses again.
This is simple to implement, but I would like to avoid that. I'd
prefer to keep the legacy/modern logic complexity restricted to
the old generic virtio devices.
2) Creating a virtio-pci-generic or virtio-pci-hybrid interface
type, add it to generic_type_info at virtio_pci_types_register(),
and use it on HW_COMPAT_2_6 instead of "virtio-pci".
This is simple to implement, but I'm wary. I'm already bothered
by the complexity of our PCI and virtio type hierarchies. We
have multiple overlapping sets of virtio-pci devices (depending
which way you look), and specifying exactly which set we want to
affect is tricky.
3) Replacing "virtio-pci" on HW_COMPAT_2_6 with an explicit list
of concrete virtio-pci types that existed when QEMU 2.6 was
released. This will make HW_COMPAT_2_6 grow, but reduce
complexity of the whole system. I'm inclined to implement this
solution.
--
Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2019-01-03 18:32 ` Eduardo Habkost
@ 2019-01-04 4:27 ` Michael S. Tsirkin
2019-01-04 8:27 ` Cornelia Huck
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2019-01-04 4:27 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Cornelia Huck, Thomas Huth, qemu-devel, Kevin Wolf, Fam Zheng,
Amit Shah, qemu-s390x, Markus Armbruster, Jason Wang,
Andrea Bolognani, Wainer dos Santos Moschetta, Max Reitz,
Caio Carrara, Gonglei, Laine Stump, Gerd Hoffmann,
Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini,
Philippe Mathieu-Daudé
On Thu, Jan 03, 2019 at 04:32:06PM -0200, Eduardo Habkost wrote:
> On Thu, Jan 03, 2019 at 11:48:37AM +0100, Cornelia Huck wrote:
> > On Thu, 3 Jan 2019 11:14:44 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >
> > > On 2019-01-03 10:38, Thomas Huth wrote:
> > > > On 2018-12-05 20:57, Eduardo Habkost wrote:
> > > >> Many of the current virtio-*-pci device types actually represent
> > > >> 3 different types of devices:
> > > >> * virtio 1.0 non-transitional devices
> > > >> * virtio 1.0 transitional devices
> > > >> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > > >>
> > > >> That would be just an annoyance if it didn't break our device/bus
> > > >> compatibility QMP interfaces. With these multi-purpose device
> > > >> types, there's no way to tell management software that
> > > >> transitional devices and legacy devices require a Conventional
> > > >> PCI bus.
> > > >>
> > > >> The multi-purpose device types would also prevent us from telling
> > > >> management software what's the PCI vendor/device ID for them,
> > > >> because their PCI IDs change at runtime depending on the bus
> > > >> where they were plugged.
> > > >>
> > > >> This patch adds separate device types for each of those virtio
> > > >> device flavors:
> > > >>
> > > >> - virtio-*-pci: the existing multi-purpose device types
> > > >> - Configurable using `disable-legacy` and `disable-modern`
> > > >> properties
> > > >> - Legacy driver support is automatically enabled/disabled
> > > >> depending on the bus where it is plugged
> > > >> - Supports Conventional PCI and PCI Express buses
> > > >> (but Conventional PCI is incompatible with
> > > >> disable-legacy=off)
> > > >> - Changes PCI vendor/device IDs at runtime
> > > >> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > > >> - Supports Conventional PCI buses only, because
> > > >> it has a PIO BAR
> > > >> - virtio-*-pci-non-transitional: modern-only
> > > >> - Supports both Conventional PCI and PCI Express buses
> > > >>
> > > >> The existing TYPE_* macros for these types will point to an
> > > >> abstract base type, so existing casts in the code will keep
> > > >> working for all variants.
> > > >>
> > > >> A simple test script (tests/acceptance/virtio_version.py) is
> > > >> included, to check if the new device types are equivalent to
> > > >> using the `disable-legacy` and `disable-modern` options.
> > > >>
> > > >> Acked-by: Andrea Bolognani <abologna@redhat.com>
> > > >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > >
> > > > Hi Eduardo,
> > > >
> > > > with these new devices, I can trigger an abort on s390x:
> > > >
> > > > $ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
> > > > QEMU 3.1.50 monitor - type 'help' for more information
> > > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > > qemu-system-s390x: hw/core/qdev-properties.c:1236:
> > > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > > Aborted (core dumped)
> > > FWIW, it happens with x86, too:
> > >
> > > $ qemu-system-x86_64 -M pc-i440fx-2.6 -monitor stdio
> > > QEMU 3.1.50 monitor - type 'help' for more information
> > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > qemu-system-x86_64: hw/core/qdev-properties.c:1236:
> > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > Aborted (core dumped)
> > >
> > > Only machine types newer than 2.7 seem to be OK.
> >
> > It also fails for vhost-scsi-pci-transitional on 2.6 and older machines.
> >
> > The problem is that HW_COMPAT_2_6 tries to set the disable_modern
> > property, which the version-specific variants of virtio-pci do not have.
> >
> > Not sure how to fix this. The version-specific devices cannot really
> > work on the compat machine.
>
> Oops.
>
> My patch broke the assumption that every virtio-pci subclass is a
> generic legacy/modern device. The good news is that HW_COMPAT_2_6
> needs to affect only devices that existed in QEMU
> 2.6 (not every subclass of virtio-pci).
>
> I see 3 possible ways to address this:
>
> 1) Making disable-legacy and disable-modern available on all
> virtio-pci subclasses again.
>
> This is simple to implement, but I would like to avoid that. I'd
> prefer to keep the legacy/modern logic complexity restricted to
> the old generic virtio devices.
As it's only for compat, we can probably use an "x-" property.
>
> 2) Creating a virtio-pci-generic or virtio-pci-hybrid interface
> type, add it to generic_type_info at virtio_pci_types_register(),
> and use it on HW_COMPAT_2_6 instead of "virtio-pci".
>
> This is simple to implement, but I'm wary. I'm already bothered
> by the complexity of our PCI and virtio type hierarchies. We
> have multiple overlapping sets of virtio-pci devices (depending
> which way you look), and specifying exactly which set we want to
> affect is tricky.
I'm not sure I understand this one. Would this be required
anyway if we added a new feature to both transitional and modern
devices?
>
> 3) Replacing "virtio-pci" on HW_COMPAT_2_6 with an explicit list
> of concrete virtio-pci types that existed when QEMU 2.6 was
> released. This will make HW_COMPAT_2_6 grow, but reduce
> complexity of the whole system. I'm inclined to implement this
> solution.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices
2019-01-04 4:27 ` Michael S. Tsirkin
@ 2019-01-04 8:27 ` Cornelia Huck
0 siblings, 0 replies; 20+ messages in thread
From: Cornelia Huck @ 2019-01-04 8:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eduardo Habkost, Thomas Huth, qemu-devel, Kevin Wolf, Fam Zheng,
Amit Shah, qemu-s390x, Markus Armbruster, Jason Wang,
Andrea Bolognani, Wainer dos Santos Moschetta, Max Reitz,
Caio Carrara, Gonglei, Laine Stump, Gerd Hoffmann,
Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini,
Philippe Mathieu-Daudé
On Thu, 3 Jan 2019 23:27:08 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jan 03, 2019 at 04:32:06PM -0200, Eduardo Habkost wrote:
> > On Thu, Jan 03, 2019 at 11:48:37AM +0100, Cornelia Huck wrote:
> > > On Thu, 3 Jan 2019 11:14:44 +0100
> > > Thomas Huth <thuth@redhat.com> wrote:
> > >
> > > > On 2019-01-03 10:38, Thomas Huth wrote:
> > > > > On 2018-12-05 20:57, Eduardo Habkost wrote:
> > > > >> Many of the current virtio-*-pci device types actually represent
> > > > >> 3 different types of devices:
> > > > >> * virtio 1.0 non-transitional devices
> > > > >> * virtio 1.0 transitional devices
> > > > >> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > > > >>
> > > > >> That would be just an annoyance if it didn't break our device/bus
> > > > >> compatibility QMP interfaces. With these multi-purpose device
> > > > >> types, there's no way to tell management software that
> > > > >> transitional devices and legacy devices require a Conventional
> > > > >> PCI bus.
> > > > >>
> > > > >> The multi-purpose device types would also prevent us from telling
> > > > >> management software what's the PCI vendor/device ID for them,
> > > > >> because their PCI IDs change at runtime depending on the bus
> > > > >> where they were plugged.
> > > > >>
> > > > >> This patch adds separate device types for each of those virtio
> > > > >> device flavors:
> > > > >>
> > > > >> - virtio-*-pci: the existing multi-purpose device types
> > > > >> - Configurable using `disable-legacy` and `disable-modern`
> > > > >> properties
> > > > >> - Legacy driver support is automatically enabled/disabled
> > > > >> depending on the bus where it is plugged
> > > > >> - Supports Conventional PCI and PCI Express buses
> > > > >> (but Conventional PCI is incompatible with
> > > > >> disable-legacy=off)
> > > > >> - Changes PCI vendor/device IDs at runtime
> > > > >> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > > > >> - Supports Conventional PCI buses only, because
> > > > >> it has a PIO BAR
> > > > >> - virtio-*-pci-non-transitional: modern-only
> > > > >> - Supports both Conventional PCI and PCI Express buses
> > > > >>
> > > > >> The existing TYPE_* macros for these types will point to an
> > > > >> abstract base type, so existing casts in the code will keep
> > > > >> working for all variants.
> > > > >>
> > > > >> A simple test script (tests/acceptance/virtio_version.py) is
> > > > >> included, to check if the new device types are equivalent to
> > > > >> using the `disable-legacy` and `disable-modern` options.
> > > > >>
> > > > >> Acked-by: Andrea Bolognani <abologna@redhat.com>
> > > > >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > >
> > > > > Hi Eduardo,
> > > > >
> > > > > with these new devices, I can trigger an abort on s390x:
> > > > >
> > > > > $ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
> > > > > QEMU 3.1.50 monitor - type 'help' for more information
> > > > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > > > qemu-system-s390x: hw/core/qdev-properties.c:1236:
> > > > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > > > Aborted (core dumped)
> > > > FWIW, it happens with x86, too:
> > > >
> > > > $ qemu-system-x86_64 -M pc-i440fx-2.6 -monitor stdio
> > > > QEMU 3.1.50 monitor - type 'help' for more information
> > > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > > qemu-system-x86_64: hw/core/qdev-properties.c:1236:
> > > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > > Aborted (core dumped)
> > > >
> > > > Only machine types newer than 2.7 seem to be OK.
> > >
> > > It also fails for vhost-scsi-pci-transitional on 2.6 and older machines.
> > >
> > > The problem is that HW_COMPAT_2_6 tries to set the disable_modern
> > > property, which the version-specific variants of virtio-pci do not have.
> > >
> > > Not sure how to fix this. The version-specific devices cannot really
> > > work on the compat machine.
> >
> > Oops.
> >
> > My patch broke the assumption that every virtio-pci subclass is a
> > generic legacy/modern device. The good news is that HW_COMPAT_2_6
> > needs to affect only devices that existed in QEMU
> > 2.6 (not every subclass of virtio-pci).
> >
> > I see 3 possible ways to address this:
> >
> > 1) Making disable-legacy and disable-modern available on all
> > virtio-pci subclasses again.
> >
> > This is simple to implement, but I would like to avoid that. I'd
> > prefer to keep the legacy/modern logic complexity restricted to
> > the old generic virtio devices.
>
> As it's only for compat, we can probably use an "x-" property.
I'm not sure that makes it much better, though.
> > 2) Creating a virtio-pci-generic or virtio-pci-hybrid interface
> > type, add it to generic_type_info at virtio_pci_types_register(),
> > and use it on HW_COMPAT_2_6 instead of "virtio-pci".
> >
> > This is simple to implement, but I'm wary. I'm already bothered
> > by the complexity of our PCI and virtio type hierarchies. We
> > have multiple overlapping sets of virtio-pci devices (depending
> > which way you look), and specifying exactly which set we want to
> > affect is tricky.
I think the various virtio-pci types are already confusing enough
without an extra interface type.
> I'm not sure I understand this one. Would this be required
> anyway if we added a new feature to both transitional and modern
> devices?
I thought new features would need to be added to the base class anyway?
>
> >
> > 3) Replacing "virtio-pci" on HW_COMPAT_2_6 with an explicit list
> > of concrete virtio-pci types that existed when QEMU 2.6 was
> > released. This will make HW_COMPAT_2_6 grow, but reduce
> > complexity of the whole system. I'm inclined to implement this
> > solution.
This solution looks best to me as well.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-05 19:57 [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
2018-12-05 19:57 ` [Qemu-devel] [PATCH for-4.0 v4 1/2] virtio: Helper for registering virtio device types Eduardo Habkost
2018-12-05 19:57 ` [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
@ 2018-12-05 21:42 ` no-reply
2018-12-12 1:18 ` [Qemu-devel] " Eduardo Habkost
2019-03-05 12:09 ` Andrea Bolognani
4 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2018-12-05 21:42 UTC (permalink / raw)
To: ehabkost; +Cc: famz, qemu-devel, mst, kwolf
Patchew URL: https://patchew.org/QEMU/20181205195704.17605-1-ehabkost@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20181205195704.17605-1-ehabkost@redhat.com
Subject: [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
486a758 virtio: Provide version-specific variants of virtio PCI devices
85361d9 virtio: Helper for registering virtio device types
=== OUTPUT BEGIN ===
Checking PATCH 1/2: virtio: Helper for registering virtio device types...
WARNING: line over 80 characters
#496: FILE: hw/virtio/virtio-pci.h:443:
+ * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE,
WARNING: line over 80 characters
#505: FILE: hw/virtio/virtio-pci.h:452:
+ * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE.
total: 0 errors, 2 warnings, 469 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/2: virtio: Provide version-specific variants of virtio PCI devices...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#316:
new file mode 100644
ERROR: line over 90 characters
#372: FILE: tests/acceptance/virtio_version.py:52:
+ return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)]
WARNING: line over 80 characters
#427: FILE: tests/acceptance/virtio_version.py:107:
+ dev_1_0, nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype))
WARNING: line over 80 characters
#451: FILE: tests/acceptance/virtio_version.py:131:
+ dev_trans, trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype))
total: 1 errors, 3 warnings, 404 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20181205195704.17605-1-ehabkost@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-05 19:57 [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
` (2 preceding siblings ...)
2018-12-05 21:42 ` [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] " no-reply
@ 2018-12-12 1:18 ` Eduardo Habkost
2018-12-12 1:22 ` Michael S. Tsirkin
2019-03-05 12:09 ` Andrea Bolognani
4 siblings, 1 reply; 20+ messages in thread
From: Eduardo Habkost @ 2018-12-12 1:18 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin
Cc: Gonglei, Paolo Bonzini, Stefan Hajnoczi, Amit Shah,
Andrea Bolognani, Cleber Rosa, Marcel Apfelbaum, Fam Zheng,
Cornelia Huck, Kevin Wolf, Max Reitz, libvir-list,
Wainer dos Santos Moschetta, Philippe Mathieu-Daudé,
Markus Armbruster, Laine Stump, Jason Wang, Gerd Hoffmann,
Daniel P. Berrangé,
Caio Carrara
Friendly ping. 3.1.0 is tagged now, so there's anything else
blocking this series?
On Wed, Dec 05, 2018 at 05:57:02PM -0200, Eduardo Habkost wrote:
> Existing modern-only device types are not being touched by v3, as
> they don't need separate variants. However, I plan to implement
> separate cleanups in the code that calls virtio_pci_force_virtio_1(),
> first, and then propose additional changes (e.g. deprecating
> disable-legacy and disable-modern in those device types).
>
> Changes v3 -> v4:
> * Trivial comment fixes (Cornelia Huck)
> * Test code update (Caio Carrara)
>
> Changes v2 -> v3:
> * Split into two separate patches (type registration helper
> and introduction of new types)
> * Rewrote virtio_pci_types_register() completely:
> * Replaced magic generation of type names with explicit fields in
> VirtioPCIDeviceTypeInfo
> * Removed modern_only field (not necessary anymore)
> * Don't register a separate base type unless necessary
>
> Changes v1 -> v2:
> * Removed *-0.9 devices. Nobody will want to use them, if
> transitional devices work with legacy drivers
> (Gerd Hoffmann, Michael S. Tsirkin)
> * Drop virtio version from name: rename -1.0-transitional to
> -transitional (Michael S. Tsirkin)
> * Renamed -1.0 to -non-transitional
> * Don't add any extra variants to modern-only device types
> (they don't need it)
> * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
> Cornelia Huck)
> * No need to change cast macros for modern-only devices
> * Rename virtio_register_types() to virtio_pci_types_register()
>
> Original patch description:
>
> Many of the current virtio-*-pci device types actually represent
> 3 different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With this multi-purpose device
> type, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> * virtio-*-pci: the existing multi-purpose device types
> * virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> * virtio-*-pci-non-transitional: modern-only
>
> Reference to previous discussion that originated this idea:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
>
> Eduardo Habkost (2):
> virtio: Helper for registering virtio device types
> virtio: Provide version-specific variants of virtio PCI devices
>
> hw/virtio/virtio-pci.h | 78 +++++++--
> hw/display/virtio-gpu-pci.c | 7 +-
> hw/display/virtio-vga.c | 7 +-
> hw/virtio/virtio-crypto-pci.c | 7 +-
> hw/virtio/virtio-pci.c | 267 ++++++++++++++++++++++-------
> tests/acceptance/virtio_version.py | 176 +++++++++++++++++++
> 6 files changed, 452 insertions(+), 90 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
>
> --
> 2.18.0.rc1.1.g3f1ff2140
>
--
Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-12 1:18 ` [Qemu-devel] " Eduardo Habkost
@ 2018-12-12 1:22 ` Michael S. Tsirkin
0 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2018-12-12 1:22 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Gonglei, Paolo Bonzini, Stefan Hajnoczi, Amit Shah,
Andrea Bolognani, Cleber Rosa, Marcel Apfelbaum, Fam Zheng,
Cornelia Huck, Kevin Wolf, Max Reitz, libvir-list,
Wainer dos Santos Moschetta, Philippe Mathieu-Daudé,
Markus Armbruster, Laine Stump, Jason Wang, Gerd Hoffmann,
Daniel P. Berrangé,
Caio Carrara
Nothing, I'm packing up the 1st pull request.
On Tue, Dec 11, 2018 at 11:18:51PM -0200, Eduardo Habkost wrote:
> Friendly ping. 3.1.0 is tagged now, so there's anything else
> blocking this series?
>
>
> On Wed, Dec 05, 2018 at 05:57:02PM -0200, Eduardo Habkost wrote:
> > Existing modern-only device types are not being touched by v3, as
> > they don't need separate variants. However, I plan to implement
> > separate cleanups in the code that calls virtio_pci_force_virtio_1(),
> > first, and then propose additional changes (e.g. deprecating
> > disable-legacy and disable-modern in those device types).
> >
> > Changes v3 -> v4:
> > * Trivial comment fixes (Cornelia Huck)
> > * Test code update (Caio Carrara)
> >
> > Changes v2 -> v3:
> > * Split into two separate patches (type registration helper
> > and introduction of new types)
> > * Rewrote virtio_pci_types_register() completely:
> > * Replaced magic generation of type names with explicit fields in
> > VirtioPCIDeviceTypeInfo
> > * Removed modern_only field (not necessary anymore)
> > * Don't register a separate base type unless necessary
> >
> > Changes v1 -> v2:
> > * Removed *-0.9 devices. Nobody will want to use them, if
> > transitional devices work with legacy drivers
> > (Gerd Hoffmann, Michael S. Tsirkin)
> > * Drop virtio version from name: rename -1.0-transitional to
> > -transitional (Michael S. Tsirkin)
> > * Renamed -1.0 to -non-transitional
> > * Don't add any extra variants to modern-only device types
> > (they don't need it)
> > * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
> > Cornelia Huck)
> > * No need to change cast macros for modern-only devices
> > * Rename virtio_register_types() to virtio_pci_types_register()
> >
> > Original patch description:
> >
> > Many of the current virtio-*-pci device types actually represent
> > 3 different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > That would be just an annoyance if it didn't break our device/bus
> > compatibility QMP interfaces. With this multi-purpose device
> > type, there's no way to tell management software that
> > transitional devices and legacy devices require a Conventional
> > PCI bus.
> >
> > The multi-purpose device types would also prevent us from telling
> > management software what's the PCI vendor/device ID for them,
> > because their PCI IDs change at runtime depending on the bus
> > where they were plugged.
> >
> > This patch adds separate device types for each of those virtio
> > device flavors:
> >
> > * virtio-*-pci: the existing multi-purpose device types
> > * virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > * virtio-*-pci-non-transitional: modern-only
> >
> > Reference to previous discussion that originated this idea:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> >
> > Eduardo Habkost (2):
> > virtio: Helper for registering virtio device types
> > virtio: Provide version-specific variants of virtio PCI devices
> >
> > hw/virtio/virtio-pci.h | 78 +++++++--
> > hw/display/virtio-gpu-pci.c | 7 +-
> > hw/display/virtio-vga.c | 7 +-
> > hw/virtio/virtio-crypto-pci.c | 7 +-
> > hw/virtio/virtio-pci.c | 267 ++++++++++++++++++++++-------
> > tests/acceptance/virtio_version.py | 176 +++++++++++++++++++
> > 6 files changed, 452 insertions(+), 90 deletions(-)
> > create mode 100644 tests/acceptance/virtio_version.py
> >
> > --
> > 2.18.0.rc1.1.g3f1ff2140
> >
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2018-12-05 19:57 [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
` (3 preceding siblings ...)
2018-12-12 1:18 ` [Qemu-devel] " Eduardo Habkost
@ 2019-03-05 12:09 ` Andrea Bolognani
2019-03-05 14:38 ` Gerd Hoffmann
4 siblings, 1 reply; 20+ messages in thread
From: Andrea Bolognani @ 2019-03-05 12:09 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel, Michael S. Tsirkin
Cc: Kevin Wolf, Amit Shah, libvir-list, Markus Armbruster,
Jason Wang, Cornelia Huck, Wainer dos Santos Moschetta,
Max Reitz, Caio Carrara, Gonglei, Laine Stump, Gerd Hoffmann,
Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini,
Philippe Mathieu-Daudé,
Cole Robinson, Daniel Berrange
Sorry to resurrect such an old thread, but I have been wondering...
On Wed, 2018-12-05 at 17:57 -0200, Eduardo Habkost wrote:
[...]
> Changes v1 -> v2:
> * Removed *-0.9 devices. Nobody will want to use them, if
> transitional devices work with legacy drivers
> (Gerd Hoffmann, Michael S. Tsirkin)
> * Drop virtio version from name: rename -1.0-transitional to
> -transitional (Michael S. Tsirkin)
> * Renamed -1.0 to -non-transitional
> * Don't add any extra variants to modern-only device types
> (they don't need it)
... if doing this was a good idea after all?
While I understand that something like virtio-gpu, which supports
the 1.0 specification exclusively, only really needs to have a
single device associated with it from the functionality point of
view, looking at it from a user's perspective it seems to me like
providing an explicit non-transitional variant would be appropriate
for consistency reasons, so that your guest could look like
-device virtio-blk-pci-non-transitional \
-device virtio-net-pci-non-transitional \
-device virtio-gpu-pci-non-transitional \
and you wouldn't have to question why you can use the
non-transitional variant for pretty much everything, except for the
few cases where you can't - for no apparent reason...
It would also signal quite clearly which devices support both
transitional and non-transitional variants and which ones don't,
without having to infer that the complete lack of (non-)transitional
variants means that only the non-transitional variant is available -
except you have to use the suffix-less device name to use it.
tl;dr providing the non-transitional variant for virtio 1.0-only
devices would make using this much more user-friendly.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2019-03-05 12:09 ` Andrea Bolognani
@ 2019-03-05 14:38 ` Gerd Hoffmann
2019-03-05 15:56 ` Andrea Bolognani
0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2019-03-05 14:38 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Eduardo Habkost, qemu-devel, Michael S. Tsirkin, Kevin Wolf,
Amit Shah, libvir-list, Markus Armbruster, Jason Wang,
Cornelia Huck, Wainer dos Santos Moschetta, Max Reitz,
Caio Carrara, Gonglei, Laine Stump, Stefan Hajnoczi, Cleber Rosa,
Paolo Bonzini, Philippe Mathieu-Daudé,
Cole Robinson, Daniel Berrange
Hi,
> -device virtio-blk-pci-non-transitional \
> -device virtio-net-pci-non-transitional \
> -device virtio-gpu-pci-non-transitional \
>
> and you wouldn't have to question why you can use the
> non-transitional variant for pretty much everything, except for the
> few cases where you can't - for no apparent reason...
Well, there are no variants, only a single virtio-$foo-pci* device. So
you don't have to worry about picking one of the available variants,
there is no choice in the first place.
When adding an virtio-gpu-pci-non-transitional variant we'll create
confusion too, because it wouldn't be a real variant. We would have two
100% identical devices then, and people will probably wonder why they
exist and what the difference is ...
So I can't see how this would be so much better. We have to document
the mess no matter what.
cheers,
Gerd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2019-03-05 14:38 ` Gerd Hoffmann
@ 2019-03-05 15:56 ` Andrea Bolognani
2019-03-06 7:41 ` [Qemu-devel] [libvirt] " Peter Krempa
0 siblings, 1 reply; 20+ messages in thread
From: Andrea Bolognani @ 2019-03-05 15:56 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Eduardo Habkost, qemu-devel, Michael S. Tsirkin, Kevin Wolf,
Amit Shah, libvir-list, Markus Armbruster, Jason Wang,
Cornelia Huck, Wainer dos Santos Moschetta, Max Reitz,
Caio Carrara, Gonglei, Laine Stump, Stefan Hajnoczi, Cleber Rosa,
Paolo Bonzini, Philippe Mathieu-Daudé,
Cole Robinson, Daniel Berrange
On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
> Hi,
>
> > -device virtio-blk-pci-non-transitional \
> > -device virtio-net-pci-non-transitional \
> > -device virtio-gpu-pci-non-transitional \
> >
> > and you wouldn't have to question why you can use the
> > non-transitional variant for pretty much everything, except for the
> > few cases where you can't - for no apparent reason...
>
> Well, there are no variants, only a single virtio-$foo-pci* device. So
> you don't have to worry about picking one of the available variants,
> there is no choice in the first place.
>
> When adding an virtio-gpu-pci-non-transitional variant we'll create
> confusion too, because it wouldn't be a real variant. We would have two
> 100% identical devices then, and people will probably wonder why they
> exist and what the difference is ...
When looking at a single device, I mostly agree with your assessment;
however, when looking at the overall situation with VirtIO devices,
one might quite reasonably infer the following rules:
* devices marked as (non-)transitional are going to show up as
(non-)transitional;
* unmarked devices might show up as either one, depending on some
factor which is not immediately obvious.
So if you knew you wanted non-transitional devices you would expect
to just use the non-transitional variant for *all* VirtIO devices,
including virtio-gpu, without necessarily caring whether the unmarked
devices behaves any differently; if you tried to use the transitional
device, you'd get an error message telling you that device doesn't
exist, which is pretty reasonable and easy to research / understand.
With the current situation, once you've tried using non-transitional
virtio-gpu and gotten back an error message, there's quite a bit more
digging required to figure out *why* the device is not there in the
first place.
So I agree neither scenario is exactly perfect, but I still think
adding non-transitional alias devices would overall be more
user-friendly.
> So I can't see how this would be so much better. We have to document
> the mess no matter what.
We have some documentation in libvirt:
https://libvirt.org/formatdomain.html#elementsVirtioTransitional
Not that more / improved documentation is ever a bad idea :)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2019-03-05 15:56 ` Andrea Bolognani
@ 2019-03-06 7:41 ` Peter Krempa
2019-03-06 8:30 ` Ján Tomko
0 siblings, 1 reply; 20+ messages in thread
From: Peter Krempa @ 2019-03-06 7:41 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Gerd Hoffmann, Kevin Wolf, Cornelia Huck, Eduardo Habkost,
Michael S. Tsirkin, libvir-list, Max, Jason Wang, Amit Shah,
Stefan Hajnoczi, qemu-devel, Wainer dos Santos Moschetta,
Cleber Rosa, Gonglei, Laine Stump, Caio Carrara, Paolo Bonzini,
Reitz, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 718 bytes --]
On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
[...]
> So I agree neither scenario is exactly perfect, but I still think
> adding non-transitional alias devices would overall be more
> user-friendly.
I don't think it makes sense to add it at the qemu level. From libvirt's
point of view users should be shielded from any qemu impl detail or
inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
devices would be the same and thus does not make sense to do that
because it would be more confusing.
You can argue that we should add the alias at the libvirt level though.
[1] Yes. I'm aware that statement is quite ironical.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2019-03-06 7:41 ` [Qemu-devel] [libvirt] " Peter Krempa
@ 2019-03-06 8:30 ` Ján Tomko
2019-03-06 9:08 ` Andrea Bolognani
2019-03-06 9:10 ` Daniel P. Berrangé
0 siblings, 2 replies; 20+ messages in thread
From: Ján Tomko @ 2019-03-06 8:30 UTC (permalink / raw)
To: Peter Krempa
Cc: Andrea Bolognani, Kevin Wolf, Eduardo Habkost,
Michael S. Tsirkin, libvir-list, Jason Wang, Cornelia Huck,
qemu-devel, Amit Shah, Reitz, Max, Caio Carrara, Gonglei,
Laine Stump, Gerd Hoffmann, Stefan Hajnoczi, Cleber Rosa,
Paolo Bonzini, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta
[-- Attachment #1: Type: text/plain, Size: 944 bytes --]
On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:
>On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
>> On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
>
>[...]
>
>> So I agree neither scenario is exactly perfect, but I still think
>> adding non-transitional alias devices would overall be more
>> user-friendly.
>
>I don't think it makes sense to add it at the qemu level. From libvirt's
>point of view users should be shielded from any qemu impl detail or
>inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
>devices would be the same and thus does not make sense to do that
>because it would be more confusing.
>
>You can argue that we should add the alias at the libvirt level though.
>
You can, but please don't.
Jano
>[1] Yes. I'm aware that statement is quite ironical.
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2019-03-06 8:30 ` Ján Tomko
@ 2019-03-06 9:08 ` Andrea Bolognani
2019-03-06 9:10 ` Daniel P. Berrangé
1 sibling, 0 replies; 20+ messages in thread
From: Andrea Bolognani @ 2019-03-06 9:08 UTC (permalink / raw)
To: Ján Tomko, Peter Krempa
Cc: Kevin Wolf, Eduardo Habkost, Michael S. Tsirkin, libvir-list,
Jason Wang, Cornelia Huck, qemu-devel, Amit Shah, Reitz, Max,
Caio Carrara, Gonglei, Laine Stump, Gerd Hoffmann,
Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta
On Wed, 2019-03-06 at 09:30 +0100, Ján Tomko wrote:
> On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:
> > On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> > > So I agree neither scenario is exactly perfect, but I still think
> > > adding non-transitional alias devices would overall be more
> > > user-friendly.
> >
> > I don't think it makes sense to add it at the qemu level. From libvirt's
> > point of view users should be shielded from any qemu impl detail or
> > inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
> > devices would be the same and thus does not make sense to do that
> > because it would be more confusing.
> >
> > You can argue that we should add the alias at the libvirt level though.
>
> You can, but please don't.
It would seem nobody except me thinks this is a good idea, so I
guess I'll just drop it now.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
2019-03-06 8:30 ` Ján Tomko
2019-03-06 9:08 ` Andrea Bolognani
@ 2019-03-06 9:10 ` Daniel P. Berrangé
1 sibling, 0 replies; 20+ messages in thread
From: Daniel P. Berrangé @ 2019-03-06 9:10 UTC (permalink / raw)
To: Ján Tomko
Cc: Peter Krempa, Kevin Wolf, Eduardo Habkost, Michael S. Tsirkin,
libvir-list, Jason Wang, Cornelia Huck, Stefan Hajnoczi,
Andrea Bolognani, Amit Shah, qemu-devel, Max, Cleber Rosa,
Gonglei, Gerd Hoffmann, Laine Stump, Caio Carrara, Paolo Bonzini,
Reitz, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta
On Wed, Mar 06, 2019 at 09:30:32AM +0100, Ján Tomko wrote:
> On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:
> > On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> > > On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
> >
> > [...]
> >
> > > So I agree neither scenario is exactly perfect, but I still think
> > > adding non-transitional alias devices would overall be more
> > > user-friendly.
> >
> > I don't think it makes sense to add it at the qemu level. From libvirt's
> > point of view users should be shielded from any qemu impl detail or
> > inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
> > devices would be the same and thus does not make sense to do that
> > because it would be more confusing.
> >
> > You can argue that we should add the alias at the libvirt level though.
> >
>
> You can, but please don't.
Indeed, at the libvirt level we've always tried to take the view that
there should be one way to expressing each concept/feature. Adding
new names / xml elements that duplicate existing supported concepts
to make things "consistent" is a slippery slope becasue there are
100's of places to which that can apply when you have hindsight.
It is not going to make a significant difference to how "user friendly"
libvirt is - that is not a core goal in its own right at the API / XML
schema level. It is can be a factor in deciding between multiple competing
designs when first adding a feature, but it isn't a reason to add duplication
in the API / XML.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 20+ messages in thread