* [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
@ 2018-10-13 2:54 Eduardo Habkost
2018-10-14 21:35 ` Michael S. Tsirkin
` (3 more replies)
0 siblings, 4 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-13 2:54 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara, Gerd Hoffmann
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 is 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-0.9: legacy virtio device
- Supports Conventional PCI buses only, because
it has a PIO BAR
- virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
- Supports Conventional PCI buses only, because
it has a PIO BAR
- virtio-*-pci-1.0: modern-only
- Supports both Conventional PCI and PCI Express buses
All the types above will inherit from an abstract
"virtio-*-pci-base" type, so existing code that doesn't care
about the virtio version can use "virtio-*-pci-base" on type
casts.
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.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Reference to previous discussion that originated this idea:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
---
hw/virtio/virtio-pci.h | 93 +++++++++---
hw/display/virtio-gpu-pci.c | 8 +-
hw/display/virtio-vga.c | 11 +-
hw/virtio/virtio-crypto-pci.c | 8 +-
hw/virtio/virtio-pci.c | 225 +++++++++++++++++++++--------
tests/acceptance/virtio_version.py | 138 ++++++++++++++++++
6 files changed, 390 insertions(+), 93 deletions(-)
create mode 100644 tests/acceptance/virtio_version.py
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 813082b0d7..f1cfb60277 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -216,7 +216,8 @@ 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_PREFIX "virtio-scsi-pci"
+#define TYPE_VIRTIO_SCSI_PCI (TYPE_VIRTIO_SCSI_PCI_PREFIX "-base")
#define VIRTIO_SCSI_PCI(obj) \
OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
@@ -229,7 +230,8 @@ struct VirtIOSCSIPCI {
/*
* vhost-scsi-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
+#define TYPE_VHOST_SCSI_PCI_PREFIX "vhost-scsi-pci"
+#define TYPE_VHOST_SCSI_PCI (TYPE_VHOST_SCSI_PCI_PREFIX "-base")
#define VHOST_SCSI_PCI(obj) \
OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
@@ -239,7 +241,8 @@ struct VHostSCSIPCI {
};
#endif
-#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
+#define TYPE_VHOST_USER_SCSI_PCI_PREFIX "vhost-user-scsi-pci"
+#define TYPE_VHOST_USER_SCSI_PCI (TYPE_VHOST_USER_SCSI_PCI_PREFIX "-base")
#define VHOST_USER_SCSI_PCI(obj) \
OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
@@ -252,7 +255,8 @@ 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_PREFIX "vhost-user-blk-pci"
+#define TYPE_VHOST_USER_BLK_PCI (TYPE_VHOST_USER_BLK_PCI_PREFIX "-base")
#define VHOST_USER_BLK_PCI(obj) \
OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
@@ -265,7 +269,8 @@ struct VHostUserBlkPCI {
/*
* virtio-blk-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
+#define TYPE_VIRTIO_BLK_PCI_PREFIX "virtio-blk-pci"
+#define TYPE_VIRTIO_BLK_PCI (TYPE_VIRTIO_BLK_PCI_PREFIX "-base")
#define VIRTIO_BLK_PCI(obj) \
OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
@@ -277,7 +282,8 @@ struct VirtIOBlkPCI {
/*
* virtio-balloon-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci"
+#define TYPE_VIRTIO_BALLOON_PCI_PREFIX "virtio-balloon-pci"
+#define TYPE_VIRTIO_BALLOON_PCI (TYPE_VIRTIO_BALLOON_PCI_PREFIX "-base")
#define VIRTIO_BALLOON_PCI(obj) \
OBJECT_CHECK(VirtIOBalloonPCI, (obj), TYPE_VIRTIO_BALLOON_PCI)
@@ -289,7 +295,8 @@ struct VirtIOBalloonPCI {
/*
* virtio-serial-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci"
+#define TYPE_VIRTIO_SERIAL_PCI_PREFIX "virtio-serial-pci"
+#define TYPE_VIRTIO_SERIAL_PCI (TYPE_VIRTIO_SERIAL_PCI_PREFIX "-base")
#define VIRTIO_SERIAL_PCI(obj) \
OBJECT_CHECK(VirtIOSerialPCI, (obj), TYPE_VIRTIO_SERIAL_PCI)
@@ -301,7 +308,8 @@ struct VirtIOSerialPCI {
/*
* virtio-net-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_NET_PCI "virtio-net-pci"
+#define TYPE_VIRTIO_NET_PCI_PREFIX "virtio-net-pci"
+#define TYPE_VIRTIO_NET_PCI (TYPE_VIRTIO_NET_PCI_PREFIX "-base")
#define VIRTIO_NET_PCI(obj) \
OBJECT_CHECK(VirtIONetPCI, (obj), TYPE_VIRTIO_NET_PCI)
@@ -316,7 +324,8 @@ struct VirtIONetPCI {
#ifdef CONFIG_VIRTFS
-#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci"
+#define TYPE_VIRTIO_9P_PCI_PREFIX "virtio-9p-pci"
+#define TYPE_VIRTIO_9P_PCI (TYPE_VIRTIO_9P_PCI_PREFIX "-base")
#define VIRTIO_9P_PCI(obj) \
OBJECT_CHECK(V9fsPCIState, (obj), TYPE_VIRTIO_9P_PCI)
@@ -330,7 +339,8 @@ typedef struct V9fsPCIState {
/*
* virtio-rng-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci"
+#define TYPE_VIRTIO_RNG_PCI_PREFIX "virtio-rng-pci"
+#define TYPE_VIRTIO_RNG_PCI (TYPE_VIRTIO_RNG_PCI_PREFIX "-base")
#define VIRTIO_RNG_PCI(obj) \
OBJECT_CHECK(VirtIORngPCI, (obj), TYPE_VIRTIO_RNG_PCI)
@@ -352,9 +362,13 @@ struct VirtIOInputPCI {
};
#define TYPE_VIRTIO_INPUT_HID_PCI "virtio-input-hid-pci"
-#define TYPE_VIRTIO_KEYBOARD_PCI "virtio-keyboard-pci"
-#define TYPE_VIRTIO_MOUSE_PCI "virtio-mouse-pci"
-#define TYPE_VIRTIO_TABLET_PCI "virtio-tablet-pci"
+
+#define TYPE_VIRTIO_KEYBOARD_PCI_PREFIX "virtio-keyboard-pci"
+#define TYPE_VIRTIO_KEYBOARD_PCI (TYPE_VIRTIO_KEYBOARD_PCI_PREFIX "-base")
+#define TYPE_VIRTIO_MOUSE_PCI_PREFIX "virtio-mouse-pci"
+#define TYPE_VIRTIO_MOUSE_PCI (TYPE_VIRTIO_MOUSE_PCI_PREFIX "-base")
+#define TYPE_VIRTIO_TABLET_PCI_PREFIX "virtio-tablet-pci"
+#define TYPE_VIRTIO_TABLET_PCI (TYPE_VIRTIO_TABLET_PCI_PREFIX "-base")
#define VIRTIO_INPUT_HID_PCI(obj) \
OBJECT_CHECK(VirtIOInputHIDPCI, (obj), TYPE_VIRTIO_INPUT_HID_PCI)
@@ -365,7 +379,8 @@ struct VirtIOInputHIDPCI {
#ifdef CONFIG_LINUX
-#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci"
+#define TYPE_VIRTIO_INPUT_HOST_PCI_PREFIX "virtio-input-host-pci"
+#define TYPE_VIRTIO_INPUT_HOST_PCI (TYPE_VIRTIO_INPUT_HOST "-base")
#define VIRTIO_INPUT_HOST_PCI(obj) \
OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
@@ -379,7 +394,8 @@ struct VirtIOInputHostPCI {
/*
* virtio-gpu-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
+#define TYPE_VIRTIO_GPU_PCI_PREFIX "virtio-gpu-pci"
+#define TYPE_VIRTIO_GPU_PCI (TYPE_VIRTIO_GPU_PCI_PREFIX "-base")
#define VIRTIO_GPU_PCI(obj) \
OBJECT_CHECK(VirtIOGPUPCI, (obj), TYPE_VIRTIO_GPU_PCI)
@@ -392,7 +408,8 @@ struct VirtIOGPUPCI {
/*
* vhost-vsock-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci"
+#define TYPE_VHOST_VSOCK_PCI_PREFIX "vhost-vsock-pci"
+#define TYPE_VHOST_VSOCK_PCI (TYPE_VHOST_VSOCK_PCI_PREFIX "-base")
#define VHOST_VSOCK_PCI(obj) \
OBJECT_CHECK(VHostVSockPCI, (obj), TYPE_VHOST_VSOCK_PCI)
@@ -405,7 +422,8 @@ struct VHostVSockPCI {
/*
* virtio-crypto-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_CRYPTO_PCI "virtio-crypto-pci"
+#define TYPE_VIRTIO_CRYPTO_PCI_PREFIX "virtio-crypto-pci"
+#define TYPE_VIRTIO_CRYPTO_PCI (TYPE_VIRTIO_CRYPTO_PCI_PREFIX "-base")
#define VIRTIO_CRYPTO_PCI(obj) \
OBJECT_CHECK(VirtIOCryptoPCI, (obj), TYPE_VIRTIO_CRYPTO_PCI)
@@ -417,4 +435,45 @@ struct VirtIOCryptoPCI {
/* Virtio ABI version, if we increment this, we break the guest driver. */
#define VIRTIO_PCI_ABI_VERSION 0
+/**
+ * VirtioPCIDeviceTypeInfo:
+ *
+ * Template for virtio PCI device types. See virtio_register_types()
+ * for details.
+ */
+typedef struct VirtioPCIDeviceTypeInfo {
+ /* Prefix for the subclass names */
+ const char *name_prefix;
+ /* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
+ const char *parent;
+ /*
+ * If modern_only is true, only <name_prefix> and <name_prefix>-1.0
+ * types will be registered.
+ */
+ bool modern_only;
+
+ /* Same as TypeInfo fields: */
+ size_t instance_size;
+ void (*instance_init)(Object *obj);
+ void (*class_init)(ObjectClass *klass, void *data);
+} VirtioPCIDeviceTypeInfo;
+
+/*
+ * Register virtio-pci types. Each virtio-pci device type will be available
+ * in 4 flavors:
+ * - <name_prefix>-0.9: legacy virtio devices
+ * - Supports Conventional PCI buses only
+ * - <name_prefix>-1.0-transitional: modern device supporting legacy drivers
+ * - Supports Conventional PCI buses only
+ * - <name_prefix>-1.0: modern-only
+ * - Supports Conventional PCI and PCI Express buses
+ * - <name_prefix>: virtio version configurable, legacy driver support
+ * automatically selected when device is plugged
+ * - Supports Conventional PCI and PCI Express buses
+ *
+ * All the types above will inherit from "<name_prefix>-base", so generic
+ * code can use "<name_prefix>-base" on type casts.
+ */
+void virtio_register_types(const VirtioPCIDeviceTypeInfo *t);
+
#endif
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index cece4aa495..a0fb6e46da 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -69,9 +69,9 @@ 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 = {
+ .name_prefix = TYPE_VIRTIO_GPU_PCI_PREFIX,
+ .modern_only = true,
.instance_size = sizeof(VirtIOGPUPCI),
.instance_init = virtio_gpu_initfn,
.class_init = virtio_gpu_pci_class_init,
@@ -79,6 +79,6 @@ static const TypeInfo virtio_gpu_pci_info = {
static void virtio_gpu_pci_register_types(void)
{
- type_register_static(&virtio_gpu_pci_info);
+ virtio_register_types(&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..07d6e4b247 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -8,7 +8,8 @@
/*
* virtio-vga: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_VGA "virtio-vga"
+#define TYPE_VIRTIO_VGA_PREFIX "virtio-vga"
+#define TYPE_VIRTIO_VGA (TYPE_VIRTIO_VGA_PREFIX "-base")
#define VIRTIO_VGA(obj) \
OBJECT_CHECK(VirtIOVGA, (obj), TYPE_VIRTIO_VGA)
@@ -207,9 +208,9 @@ 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 = {
+ .name_prefix = TYPE_VIRTIO_VGA_PREFIX,
+ .modern_only = true,
.instance_size = sizeof(struct VirtIOVGA),
.instance_init = virtio_vga_inst_initfn,
.class_init = virtio_vga_class_init,
@@ -217,7 +218,7 @@ static TypeInfo virtio_vga_info = {
static void virtio_vga_register_types(void)
{
- type_register_static(&virtio_vga_info);
+ virtio_register_types(&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..1de0f88c51 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -64,9 +64,9 @@ 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 = {
+ .name_prefix = TYPE_VIRTIO_CRYPTO_PCI_PREFIX,
+ .modern_only = true,
.instance_size = sizeof(VirtIOCryptoPCI),
.instance_init = virtio_crypto_initfn,
.class_init = virtio_crypto_pci_class_init,
@@ -74,6 +74,6 @@ static const TypeInfo virtio_crypto_pci_info = {
static void virtio_crypto_pci_register_types(void)
{
- type_register_static(&virtio_crypto_pci_info);
+ virtio_register_types(&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 3a01fe90f0..6e2141e009 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 = {
+ .name_prefix = TYPE_VIRTIO_9P_PCI_PREFIX,
.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,
@@ -1940,12 +1936,119 @@ static const TypeInfo virtio_pci_info = {
.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_generic_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->props = virtio_pci_generic_properties;
+}
+
+static void virtio_pci_0_9_instance_init(Object *obj)
+{
+ VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
+
+ proxy->disable_legacy = ON_OFF_AUTO_OFF;
+ proxy->disable_modern = true;
+}
+
+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_1_0_instance_init(Object *obj)
+{
+ VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
+
+ proxy->disable_legacy = ON_OFF_AUTO_ON;
+ proxy->disable_modern = false;
+}
+
+
+void virtio_register_types(const VirtioPCIDeviceTypeInfo *t)
+{
+ const TypeInfo base_type_info = {
+ .name = g_strdup_printf("%s-base", t->name_prefix),
+ .parent = t->parent ? t->parent : TYPE_VIRTIO_PCI,
+ .instance_size = t->instance_size,
+ .instance_init = t->instance_init,
+ .class_init = t->class_init,
+ .abstract = true,
+ };
+
+ const TypeInfo generic_type_info = {
+ .name = t->name_prefix,
+ .parent = base_type_info.name,
+ .class_init = virtio_pci_generic_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_PCIE_DEVICE },
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ },
+ };
+
+ const TypeInfo virtio_1_0_type_info = {
+ .name = g_strdup_printf("%s-1.0", t->name_prefix),
+ .parent = base_type_info.name,
+ .instance_init = virtio_pci_1_0_instance_init,
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_PCIE_DEVICE },
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ },
+ };
+
+ const TypeInfo virtio_1_0_transitional_type_info = {
+ .name = g_strdup_printf("%s-1.0-transitional", t->name_prefix),
+ .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 },
+ { }
+ },
+ };
+
+ const TypeInfo virtio_0_9_type_info = {
+ .name = g_strdup_printf("%s-0.9", t->name_prefix),
+ .parent = base_type_info.name,
+ .instance_init = virtio_pci_0_9_instance_init,
+ .interfaces = (InterfaceInfo[]) {
+ /*
+ * Transitional virtio devices work only as conventional PCI devices
+ * because they require PIO ports.
+ */
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ },
+ };
+
+ type_register(&base_type_info);
+ type_register(&generic_type_info);
+ type_register(&virtio_1_0_type_info);
+ if (!t->modern_only) {
+ type_register(&virtio_1_0_transitional_type_info);
+ type_register(&virtio_0_9_type_info);
+ }
+}
+
/* virtio-blk-pci */
static Property virtio_blk_pci_properties[] = {
@@ -1995,9 +2098,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 = {
+ .name_prefix = TYPE_VIRTIO_BLK_PCI_PREFIX,
.instance_size = sizeof(VirtIOBlkPCI),
.instance_init = virtio_blk_pci_instance_init,
.class_init = virtio_blk_pci_class_init,
@@ -2051,9 +2153,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 = {
+ .name_prefix = TYPE_VHOST_USER_BLK_PCI_PREFIX,
.instance_size = sizeof(VHostUserBlkPCI),
.instance_init = vhost_user_blk_pci_instance_init,
.class_init = vhost_user_blk_pci_class_init,
@@ -2119,9 +2220,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 = {
+ .name_prefix = TYPE_VIRTIO_SCSI_PCI_PREFIX,
.instance_size = sizeof(VirtIOSCSIPCI),
.instance_init = virtio_scsi_pci_instance_init,
.class_init = virtio_scsi_pci_class_init,
@@ -2174,9 +2274,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 = {
+ .name_prefix = TYPE_VHOST_SCSI_PCI_PREFIX,
.instance_size = sizeof(VHostSCSIPCI),
.instance_init = vhost_scsi_pci_instance_init,
.class_init = vhost_scsi_pci_class_init,
@@ -2229,9 +2328,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 = {
+ .name_prefix = TYPE_VHOST_USER_SCSI_PCI_PREFIX,
.instance_size = sizeof(VHostUserSCSIPCI),
.instance_init = vhost_user_scsi_pci_instance_init,
.class_init = vhost_user_scsi_pci_class_init,
@@ -2277,9 +2375,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 = {
+ .name_prefix = TYPE_VHOST_VSOCK_PCI_PREFIX,
.instance_size = sizeof(VHostVSockPCI),
.instance_init = vhost_vsock_pci_instance_init,
.class_init = vhost_vsock_pci_class_init,
@@ -2334,9 +2431,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 = {
+ .name_prefix = TYPE_VIRTIO_BALLOON_PCI_PREFIX,
.instance_size = sizeof(VirtIOBalloonPCI),
.instance_init = virtio_balloon_pci_instance_init,
.class_init = virtio_balloon_pci_class_init,
@@ -2407,9 +2503,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 = {
+ .name_prefix = TYPE_VIRTIO_SERIAL_PCI_PREFIX,
.instance_size = sizeof(VirtIOSerialPCI),
.instance_init = virtio_serial_pci_instance_init,
.class_init = virtio_serial_pci_class_init,
@@ -2462,9 +2557,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 = {
+ .name_prefix = TYPE_VIRTIO_NET_PCI_PREFIX,
.instance_size = sizeof(VirtIONetPCI),
.instance_init = virtio_net_pci_instance_init,
.class_init = virtio_net_pci_class_init,
@@ -2513,9 +2607,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 = {
+ .name_prefix = TYPE_VIRTIO_RNG_PCI_PREFIX,
.instance_size = sizeof(VirtIORngPCI),
.instance_init = virtio_rng_initfn,
.class_init = virtio_rng_pci_class_init,
@@ -2605,25 +2698,28 @@ 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 = {
+ .name_prefix = TYPE_VIRTIO_KEYBOARD_PCI_PREFIX,
.parent = TYPE_VIRTIO_INPUT_HID_PCI,
+ .modern_only = true,
.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 = {
+ .name_prefix = TYPE_VIRTIO_MOUSE_PCI_PREFIX,
.parent = TYPE_VIRTIO_INPUT_HID_PCI,
+ .modern_only = true,
.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 = {
+ .name_prefix = TYPE_VIRTIO_TABLET_PCI_PREFIX,
.parent = TYPE_VIRTIO_INPUT_HID_PCI,
+ .modern_only = true,
.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 = {
+ .name_prefix = TYPE_VIRTIO_INPUT_HOST_PCI_PREFIX,
.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_register_types(&virtio_rng_pci_info);
+ virtio_register_types(&virtio_keyboard_pci_info);
+ virtio_register_types(&virtio_mouse_pci_info);
+ virtio_register_types(&virtio_tablet_pci_info);
#ifdef CONFIG_LINUX
- type_register_static(&virtio_host_pci_info);
+ virtio_register_types(&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_register_types(&virtio_9p_pci_info);
#endif
- type_register_static(&virtio_blk_pci_info);
+ virtio_register_types(&virtio_blk_pci_info);
#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
- type_register_static(&vhost_user_blk_pci_info);
+ virtio_register_types(&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_register_types(&virtio_scsi_pci_info);
+ virtio_register_types(&virtio_balloon_pci_info);
+ virtio_register_types(&virtio_serial_pci_info);
+ virtio_register_types(&virtio_net_pci_info);
#ifdef CONFIG_VHOST_SCSI
- type_register_static(&vhost_scsi_pci_info);
+ virtio_register_types(&vhost_scsi_pci_info);
#endif
#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
- type_register_static(&vhost_user_scsi_pci_info);
+ virtio_register_types(&vhost_user_scsi_pci_info);
#endif
#ifdef CONFIG_VHOST_VSOCK
- type_register_static(&vhost_vsock_pci_info);
+ virtio_register_types(&vhost_vsock_pci_info);
#endif
}
diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py
new file mode 100644
index 0000000000..e1e0038530
--- /dev/null
+++ b/tests/acceptance/virtio_version.py
@@ -0,0 +1,138 @@
+"""
+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
+
+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):
+ """
+ Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
+ """
+ with QEMUMachine(self.qemu_bin) as vm:
+ vm.set_machine('pc')
+ 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]
+
+
+ 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_modern_variants(self, qemu_devtype, virtio_devid):
+ # 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)
+
+ dev_1_0 = self.run_device('%s-1.0' % (qemu_devtype))
+ self.assertEqual(dev_modern, dev_1_0)
+
+ def check_all_variants(self, qemu_devtype, virtio_devid):
+ self.check_modern_variants(qemu_devtype, virtio_devid)
+
+ # 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:
+ dev_no_opts = self.run_device(qemu_devtype)
+ self.assertEqual(dev_trans, dev_no_opts)
+
+ # <prefix>-0.9 and <prefix>-1.0-transitional device types:
+ dev_0_9 = self.run_device('%s-0.9' % (qemu_devtype))
+ self.assertEqual(dev_legacy, dev_0_9)
+ dev_1_0_trans = self.run_device('%s-1.0-transitional' % (qemu_devtype))
+ self.assertEqual(dev_trans, dev_1_0_trans)
+
+ def testConventionalDevs(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)
+ self.check_modern_variants('virtio-vga', VIRTIO_GPU)
+ self.check_modern_variants('virtio-gpu-pci', VIRTIO_GPU)
+ self.check_modern_variants('virtio-mouse-pci', VIRTIO_INPUT)
+ self.check_modern_variants('virtio-tablet-pci', VIRTIO_INPUT)
+ self.check_modern_variants('virtio-keyboard-pci', VIRTIO_INPUT)
--
2.18.0.rc1.1.g3f1ff2140
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-13 2:54 [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
@ 2018-10-14 21:35 ` Michael S. Tsirkin
2018-10-15 18:14 ` Eduardo Habkost
2018-10-15 8:16 ` Gerd Hoffmann
` (2 subsequent siblings)
3 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2018-10-14 21:35 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara, Gerd Hoffmann
On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> 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 is 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-0.9: legacy virtio device
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0: modern-only
> - Supports both Conventional PCI and PCI Express buses
I would prefer a "modern" suffix since it will likely cover future
revisions as well.
Besides, I don't have a problem with this but I'd like an
ack from someone on the management side, confirming
these new interfaces are actually going to be used.
Could you copy some relevant people as well pls?
> All the types above will inherit from an abstract
> "virtio-*-pci-base" type, so existing code that doesn't care
> about the virtio version can use "virtio-*-pci-base" on type
> casts.
>
> 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.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Reference to previous discussion that originated this idea:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> ---
> hw/virtio/virtio-pci.h | 93 +++++++++---
> hw/display/virtio-gpu-pci.c | 8 +-
> hw/display/virtio-vga.c | 11 +-
> hw/virtio/virtio-crypto-pci.c | 8 +-
> hw/virtio/virtio-pci.c | 225 +++++++++++++++++++++--------
> tests/acceptance/virtio_version.py | 138 ++++++++++++++++++
> 6 files changed, 390 insertions(+), 93 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 813082b0d7..f1cfb60277 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -216,7 +216,8 @@ 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_PREFIX "virtio-scsi-pci"
> +#define TYPE_VIRTIO_SCSI_PCI (TYPE_VIRTIO_SCSI_PCI_PREFIX "-base")
> #define VIRTIO_SCSI_PCI(obj) \
> OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
>
> @@ -229,7 +230,8 @@ struct VirtIOSCSIPCI {
> /*
> * vhost-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI_PREFIX "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI (TYPE_VHOST_SCSI_PCI_PREFIX "-base")
> #define VHOST_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
>
> @@ -239,7 +241,8 @@ struct VHostSCSIPCI {
> };
> #endif
>
> -#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
> +#define TYPE_VHOST_USER_SCSI_PCI_PREFIX "vhost-user-scsi-pci"
> +#define TYPE_VHOST_USER_SCSI_PCI (TYPE_VHOST_USER_SCSI_PCI_PREFIX "-base")
> #define VHOST_USER_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
>
> @@ -252,7 +255,8 @@ 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_PREFIX "vhost-user-blk-pci"
> +#define TYPE_VHOST_USER_BLK_PCI (TYPE_VHOST_USER_BLK_PCI_PREFIX "-base")
> #define VHOST_USER_BLK_PCI(obj) \
> OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
>
> @@ -265,7 +269,8 @@ struct VHostUserBlkPCI {
> /*
> * virtio-blk-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
> +#define TYPE_VIRTIO_BLK_PCI_PREFIX "virtio-blk-pci"
> +#define TYPE_VIRTIO_BLK_PCI (TYPE_VIRTIO_BLK_PCI_PREFIX "-base")
> #define VIRTIO_BLK_PCI(obj) \
> OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
>
> @@ -277,7 +282,8 @@ struct VirtIOBlkPCI {
> /*
> * virtio-balloon-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci"
> +#define TYPE_VIRTIO_BALLOON_PCI_PREFIX "virtio-balloon-pci"
> +#define TYPE_VIRTIO_BALLOON_PCI (TYPE_VIRTIO_BALLOON_PCI_PREFIX "-base")
> #define VIRTIO_BALLOON_PCI(obj) \
> OBJECT_CHECK(VirtIOBalloonPCI, (obj), TYPE_VIRTIO_BALLOON_PCI)
>
> @@ -289,7 +295,8 @@ struct VirtIOBalloonPCI {
> /*
> * virtio-serial-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci"
> +#define TYPE_VIRTIO_SERIAL_PCI_PREFIX "virtio-serial-pci"
> +#define TYPE_VIRTIO_SERIAL_PCI (TYPE_VIRTIO_SERIAL_PCI_PREFIX "-base")
> #define VIRTIO_SERIAL_PCI(obj) \
> OBJECT_CHECK(VirtIOSerialPCI, (obj), TYPE_VIRTIO_SERIAL_PCI)
>
> @@ -301,7 +308,8 @@ struct VirtIOSerialPCI {
> /*
> * virtio-net-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_NET_PCI "virtio-net-pci"
> +#define TYPE_VIRTIO_NET_PCI_PREFIX "virtio-net-pci"
> +#define TYPE_VIRTIO_NET_PCI (TYPE_VIRTIO_NET_PCI_PREFIX "-base")
> #define VIRTIO_NET_PCI(obj) \
> OBJECT_CHECK(VirtIONetPCI, (obj), TYPE_VIRTIO_NET_PCI)
>
> @@ -316,7 +324,8 @@ struct VirtIONetPCI {
>
> #ifdef CONFIG_VIRTFS
>
> -#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci"
> +#define TYPE_VIRTIO_9P_PCI_PREFIX "virtio-9p-pci"
> +#define TYPE_VIRTIO_9P_PCI (TYPE_VIRTIO_9P_PCI_PREFIX "-base")
> #define VIRTIO_9P_PCI(obj) \
> OBJECT_CHECK(V9fsPCIState, (obj), TYPE_VIRTIO_9P_PCI)
>
> @@ -330,7 +339,8 @@ typedef struct V9fsPCIState {
> /*
> * virtio-rng-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci"
> +#define TYPE_VIRTIO_RNG_PCI_PREFIX "virtio-rng-pci"
> +#define TYPE_VIRTIO_RNG_PCI (TYPE_VIRTIO_RNG_PCI_PREFIX "-base")
> #define VIRTIO_RNG_PCI(obj) \
> OBJECT_CHECK(VirtIORngPCI, (obj), TYPE_VIRTIO_RNG_PCI)
>
> @@ -352,9 +362,13 @@ struct VirtIOInputPCI {
> };
>
> #define TYPE_VIRTIO_INPUT_HID_PCI "virtio-input-hid-pci"
> -#define TYPE_VIRTIO_KEYBOARD_PCI "virtio-keyboard-pci"
> -#define TYPE_VIRTIO_MOUSE_PCI "virtio-mouse-pci"
> -#define TYPE_VIRTIO_TABLET_PCI "virtio-tablet-pci"
> +
> +#define TYPE_VIRTIO_KEYBOARD_PCI_PREFIX "virtio-keyboard-pci"
> +#define TYPE_VIRTIO_KEYBOARD_PCI (TYPE_VIRTIO_KEYBOARD_PCI_PREFIX "-base")
> +#define TYPE_VIRTIO_MOUSE_PCI_PREFIX "virtio-mouse-pci"
> +#define TYPE_VIRTIO_MOUSE_PCI (TYPE_VIRTIO_MOUSE_PCI_PREFIX "-base")
> +#define TYPE_VIRTIO_TABLET_PCI_PREFIX "virtio-tablet-pci"
> +#define TYPE_VIRTIO_TABLET_PCI (TYPE_VIRTIO_TABLET_PCI_PREFIX "-base")
> #define VIRTIO_INPUT_HID_PCI(obj) \
> OBJECT_CHECK(VirtIOInputHIDPCI, (obj), TYPE_VIRTIO_INPUT_HID_PCI)
>
> @@ -365,7 +379,8 @@ struct VirtIOInputHIDPCI {
>
> #ifdef CONFIG_LINUX
>
> -#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci"
> +#define TYPE_VIRTIO_INPUT_HOST_PCI_PREFIX "virtio-input-host-pci"
> +#define TYPE_VIRTIO_INPUT_HOST_PCI (TYPE_VIRTIO_INPUT_HOST "-base")
> #define VIRTIO_INPUT_HOST_PCI(obj) \
> OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
>
> @@ -379,7 +394,8 @@ struct VirtIOInputHostPCI {
> /*
> * virtio-gpu-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
> +#define TYPE_VIRTIO_GPU_PCI_PREFIX "virtio-gpu-pci"
> +#define TYPE_VIRTIO_GPU_PCI (TYPE_VIRTIO_GPU_PCI_PREFIX "-base")
> #define VIRTIO_GPU_PCI(obj) \
> OBJECT_CHECK(VirtIOGPUPCI, (obj), TYPE_VIRTIO_GPU_PCI)
>
> @@ -392,7 +408,8 @@ struct VirtIOGPUPCI {
> /*
> * vhost-vsock-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci"
> +#define TYPE_VHOST_VSOCK_PCI_PREFIX "vhost-vsock-pci"
> +#define TYPE_VHOST_VSOCK_PCI (TYPE_VHOST_VSOCK_PCI_PREFIX "-base")
> #define VHOST_VSOCK_PCI(obj) \
> OBJECT_CHECK(VHostVSockPCI, (obj), TYPE_VHOST_VSOCK_PCI)
>
> @@ -405,7 +422,8 @@ struct VHostVSockPCI {
> /*
> * virtio-crypto-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_CRYPTO_PCI "virtio-crypto-pci"
> +#define TYPE_VIRTIO_CRYPTO_PCI_PREFIX "virtio-crypto-pci"
> +#define TYPE_VIRTIO_CRYPTO_PCI (TYPE_VIRTIO_CRYPTO_PCI_PREFIX "-base")
> #define VIRTIO_CRYPTO_PCI(obj) \
> OBJECT_CHECK(VirtIOCryptoPCI, (obj), TYPE_VIRTIO_CRYPTO_PCI)
>
> @@ -417,4 +435,45 @@ struct VirtIOCryptoPCI {
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> #define VIRTIO_PCI_ABI_VERSION 0
>
> +/**
> + * VirtioPCIDeviceTypeInfo:
> + *
> + * Template for virtio PCI device types. See virtio_register_types()
> + * for details.
> + */
> +typedef struct VirtioPCIDeviceTypeInfo {
> + /* Prefix for the subclass names */
> + const char *name_prefix;
> + /* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
> + const char *parent;
> + /*
> + * If modern_only is true, only <name_prefix> and <name_prefix>-1.0
> + * types will be registered.
> + */
> + bool modern_only;
> +
> + /* Same as TypeInfo fields: */
> + size_t instance_size;
> + void (*instance_init)(Object *obj);
> + void (*class_init)(ObjectClass *klass, void *data);
> +} VirtioPCIDeviceTypeInfo;
> +
> +/*
> + * Register virtio-pci types. Each virtio-pci device type will be available
> + * in 4 flavors:
> + * - <name_prefix>-0.9: legacy virtio devices
> + * - Supports Conventional PCI buses only
> + * - <name_prefix>-1.0-transitional: modern device supporting legacy drivers
> + * - Supports Conventional PCI buses only
> + * - <name_prefix>-1.0: modern-only
> + * - Supports Conventional PCI and PCI Express buses
> + * - <name_prefix>: virtio version configurable, legacy driver support
> + * automatically selected when device is plugged
> + * - Supports Conventional PCI and PCI Express buses
> + *
> + * All the types above will inherit from "<name_prefix>-base", so generic
> + * code can use "<name_prefix>-base" on type casts.
> + */
> +void virtio_register_types(const VirtioPCIDeviceTypeInfo *t);
> +
> #endif
> diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
> index cece4aa495..a0fb6e46da 100644
> --- a/hw/display/virtio-gpu-pci.c
> +++ b/hw/display/virtio-gpu-pci.c
> @@ -69,9 +69,9 @@ 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 = {
> + .name_prefix = TYPE_VIRTIO_GPU_PCI_PREFIX,
> + .modern_only = true,
> .instance_size = sizeof(VirtIOGPUPCI),
> .instance_init = virtio_gpu_initfn,
> .class_init = virtio_gpu_pci_class_init,
> @@ -79,6 +79,6 @@ static const TypeInfo virtio_gpu_pci_info = {
>
> static void virtio_gpu_pci_register_types(void)
> {
> - type_register_static(&virtio_gpu_pci_info);
> + virtio_register_types(&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..07d6e4b247 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -8,7 +8,8 @@
> /*
> * virtio-vga: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_VGA "virtio-vga"
> +#define TYPE_VIRTIO_VGA_PREFIX "virtio-vga"
> +#define TYPE_VIRTIO_VGA (TYPE_VIRTIO_VGA_PREFIX "-base")
> #define VIRTIO_VGA(obj) \
> OBJECT_CHECK(VirtIOVGA, (obj), TYPE_VIRTIO_VGA)
>
> @@ -207,9 +208,9 @@ 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 = {
> + .name_prefix = TYPE_VIRTIO_VGA_PREFIX,
> + .modern_only = true,
> .instance_size = sizeof(struct VirtIOVGA),
> .instance_init = virtio_vga_inst_initfn,
> .class_init = virtio_vga_class_init,
> @@ -217,7 +218,7 @@ static TypeInfo virtio_vga_info = {
>
> static void virtio_vga_register_types(void)
> {
> - type_register_static(&virtio_vga_info);
> + virtio_register_types(&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..1de0f88c51 100644
> --- a/hw/virtio/virtio-crypto-pci.c
> +++ b/hw/virtio/virtio-crypto-pci.c
> @@ -64,9 +64,9 @@ 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 = {
> + .name_prefix = TYPE_VIRTIO_CRYPTO_PCI_PREFIX,
> + .modern_only = true,
> .instance_size = sizeof(VirtIOCryptoPCI),
> .instance_init = virtio_crypto_initfn,
> .class_init = virtio_crypto_pci_class_init,
> @@ -74,6 +74,6 @@ static const TypeInfo virtio_crypto_pci_info = {
>
> static void virtio_crypto_pci_register_types(void)
> {
> - type_register_static(&virtio_crypto_pci_info);
> + virtio_register_types(&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 3a01fe90f0..6e2141e009 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 = {
> + .name_prefix = TYPE_VIRTIO_9P_PCI_PREFIX,
> .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,
> @@ -1940,12 +1936,119 @@ static const TypeInfo virtio_pci_info = {
> .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_generic_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->props = virtio_pci_generic_properties;
> +}
> +
> +static void virtio_pci_0_9_instance_init(Object *obj)
> +{
> + VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> +
> + proxy->disable_legacy = ON_OFF_AUTO_OFF;
> + proxy->disable_modern = true;
> +}
> +
> +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_1_0_instance_init(Object *obj)
> +{
> + VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> +
> + proxy->disable_legacy = ON_OFF_AUTO_ON;
> + proxy->disable_modern = false;
> +}
> +
> +
> +void virtio_register_types(const VirtioPCIDeviceTypeInfo *t)
> +{
> + const TypeInfo base_type_info = {
> + .name = g_strdup_printf("%s-base", t->name_prefix),
> + .parent = t->parent ? t->parent : TYPE_VIRTIO_PCI,
> + .instance_size = t->instance_size,
> + .instance_init = t->instance_init,
> + .class_init = t->class_init,
> + .abstract = true,
> + };
> +
> + const TypeInfo generic_type_info = {
> + .name = t->name_prefix,
> + .parent = base_type_info.name,
> + .class_init = virtio_pci_generic_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_PCIE_DEVICE },
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { }
> + },
> + };
> +
> + const TypeInfo virtio_1_0_type_info = {
> + .name = g_strdup_printf("%s-1.0", t->name_prefix),
> + .parent = base_type_info.name,
> + .instance_init = virtio_pci_1_0_instance_init,
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_PCIE_DEVICE },
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { }
> + },
> + };
> +
> + const TypeInfo virtio_1_0_transitional_type_info = {
> + .name = g_strdup_printf("%s-1.0-transitional", t->name_prefix),
> + .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 },
> + { }
> + },
> + };
> +
> + const TypeInfo virtio_0_9_type_info = {
> + .name = g_strdup_printf("%s-0.9", t->name_prefix),
> + .parent = base_type_info.name,
> + .instance_init = virtio_pci_0_9_instance_init,
> + .interfaces = (InterfaceInfo[]) {
> + /*
> + * Transitional virtio devices work only as conventional PCI devices
> + * because they require PIO ports.
> + */
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { }
> + },
> + };
> +
> + type_register(&base_type_info);
> + type_register(&generic_type_info);
> + type_register(&virtio_1_0_type_info);
> + if (!t->modern_only) {
> + type_register(&virtio_1_0_transitional_type_info);
> + type_register(&virtio_0_9_type_info);
> + }
> +}
> +
> /* virtio-blk-pci */
>
> static Property virtio_blk_pci_properties[] = {
> @@ -1995,9 +2098,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 = {
> + .name_prefix = TYPE_VIRTIO_BLK_PCI_PREFIX,
> .instance_size = sizeof(VirtIOBlkPCI),
> .instance_init = virtio_blk_pci_instance_init,
> .class_init = virtio_blk_pci_class_init,
> @@ -2051,9 +2153,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 = {
> + .name_prefix = TYPE_VHOST_USER_BLK_PCI_PREFIX,
> .instance_size = sizeof(VHostUserBlkPCI),
> .instance_init = vhost_user_blk_pci_instance_init,
> .class_init = vhost_user_blk_pci_class_init,
> @@ -2119,9 +2220,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 = {
> + .name_prefix = TYPE_VIRTIO_SCSI_PCI_PREFIX,
> .instance_size = sizeof(VirtIOSCSIPCI),
> .instance_init = virtio_scsi_pci_instance_init,
> .class_init = virtio_scsi_pci_class_init,
> @@ -2174,9 +2274,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 = {
> + .name_prefix = TYPE_VHOST_SCSI_PCI_PREFIX,
> .instance_size = sizeof(VHostSCSIPCI),
> .instance_init = vhost_scsi_pci_instance_init,
> .class_init = vhost_scsi_pci_class_init,
> @@ -2229,9 +2328,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 = {
> + .name_prefix = TYPE_VHOST_USER_SCSI_PCI_PREFIX,
> .instance_size = sizeof(VHostUserSCSIPCI),
> .instance_init = vhost_user_scsi_pci_instance_init,
> .class_init = vhost_user_scsi_pci_class_init,
> @@ -2277,9 +2375,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 = {
> + .name_prefix = TYPE_VHOST_VSOCK_PCI_PREFIX,
> .instance_size = sizeof(VHostVSockPCI),
> .instance_init = vhost_vsock_pci_instance_init,
> .class_init = vhost_vsock_pci_class_init,
> @@ -2334,9 +2431,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 = {
> + .name_prefix = TYPE_VIRTIO_BALLOON_PCI_PREFIX,
> .instance_size = sizeof(VirtIOBalloonPCI),
> .instance_init = virtio_balloon_pci_instance_init,
> .class_init = virtio_balloon_pci_class_init,
> @@ -2407,9 +2503,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 = {
> + .name_prefix = TYPE_VIRTIO_SERIAL_PCI_PREFIX,
> .instance_size = sizeof(VirtIOSerialPCI),
> .instance_init = virtio_serial_pci_instance_init,
> .class_init = virtio_serial_pci_class_init,
> @@ -2462,9 +2557,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 = {
> + .name_prefix = TYPE_VIRTIO_NET_PCI_PREFIX,
> .instance_size = sizeof(VirtIONetPCI),
> .instance_init = virtio_net_pci_instance_init,
> .class_init = virtio_net_pci_class_init,
> @@ -2513,9 +2607,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 = {
> + .name_prefix = TYPE_VIRTIO_RNG_PCI_PREFIX,
> .instance_size = sizeof(VirtIORngPCI),
> .instance_init = virtio_rng_initfn,
> .class_init = virtio_rng_pci_class_init,
> @@ -2605,25 +2698,28 @@ 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 = {
> + .name_prefix = TYPE_VIRTIO_KEYBOARD_PCI_PREFIX,
> .parent = TYPE_VIRTIO_INPUT_HID_PCI,
> + .modern_only = true,
> .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 = {
> + .name_prefix = TYPE_VIRTIO_MOUSE_PCI_PREFIX,
> .parent = TYPE_VIRTIO_INPUT_HID_PCI,
> + .modern_only = true,
> .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 = {
> + .name_prefix = TYPE_VIRTIO_TABLET_PCI_PREFIX,
> .parent = TYPE_VIRTIO_INPUT_HID_PCI,
> + .modern_only = true,
> .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 = {
> + .name_prefix = TYPE_VIRTIO_INPUT_HOST_PCI_PREFIX,
> .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_register_types(&virtio_rng_pci_info);
> + virtio_register_types(&virtio_keyboard_pci_info);
> + virtio_register_types(&virtio_mouse_pci_info);
> + virtio_register_types(&virtio_tablet_pci_info);
> #ifdef CONFIG_LINUX
> - type_register_static(&virtio_host_pci_info);
> + virtio_register_types(&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_register_types(&virtio_9p_pci_info);
> #endif
> - type_register_static(&virtio_blk_pci_info);
> + virtio_register_types(&virtio_blk_pci_info);
> #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
> - type_register_static(&vhost_user_blk_pci_info);
> + virtio_register_types(&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_register_types(&virtio_scsi_pci_info);
> + virtio_register_types(&virtio_balloon_pci_info);
> + virtio_register_types(&virtio_serial_pci_info);
> + virtio_register_types(&virtio_net_pci_info);
> #ifdef CONFIG_VHOST_SCSI
> - type_register_static(&vhost_scsi_pci_info);
> + virtio_register_types(&vhost_scsi_pci_info);
> #endif
> #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
> - type_register_static(&vhost_user_scsi_pci_info);
> + virtio_register_types(&vhost_user_scsi_pci_info);
> #endif
> #ifdef CONFIG_VHOST_VSOCK
> - type_register_static(&vhost_vsock_pci_info);
> + virtio_register_types(&vhost_vsock_pci_info);
> #endif
> }
>
> diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py
> new file mode 100644
> index 0000000000..e1e0038530
> --- /dev/null
> +++ b/tests/acceptance/virtio_version.py
> @@ -0,0 +1,138 @@
> +"""
> +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
> +
> +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):
> + """
> + Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
> + """
> + with QEMUMachine(self.qemu_bin) as vm:
> + vm.set_machine('pc')
> + 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]
> +
> +
> + 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_modern_variants(self, qemu_devtype, virtio_devid):
> + # 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)
> +
> + dev_1_0 = self.run_device('%s-1.0' % (qemu_devtype))
> + self.assertEqual(dev_modern, dev_1_0)
> +
> + def check_all_variants(self, qemu_devtype, virtio_devid):
> + self.check_modern_variants(qemu_devtype, virtio_devid)
> +
> + # 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:
> + dev_no_opts = self.run_device(qemu_devtype)
> + self.assertEqual(dev_trans, dev_no_opts)
> +
> + # <prefix>-0.9 and <prefix>-1.0-transitional device types:
> + dev_0_9 = self.run_device('%s-0.9' % (qemu_devtype))
> + self.assertEqual(dev_legacy, dev_0_9)
> + dev_1_0_trans = self.run_device('%s-1.0-transitional' % (qemu_devtype))
> + self.assertEqual(dev_trans, dev_1_0_trans)
> +
> + def testConventionalDevs(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)
> + self.check_modern_variants('virtio-vga', VIRTIO_GPU)
> + self.check_modern_variants('virtio-gpu-pci', VIRTIO_GPU)
> + self.check_modern_variants('virtio-mouse-pci', VIRTIO_INPUT)
> + self.check_modern_variants('virtio-tablet-pci', VIRTIO_INPUT)
> + self.check_modern_variants('virtio-keyboard-pci', VIRTIO_INPUT)
> --
> 2.18.0.rc1.1.g3f1ff2140
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-13 2:54 [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
2018-10-14 21:35 ` Michael S. Tsirkin
@ 2018-10-15 8:16 ` Gerd Hoffmann
2018-10-15 10:27 ` Michael S. Tsirkin
2018-10-15 9:45 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Cornelia Huck
2018-10-17 9:22 ` Stefan Hajnoczi
3 siblings, 1 reply; 41+ messages in thread
From: Gerd Hoffmann @ 2018-10-15 8:16 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara
On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> The current virtio-*-pci device types actually represent 3
> different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
Ok.
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
Why? Is there any reason to prefer 0.9 over transitional?
cheers,
Gerd
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-13 2:54 [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
2018-10-14 21:35 ` Michael S. Tsirkin
2018-10-15 8:16 ` Gerd Hoffmann
@ 2018-10-15 9:45 ` Cornelia Huck
2018-10-15 23:32 ` Eduardo Habkost
2018-10-17 9:22 ` Stefan Hajnoczi
3 siblings, 1 reply; 41+ messages in thread
From: Cornelia Huck @ 2018-10-15 9:45 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Caio Carrara, Markus Armbruster,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann, Laine Stump,
Cleber Rosa, Philippe Mathieu-Daudé
On Fri, 12 Oct 2018 23:54:35 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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 is 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-0.9: legacy virtio device
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0: modern-only
> - Supports both Conventional PCI and PCI Express buses
>
> All the types above will inherit from an abstract
> "virtio-*-pci-base" type, so existing code that doesn't care
> about the virtio version can use "virtio-*-pci-base" on type
> casts.
I get a crash when running 'make check'; might be missing a change to
virtio-input-host?
TEST: tests/device-introspect-test... (pid=28626)
/s390x/device/introspect/list: OK
/s390x/device/introspect/list-fields: OK
/s390x/device/introspect/none: OK
/s390x/device/introspect/abstract: OK
/s390x/device/introspect/abstract-interfaces: OK
/s390x/device/introspect/concrete/defaults/none: /home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730:virtio_host_initfn: Object 0x5633a43a1480 is not an instance of type virtio-input-host-device-base
Broken pipe
/home/cohuck/git/qemu/tests/libqtest.c:125: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
(...)
#2 0x00005633a2e92555 in object_dynamic_cast_assert (obj=0x5633a43a1480,
typename=0x5633a30a295c "virtio-input-host-device-base",
file=0x5633a30a2152 "/home/cohuck/git/qemu/hw/virtio/virtio-pci.c",
line=2730, func=0x5633a30a297a "virtio_host_initfn")
at /home/cohuck/git/qemu/qom/object.c:702
#3 0x00005633a2e30fa3 in virtio_host_initfn (obj=0x5633a43a1480)
at /home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730
#4 0x00005633a2e966a2 in object_init_with_type (obj=0x5633a43a1480,
ti=0x5633a42eb990) at /home/cohuck/git/qemu/qom/object.c:356
#5 0x00005633a2e91433 in object_initialize_with_type (data=0x5633a43a1480,
size=33632, type=0x5633a42eb990) at /home/cohuck/git/qemu/qom/object.c:391
#6 0x00005633a2e91db8 in object_new_with_type (type=<optimized out>)
at /home/cohuck/git/qemu/qom/object.c:553
#7 object_new (typename=<optimized out>)
at /home/cohuck/git/qemu/qom/object.c:563
#8 0x00005633a2dbfc49 in qmp_device_list_properties (
typename=0x5633a437cb40 "virtio-input-host-pci-1.0-transitional",
errp=0x7ffd66276010) at /home/cohuck/git/qemu/qmp.c:513
(...)
>
> 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.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Reference to previous discussion that originated this idea:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> ---
> hw/virtio/virtio-pci.h | 93 +++++++++---
> hw/display/virtio-gpu-pci.c | 8 +-
> hw/display/virtio-vga.c | 11 +-
> hw/virtio/virtio-crypto-pci.c | 8 +-
> hw/virtio/virtio-pci.c | 225 +++++++++++++++++++++--------
> tests/acceptance/virtio_version.py | 138 ++++++++++++++++++
> 6 files changed, 390 insertions(+), 93 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
The approach makes sense to me, but I second the suggestion to use
something like 'modern' (or 'standard'?) instead of '1.0'.
Also, before someone asks: I don't think we need something like that
for virtio-ccw, as we (a) only support fencing newer revisions, not
older ones, and (b) the implications of using different virtio
revisions are localized to virtio-ccw only and do not have further
implications as for virtio-pci.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-15 8:16 ` Gerd Hoffmann
@ 2018-10-15 10:27 ` Michael S. Tsirkin
2018-10-15 23:03 ` Eduardo Habkost
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2018-10-15 10:27 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Eduardo Habkost, qemu-devel, Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara
On Mon, Oct 15, 2018 at 10:16:41AM +0200, Gerd Hoffmann wrote:
> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > The current virtio-*-pci device types actually represent 3
> > different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
>
> Ok.
>
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> Why? Is there any reason to prefer 0.9 over transitional?
>
> cheers,
> Gerd
Right - maybe a flag to disable modern interface is enough.
We thus get two types:
- transitional (legacy)
- non-transitional (modern)
which matches spec nicely.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-14 21:35 ` Michael S. Tsirkin
@ 2018-10-15 18:14 ` Eduardo Habkost
2018-10-16 8:39 ` Cornelia Huck
2018-10-16 17:02 ` Daniel P. Berrangé
0 siblings, 2 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-15 18:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara, Gerd Hoffmann, Andrea Bolognani,
Daniel P. Berrange, Fabian Deutsch
On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > 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 is 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-0.9: legacy virtio device
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0: modern-only
> > - Supports both Conventional PCI and PCI Express buses
>
> I would prefer a "modern" suffix since it will likely cover future
> revisions as well.
That's on purpose. The new device types don't cover future
revisions, otherwise we'll have exactly the same ambiguity
problems in the future.
The moment we have a new virtio specification released, a device
type called "modern" will automatically become ambiguous.
>
>
> Besides, I don't have a problem with this but I'd like an
> ack from someone on the management side, confirming
> these new interfaces are actually going to be used.
>
> Could you copy some relevant people as well pls?
CCing Andrea, Daniel, and Laine from the libvirt side.
CCing Fabian from the KubeVirt side.
>
> > All the types above will inherit from an abstract
> > "virtio-*-pci-base" type, so existing code that doesn't care
> > about the virtio version can use "virtio-*-pci-base" on type
> > casts.
> >
> > 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.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Reference to previous discussion that originated this idea:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> > ---
> > hw/virtio/virtio-pci.h | 93 +++++++++---
> > hw/display/virtio-gpu-pci.c | 8 +-
> > hw/display/virtio-vga.c | 11 +-
> > hw/virtio/virtio-crypto-pci.c | 8 +-
> > hw/virtio/virtio-pci.c | 225 +++++++++++++++++++++--------
> > tests/acceptance/virtio_version.py | 138 ++++++++++++++++++
> > 6 files changed, 390 insertions(+), 93 deletions(-)
> > create mode 100644 tests/acceptance/virtio_version.py
> >
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 813082b0d7..f1cfb60277 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -216,7 +216,8 @@ 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_PREFIX "virtio-scsi-pci"
> > +#define TYPE_VIRTIO_SCSI_PCI (TYPE_VIRTIO_SCSI_PCI_PREFIX "-base")
> > #define VIRTIO_SCSI_PCI(obj) \
> > OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
> >
> > @@ -229,7 +230,8 @@ struct VirtIOSCSIPCI {
> > /*
> > * vhost-scsi-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
> > +#define TYPE_VHOST_SCSI_PCI_PREFIX "vhost-scsi-pci"
> > +#define TYPE_VHOST_SCSI_PCI (TYPE_VHOST_SCSI_PCI_PREFIX "-base")
> > #define VHOST_SCSI_PCI(obj) \
> > OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
> >
> > @@ -239,7 +241,8 @@ struct VHostSCSIPCI {
> > };
> > #endif
> >
> > -#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
> > +#define TYPE_VHOST_USER_SCSI_PCI_PREFIX "vhost-user-scsi-pci"
> > +#define TYPE_VHOST_USER_SCSI_PCI (TYPE_VHOST_USER_SCSI_PCI_PREFIX "-base")
> > #define VHOST_USER_SCSI_PCI(obj) \
> > OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
> >
> > @@ -252,7 +255,8 @@ 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_PREFIX "vhost-user-blk-pci"
> > +#define TYPE_VHOST_USER_BLK_PCI (TYPE_VHOST_USER_BLK_PCI_PREFIX "-base")
> > #define VHOST_USER_BLK_PCI(obj) \
> > OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
> >
> > @@ -265,7 +269,8 @@ struct VHostUserBlkPCI {
> > /*
> > * virtio-blk-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
> > +#define TYPE_VIRTIO_BLK_PCI_PREFIX "virtio-blk-pci"
> > +#define TYPE_VIRTIO_BLK_PCI (TYPE_VIRTIO_BLK_PCI_PREFIX "-base")
> > #define VIRTIO_BLK_PCI(obj) \
> > OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
> >
> > @@ -277,7 +282,8 @@ struct VirtIOBlkPCI {
> > /*
> > * virtio-balloon-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci"
> > +#define TYPE_VIRTIO_BALLOON_PCI_PREFIX "virtio-balloon-pci"
> > +#define TYPE_VIRTIO_BALLOON_PCI (TYPE_VIRTIO_BALLOON_PCI_PREFIX "-base")
> > #define VIRTIO_BALLOON_PCI(obj) \
> > OBJECT_CHECK(VirtIOBalloonPCI, (obj), TYPE_VIRTIO_BALLOON_PCI)
> >
> > @@ -289,7 +295,8 @@ struct VirtIOBalloonPCI {
> > /*
> > * virtio-serial-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_SERIAL_PCI "virtio-serial-pci"
> > +#define TYPE_VIRTIO_SERIAL_PCI_PREFIX "virtio-serial-pci"
> > +#define TYPE_VIRTIO_SERIAL_PCI (TYPE_VIRTIO_SERIAL_PCI_PREFIX "-base")
> > #define VIRTIO_SERIAL_PCI(obj) \
> > OBJECT_CHECK(VirtIOSerialPCI, (obj), TYPE_VIRTIO_SERIAL_PCI)
> >
> > @@ -301,7 +308,8 @@ struct VirtIOSerialPCI {
> > /*
> > * virtio-net-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_NET_PCI "virtio-net-pci"
> > +#define TYPE_VIRTIO_NET_PCI_PREFIX "virtio-net-pci"
> > +#define TYPE_VIRTIO_NET_PCI (TYPE_VIRTIO_NET_PCI_PREFIX "-base")
> > #define VIRTIO_NET_PCI(obj) \
> > OBJECT_CHECK(VirtIONetPCI, (obj), TYPE_VIRTIO_NET_PCI)
> >
> > @@ -316,7 +324,8 @@ struct VirtIONetPCI {
> >
> > #ifdef CONFIG_VIRTFS
> >
> > -#define TYPE_VIRTIO_9P_PCI "virtio-9p-pci"
> > +#define TYPE_VIRTIO_9P_PCI_PREFIX "virtio-9p-pci"
> > +#define TYPE_VIRTIO_9P_PCI (TYPE_VIRTIO_9P_PCI_PREFIX "-base")
> > #define VIRTIO_9P_PCI(obj) \
> > OBJECT_CHECK(V9fsPCIState, (obj), TYPE_VIRTIO_9P_PCI)
> >
> > @@ -330,7 +339,8 @@ typedef struct V9fsPCIState {
> > /*
> > * virtio-rng-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_RNG_PCI "virtio-rng-pci"
> > +#define TYPE_VIRTIO_RNG_PCI_PREFIX "virtio-rng-pci"
> > +#define TYPE_VIRTIO_RNG_PCI (TYPE_VIRTIO_RNG_PCI_PREFIX "-base")
> > #define VIRTIO_RNG_PCI(obj) \
> > OBJECT_CHECK(VirtIORngPCI, (obj), TYPE_VIRTIO_RNG_PCI)
> >
> > @@ -352,9 +362,13 @@ struct VirtIOInputPCI {
> > };
> >
> > #define TYPE_VIRTIO_INPUT_HID_PCI "virtio-input-hid-pci"
> > -#define TYPE_VIRTIO_KEYBOARD_PCI "virtio-keyboard-pci"
> > -#define TYPE_VIRTIO_MOUSE_PCI "virtio-mouse-pci"
> > -#define TYPE_VIRTIO_TABLET_PCI "virtio-tablet-pci"
> > +
> > +#define TYPE_VIRTIO_KEYBOARD_PCI_PREFIX "virtio-keyboard-pci"
> > +#define TYPE_VIRTIO_KEYBOARD_PCI (TYPE_VIRTIO_KEYBOARD_PCI_PREFIX "-base")
> > +#define TYPE_VIRTIO_MOUSE_PCI_PREFIX "virtio-mouse-pci"
> > +#define TYPE_VIRTIO_MOUSE_PCI (TYPE_VIRTIO_MOUSE_PCI_PREFIX "-base")
> > +#define TYPE_VIRTIO_TABLET_PCI_PREFIX "virtio-tablet-pci"
> > +#define TYPE_VIRTIO_TABLET_PCI (TYPE_VIRTIO_TABLET_PCI_PREFIX "-base")
> > #define VIRTIO_INPUT_HID_PCI(obj) \
> > OBJECT_CHECK(VirtIOInputHIDPCI, (obj), TYPE_VIRTIO_INPUT_HID_PCI)
> >
> > @@ -365,7 +379,8 @@ struct VirtIOInputHIDPCI {
> >
> > #ifdef CONFIG_LINUX
> >
> > -#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci"
> > +#define TYPE_VIRTIO_INPUT_HOST_PCI_PREFIX "virtio-input-host-pci"
> > +#define TYPE_VIRTIO_INPUT_HOST_PCI (TYPE_VIRTIO_INPUT_HOST "-base")
> > #define VIRTIO_INPUT_HOST_PCI(obj) \
> > OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
> >
> > @@ -379,7 +394,8 @@ struct VirtIOInputHostPCI {
> > /*
> > * virtio-gpu-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
> > +#define TYPE_VIRTIO_GPU_PCI_PREFIX "virtio-gpu-pci"
> > +#define TYPE_VIRTIO_GPU_PCI (TYPE_VIRTIO_GPU_PCI_PREFIX "-base")
> > #define VIRTIO_GPU_PCI(obj) \
> > OBJECT_CHECK(VirtIOGPUPCI, (obj), TYPE_VIRTIO_GPU_PCI)
> >
> > @@ -392,7 +408,8 @@ struct VirtIOGPUPCI {
> > /*
> > * vhost-vsock-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VHOST_VSOCK_PCI "vhost-vsock-pci"
> > +#define TYPE_VHOST_VSOCK_PCI_PREFIX "vhost-vsock-pci"
> > +#define TYPE_VHOST_VSOCK_PCI (TYPE_VHOST_VSOCK_PCI_PREFIX "-base")
> > #define VHOST_VSOCK_PCI(obj) \
> > OBJECT_CHECK(VHostVSockPCI, (obj), TYPE_VHOST_VSOCK_PCI)
> >
> > @@ -405,7 +422,8 @@ struct VHostVSockPCI {
> > /*
> > * virtio-crypto-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_CRYPTO_PCI "virtio-crypto-pci"
> > +#define TYPE_VIRTIO_CRYPTO_PCI_PREFIX "virtio-crypto-pci"
> > +#define TYPE_VIRTIO_CRYPTO_PCI (TYPE_VIRTIO_CRYPTO_PCI_PREFIX "-base")
> > #define VIRTIO_CRYPTO_PCI(obj) \
> > OBJECT_CHECK(VirtIOCryptoPCI, (obj), TYPE_VIRTIO_CRYPTO_PCI)
> >
> > @@ -417,4 +435,45 @@ struct VirtIOCryptoPCI {
> > /* Virtio ABI version, if we increment this, we break the guest driver. */
> > #define VIRTIO_PCI_ABI_VERSION 0
> >
> > +/**
> > + * VirtioPCIDeviceTypeInfo:
> > + *
> > + * Template for virtio PCI device types. See virtio_register_types()
> > + * for details.
> > + */
> > +typedef struct VirtioPCIDeviceTypeInfo {
> > + /* Prefix for the subclass names */
> > + const char *name_prefix;
> > + /* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
> > + const char *parent;
> > + /*
> > + * If modern_only is true, only <name_prefix> and <name_prefix>-1.0
> > + * types will be registered.
> > + */
> > + bool modern_only;
> > +
> > + /* Same as TypeInfo fields: */
> > + size_t instance_size;
> > + void (*instance_init)(Object *obj);
> > + void (*class_init)(ObjectClass *klass, void *data);
> > +} VirtioPCIDeviceTypeInfo;
> > +
> > +/*
> > + * Register virtio-pci types. Each virtio-pci device type will be available
> > + * in 4 flavors:
> > + * - <name_prefix>-0.9: legacy virtio devices
> > + * - Supports Conventional PCI buses only
> > + * - <name_prefix>-1.0-transitional: modern device supporting legacy drivers
> > + * - Supports Conventional PCI buses only
> > + * - <name_prefix>-1.0: modern-only
> > + * - Supports Conventional PCI and PCI Express buses
> > + * - <name_prefix>: virtio version configurable, legacy driver support
> > + * automatically selected when device is plugged
> > + * - Supports Conventional PCI and PCI Express buses
> > + *
> > + * All the types above will inherit from "<name_prefix>-base", so generic
> > + * code can use "<name_prefix>-base" on type casts.
> > + */
> > +void virtio_register_types(const VirtioPCIDeviceTypeInfo *t);
> > +
> > #endif
> > diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
> > index cece4aa495..a0fb6e46da 100644
> > --- a/hw/display/virtio-gpu-pci.c
> > +++ b/hw/display/virtio-gpu-pci.c
> > @@ -69,9 +69,9 @@ 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 = {
> > + .name_prefix = TYPE_VIRTIO_GPU_PCI_PREFIX,
> > + .modern_only = true,
> > .instance_size = sizeof(VirtIOGPUPCI),
> > .instance_init = virtio_gpu_initfn,
> > .class_init = virtio_gpu_pci_class_init,
> > @@ -79,6 +79,6 @@ static const TypeInfo virtio_gpu_pci_info = {
> >
> > static void virtio_gpu_pci_register_types(void)
> > {
> > - type_register_static(&virtio_gpu_pci_info);
> > + virtio_register_types(&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..07d6e4b247 100644
> > --- a/hw/display/virtio-vga.c
> > +++ b/hw/display/virtio-vga.c
> > @@ -8,7 +8,8 @@
> > /*
> > * virtio-vga: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_VGA "virtio-vga"
> > +#define TYPE_VIRTIO_VGA_PREFIX "virtio-vga"
> > +#define TYPE_VIRTIO_VGA (TYPE_VIRTIO_VGA_PREFIX "-base")
> > #define VIRTIO_VGA(obj) \
> > OBJECT_CHECK(VirtIOVGA, (obj), TYPE_VIRTIO_VGA)
> >
> > @@ -207,9 +208,9 @@ 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 = {
> > + .name_prefix = TYPE_VIRTIO_VGA_PREFIX,
> > + .modern_only = true,
> > .instance_size = sizeof(struct VirtIOVGA),
> > .instance_init = virtio_vga_inst_initfn,
> > .class_init = virtio_vga_class_init,
> > @@ -217,7 +218,7 @@ static TypeInfo virtio_vga_info = {
> >
> > static void virtio_vga_register_types(void)
> > {
> > - type_register_static(&virtio_vga_info);
> > + virtio_register_types(&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..1de0f88c51 100644
> > --- a/hw/virtio/virtio-crypto-pci.c
> > +++ b/hw/virtio/virtio-crypto-pci.c
> > @@ -64,9 +64,9 @@ 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 = {
> > + .name_prefix = TYPE_VIRTIO_CRYPTO_PCI_PREFIX,
> > + .modern_only = true,
> > .instance_size = sizeof(VirtIOCryptoPCI),
> > .instance_init = virtio_crypto_initfn,
> > .class_init = virtio_crypto_pci_class_init,
> > @@ -74,6 +74,6 @@ static const TypeInfo virtio_crypto_pci_info = {
> >
> > static void virtio_crypto_pci_register_types(void)
> > {
> > - type_register_static(&virtio_crypto_pci_info);
> > + virtio_register_types(&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 3a01fe90f0..6e2141e009 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 = {
> > + .name_prefix = TYPE_VIRTIO_9P_PCI_PREFIX,
> > .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,
> > @@ -1940,12 +1936,119 @@ static const TypeInfo virtio_pci_info = {
> > .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_generic_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + dc->props = virtio_pci_generic_properties;
> > +}
> > +
> > +static void virtio_pci_0_9_instance_init(Object *obj)
> > +{
> > + VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> > +
> > + proxy->disable_legacy = ON_OFF_AUTO_OFF;
> > + proxy->disable_modern = true;
> > +}
> > +
> > +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_1_0_instance_init(Object *obj)
> > +{
> > + VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> > +
> > + proxy->disable_legacy = ON_OFF_AUTO_ON;
> > + proxy->disable_modern = false;
> > +}
> > +
> > +
> > +void virtio_register_types(const VirtioPCIDeviceTypeInfo *t)
> > +{
> > + const TypeInfo base_type_info = {
> > + .name = g_strdup_printf("%s-base", t->name_prefix),
> > + .parent = t->parent ? t->parent : TYPE_VIRTIO_PCI,
> > + .instance_size = t->instance_size,
> > + .instance_init = t->instance_init,
> > + .class_init = t->class_init,
> > + .abstract = true,
> > + };
> > +
> > + const TypeInfo generic_type_info = {
> > + .name = t->name_prefix,
> > + .parent = base_type_info.name,
> > + .class_init = virtio_pci_generic_class_init,
> > + .interfaces = (InterfaceInfo[]) {
> > + { INTERFACE_PCIE_DEVICE },
> > + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > + { }
> > + },
> > + };
> > +
> > + const TypeInfo virtio_1_0_type_info = {
> > + .name = g_strdup_printf("%s-1.0", t->name_prefix),
> > + .parent = base_type_info.name,
> > + .instance_init = virtio_pci_1_0_instance_init,
> > + .interfaces = (InterfaceInfo[]) {
> > + { INTERFACE_PCIE_DEVICE },
> > + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > + { }
> > + },
> > + };
> > +
> > + const TypeInfo virtio_1_0_transitional_type_info = {
> > + .name = g_strdup_printf("%s-1.0-transitional", t->name_prefix),
> > + .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 },
> > + { }
> > + },
> > + };
> > +
> > + const TypeInfo virtio_0_9_type_info = {
> > + .name = g_strdup_printf("%s-0.9", t->name_prefix),
> > + .parent = base_type_info.name,
> > + .instance_init = virtio_pci_0_9_instance_init,
> > + .interfaces = (InterfaceInfo[]) {
> > + /*
> > + * Transitional virtio devices work only as conventional PCI devices
> > + * because they require PIO ports.
> > + */
> > + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > + { }
> > + },
> > + };
> > +
> > + type_register(&base_type_info);
> > + type_register(&generic_type_info);
> > + type_register(&virtio_1_0_type_info);
> > + if (!t->modern_only) {
> > + type_register(&virtio_1_0_transitional_type_info);
> > + type_register(&virtio_0_9_type_info);
> > + }
> > +}
> > +
> > /* virtio-blk-pci */
> >
> > static Property virtio_blk_pci_properties[] = {
> > @@ -1995,9 +2098,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 = {
> > + .name_prefix = TYPE_VIRTIO_BLK_PCI_PREFIX,
> > .instance_size = sizeof(VirtIOBlkPCI),
> > .instance_init = virtio_blk_pci_instance_init,
> > .class_init = virtio_blk_pci_class_init,
> > @@ -2051,9 +2153,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 = {
> > + .name_prefix = TYPE_VHOST_USER_BLK_PCI_PREFIX,
> > .instance_size = sizeof(VHostUserBlkPCI),
> > .instance_init = vhost_user_blk_pci_instance_init,
> > .class_init = vhost_user_blk_pci_class_init,
> > @@ -2119,9 +2220,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 = {
> > + .name_prefix = TYPE_VIRTIO_SCSI_PCI_PREFIX,
> > .instance_size = sizeof(VirtIOSCSIPCI),
> > .instance_init = virtio_scsi_pci_instance_init,
> > .class_init = virtio_scsi_pci_class_init,
> > @@ -2174,9 +2274,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 = {
> > + .name_prefix = TYPE_VHOST_SCSI_PCI_PREFIX,
> > .instance_size = sizeof(VHostSCSIPCI),
> > .instance_init = vhost_scsi_pci_instance_init,
> > .class_init = vhost_scsi_pci_class_init,
> > @@ -2229,9 +2328,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 = {
> > + .name_prefix = TYPE_VHOST_USER_SCSI_PCI_PREFIX,
> > .instance_size = sizeof(VHostUserSCSIPCI),
> > .instance_init = vhost_user_scsi_pci_instance_init,
> > .class_init = vhost_user_scsi_pci_class_init,
> > @@ -2277,9 +2375,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 = {
> > + .name_prefix = TYPE_VHOST_VSOCK_PCI_PREFIX,
> > .instance_size = sizeof(VHostVSockPCI),
> > .instance_init = vhost_vsock_pci_instance_init,
> > .class_init = vhost_vsock_pci_class_init,
> > @@ -2334,9 +2431,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 = {
> > + .name_prefix = TYPE_VIRTIO_BALLOON_PCI_PREFIX,
> > .instance_size = sizeof(VirtIOBalloonPCI),
> > .instance_init = virtio_balloon_pci_instance_init,
> > .class_init = virtio_balloon_pci_class_init,
> > @@ -2407,9 +2503,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 = {
> > + .name_prefix = TYPE_VIRTIO_SERIAL_PCI_PREFIX,
> > .instance_size = sizeof(VirtIOSerialPCI),
> > .instance_init = virtio_serial_pci_instance_init,
> > .class_init = virtio_serial_pci_class_init,
> > @@ -2462,9 +2557,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 = {
> > + .name_prefix = TYPE_VIRTIO_NET_PCI_PREFIX,
> > .instance_size = sizeof(VirtIONetPCI),
> > .instance_init = virtio_net_pci_instance_init,
> > .class_init = virtio_net_pci_class_init,
> > @@ -2513,9 +2607,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 = {
> > + .name_prefix = TYPE_VIRTIO_RNG_PCI_PREFIX,
> > .instance_size = sizeof(VirtIORngPCI),
> > .instance_init = virtio_rng_initfn,
> > .class_init = virtio_rng_pci_class_init,
> > @@ -2605,25 +2698,28 @@ 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 = {
> > + .name_prefix = TYPE_VIRTIO_KEYBOARD_PCI_PREFIX,
> > .parent = TYPE_VIRTIO_INPUT_HID_PCI,
> > + .modern_only = true,
> > .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 = {
> > + .name_prefix = TYPE_VIRTIO_MOUSE_PCI_PREFIX,
> > .parent = TYPE_VIRTIO_INPUT_HID_PCI,
> > + .modern_only = true,
> > .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 = {
> > + .name_prefix = TYPE_VIRTIO_TABLET_PCI_PREFIX,
> > .parent = TYPE_VIRTIO_INPUT_HID_PCI,
> > + .modern_only = true,
> > .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 = {
> > + .name_prefix = TYPE_VIRTIO_INPUT_HOST_PCI_PREFIX,
> > .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_register_types(&virtio_rng_pci_info);
> > + virtio_register_types(&virtio_keyboard_pci_info);
> > + virtio_register_types(&virtio_mouse_pci_info);
> > + virtio_register_types(&virtio_tablet_pci_info);
> > #ifdef CONFIG_LINUX
> > - type_register_static(&virtio_host_pci_info);
> > + virtio_register_types(&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_register_types(&virtio_9p_pci_info);
> > #endif
> > - type_register_static(&virtio_blk_pci_info);
> > + virtio_register_types(&virtio_blk_pci_info);
> > #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
> > - type_register_static(&vhost_user_blk_pci_info);
> > + virtio_register_types(&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_register_types(&virtio_scsi_pci_info);
> > + virtio_register_types(&virtio_balloon_pci_info);
> > + virtio_register_types(&virtio_serial_pci_info);
> > + virtio_register_types(&virtio_net_pci_info);
> > #ifdef CONFIG_VHOST_SCSI
> > - type_register_static(&vhost_scsi_pci_info);
> > + virtio_register_types(&vhost_scsi_pci_info);
> > #endif
> > #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
> > - type_register_static(&vhost_user_scsi_pci_info);
> > + virtio_register_types(&vhost_user_scsi_pci_info);
> > #endif
> > #ifdef CONFIG_VHOST_VSOCK
> > - type_register_static(&vhost_vsock_pci_info);
> > + virtio_register_types(&vhost_vsock_pci_info);
> > #endif
> > }
> >
> > diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py
> > new file mode 100644
> > index 0000000000..e1e0038530
> > --- /dev/null
> > +++ b/tests/acceptance/virtio_version.py
> > @@ -0,0 +1,138 @@
> > +"""
> > +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
> > +
> > +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):
> > + """
> > + Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
> > + """
> > + with QEMUMachine(self.qemu_bin) as vm:
> > + vm.set_machine('pc')
> > + 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]
> > +
> > +
> > + 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_modern_variants(self, qemu_devtype, virtio_devid):
> > + # 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)
> > +
> > + dev_1_0 = self.run_device('%s-1.0' % (qemu_devtype))
> > + self.assertEqual(dev_modern, dev_1_0)
> > +
> > + def check_all_variants(self, qemu_devtype, virtio_devid):
> > + self.check_modern_variants(qemu_devtype, virtio_devid)
> > +
> > + # 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:
> > + dev_no_opts = self.run_device(qemu_devtype)
> > + self.assertEqual(dev_trans, dev_no_opts)
> > +
> > + # <prefix>-0.9 and <prefix>-1.0-transitional device types:
> > + dev_0_9 = self.run_device('%s-0.9' % (qemu_devtype))
> > + self.assertEqual(dev_legacy, dev_0_9)
> > + dev_1_0_trans = self.run_device('%s-1.0-transitional' % (qemu_devtype))
> > + self.assertEqual(dev_trans, dev_1_0_trans)
> > +
> > + def testConventionalDevs(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)
> > + self.check_modern_variants('virtio-vga', VIRTIO_GPU)
> > + self.check_modern_variants('virtio-gpu-pci', VIRTIO_GPU)
> > + self.check_modern_variants('virtio-mouse-pci', VIRTIO_INPUT)
> > + self.check_modern_variants('virtio-tablet-pci', VIRTIO_INPUT)
> > + self.check_modern_variants('virtio-keyboard-pci', VIRTIO_INPUT)
> > --
> > 2.18.0.rc1.1.g3f1ff2140
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-15 10:27 ` Michael S. Tsirkin
@ 2018-10-15 23:03 ` Eduardo Habkost
2018-10-16 6:48 ` Gerd Hoffmann
0 siblings, 1 reply; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-15 23:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Gerd Hoffmann, qemu-devel, Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara
On Mon, Oct 15, 2018 at 06:27:21AM -0400, Michael S. Tsirkin wrote:
> On Mon, Oct 15, 2018 at 10:16:41AM +0200, Gerd Hoffmann wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > The current virtio-*-pci device types actually represent 3
> > > different types of devices:
> > > * virtio 1.0 non-transitional devices
> > > * virtio 1.0 transitional devices
> >
> > Ok.
> >
> > > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > Why? Is there any reason to prefer 0.9 over transitional?
I don't know. The `disable-modern` option already exists but I
don't know who would want to use it. I'm just adding a way to
represent that existing feature without having another set of
device types with multiple-personality disorder.
> >
> > cheers,
> > Gerd
>
> Right - maybe a flag to disable modern interface is enough.
> We thus get two types:
> - transitional (legacy)
> - non-transitional (modern)
> which matches spec nicely.
I would agree with the removal of virtio-*-0.9 only if we decide
to drop support for disable-modern=true. If we still want to
provide a legacy-only virtio device, it should be a separate
device type.
Without separate device types, it becomes impossible to provide
meaningful device type information to management software,
including but not limited to:
* removal/deprecation of device types (especially important if we
plan to remove virtio 0.9 support from QEMU one day)
* PCI IDs
* Bus compatibility information
* Schema/introspection for 0.9-only or 1.0-only device options
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-15 9:45 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Cornelia Huck
@ 2018-10-15 23:32 ` Eduardo Habkost
0 siblings, 0 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-15 23:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, qemu-devel, Caio Carrara, Markus Armbruster,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann, Laine Stump,
Cleber Rosa, Philippe Mathieu-Daudé
On Mon, Oct 15, 2018 at 11:45:56AM +0200, Cornelia Huck wrote:
> On Fri, 12 Oct 2018 23:54:35 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > 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 is 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-0.9: legacy virtio device
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0: modern-only
> > - Supports both Conventional PCI and PCI Express buses
> >
> > All the types above will inherit from an abstract
> > "virtio-*-pci-base" type, so existing code that doesn't care
> > about the virtio version can use "virtio-*-pci-base" on type
> > casts.
>
> I get a crash when running 'make check'; might be missing a change to
> virtio-input-host?
>
> TEST: tests/device-introspect-test... (pid=28626)
> /s390x/device/introspect/list: OK
> /s390x/device/introspect/list-fields: OK
> /s390x/device/introspect/none: OK
> /s390x/device/introspect/abstract: OK
> /s390x/device/introspect/abstract-interfaces: OK
> /s390x/device/introspect/concrete/defaults/none: /home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730:virtio_host_initfn: Object 0x5633a43a1480 is not an instance of type virtio-input-host-device-base
> Broken pipe
> /home/cohuck/git/qemu/tests/libqtest.c:125: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
Oops. I will fix it in v2, thanks.
>
[...]
> >
> > 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.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Reference to previous discussion that originated this idea:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> > ---
> > hw/virtio/virtio-pci.h | 93 +++++++++---
> > hw/display/virtio-gpu-pci.c | 8 +-
> > hw/display/virtio-vga.c | 11 +-
> > hw/virtio/virtio-crypto-pci.c | 8 +-
> > hw/virtio/virtio-pci.c | 225 +++++++++++++++++++++--------
> > tests/acceptance/virtio_version.py | 138 ++++++++++++++++++
> > 6 files changed, 390 insertions(+), 93 deletions(-)
> > create mode 100644 tests/acceptance/virtio_version.py
>
> The approach makes sense to me, but I second the suggestion to use
> something like 'modern' (or 'standard'?) instead of '1.0'.
As noted on the reply I sent to Michael: the whole point of this
patch is to provide unambiguous device type names. What would be
the advantage of having another ambiguous device type named
"virtio-*-modern"?
>
> Also, before someone asks: I don't think we need something like that
> for virtio-ccw, as we (a) only support fencing newer revisions, not
> older ones, and (b) the implications of using different virtio
> revisions are localized to virtio-ccw only and do not have further
> implications as for virtio-pci.
Thanks! I was planning to ask about it later. :)
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-15 23:03 ` Eduardo Habkost
@ 2018-10-16 6:48 ` Gerd Hoffmann
2018-10-16 13:39 ` Eduardo Habkost
0 siblings, 1 reply; 41+ messages in thread
From: Gerd Hoffmann @ 2018-10-16 6:48 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara
Hi,
> I don't know. The `disable-modern` option already exists but I
> don't know who would want to use it.
Compat property for old (2.6 & older) machine types.
> > Right - maybe a flag to disable modern interface is enough.
> > We thus get two types:
> > - transitional (legacy)
> > - non-transitional (modern)
> > which matches spec nicely.
>
> I would agree with the removal of virtio-*-0.9 only if we decide
> to drop support for disable-modern=true.
See above. We can't drop disable-modern.
> If we still want to provide a legacy-only virtio device, it should be
> a separate device type.
I can't see a reason why we should provide legacy-only virtio devices.
cheers,
Gerd
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-15 18:14 ` Eduardo Habkost
@ 2018-10-16 8:39 ` Cornelia Huck
2018-10-16 13:32 ` Eduardo Habkost
2018-10-16 17:02 ` Daniel P. Berrangé
1 sibling, 1 reply; 41+ messages in thread
From: Cornelia Huck @ 2018-10-16 8:39 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, Caio Carrara, Fabian Deutsch,
Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel,
Gonglei, Gerd Hoffmann, Laine Stump, Cleber Rosa,
Andrea Bolognani, Philippe Mathieu-Daudé
On Mon, 15 Oct 2018 15:14:04 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > 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 is 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-0.9: legacy virtio device
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0: modern-only
> > > - Supports both Conventional PCI and PCI Express buses
> >
> > I would prefer a "modern" suffix since it will likely cover future
> > revisions as well.
>
> That's on purpose. The new device types don't cover future
> revisions, otherwise we'll have exactly the same ambiguity
> problems in the future.
>
> The moment we have a new virtio specification released, a device
> type called "modern" will automatically become ambiguous.
I don't think that's a problem.
Most of the issues come from the major changes that virtio-pci did when
moving to the 1.0 standard (like that PIO BAR, or the changed ids). The
1.0 standard allows for gradual enhancements and changes guarded by
feature bits.
Consider the packed ring layout, which is one of the major changes for
1.1: It is completely guarded by feature bits, and it does not come
with any changes down in the pci area. If we want to fence it off for
compat machines, we need to fence off offering the bits (for _any_
transport), which does not imply a new type of virtio-pci device.
There are other, pci-specific changes which are also guarded by feature
bits and don't need a new device type.
Also, many of the enhancements of the virtio standard are likely to be
new device types and new features for existing device types. None of
that is pci specific.
So, what I'd propose is:
- virtio-*-pci-standard: compliant with the virtio standard 1.0 or
later; no legacy fallback
- virtio-*-pci-transitional: compliant with the virtio standard 1.0 or
later; fallback to legacy included, as specified by the standard
(- virtio-*-pci-legacy: legacy devices, should we need that for compat
reasons)
We could also use '-virtio-1' instead of '-standard', if we do a major
break with a 2.x standard (I don't see it yet). But having a new type
for 1.1 sounds wrong.
Also note that virtio-ccw does not need this (we managed to avoid many
of the issues virtio-pci had due to being late to the party :). I'm not
sure what virtio-mmio does, or if we even care.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-16 8:39 ` Cornelia Huck
@ 2018-10-16 13:32 ` Eduardo Habkost
2018-10-16 15:51 ` Cornelia Huck
0 siblings, 1 reply; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-16 13:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Caio Carrara, Fabian Deutsch,
Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel,
Gonglei, Gerd Hoffmann, Laine Stump, Cleber Rosa,
Andrea Bolognani, Philippe Mathieu-Daudé
On Tue, Oct 16, 2018 at 10:39:30AM +0200, Cornelia Huck wrote:
> On Mon, 15 Oct 2018 15:14:04 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > > 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 is 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-0.9: legacy virtio device
> > > > - Supports Conventional PCI buses only, because
> > > > it has a PIO BAR
> > > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > > > - Supports Conventional PCI buses only, because
> > > > it has a PIO BAR
> > > > - virtio-*-pci-1.0: modern-only
> > > > - Supports both Conventional PCI and PCI Express buses
> > >
> > > I would prefer a "modern" suffix since it will likely cover future
> > > revisions as well.
> >
> > That's on purpose. The new device types don't cover future
> > revisions, otherwise we'll have exactly the same ambiguity
> > problems in the future.
> >
> > The moment we have a new virtio specification released, a device
> > type called "modern" will automatically become ambiguous.
>
> I don't think that's a problem.
>
> Most of the issues come from the major changes that virtio-pci did when
> moving to the 1.0 standard (like that PIO BAR, or the changed ids). The
> 1.0 standard allows for gradual enhancements and changes guarded by
> feature bits.
>
> Consider the packed ring layout, which is one of the major changes for
> 1.1: It is completely guarded by feature bits, and it does not come
> with any changes down in the pci area. If we want to fence it off for
> compat machines, we need to fence off offering the bits (for _any_
> transport), which does not imply a new type of virtio-pci device.
>
> There are other, pci-specific changes which are also guarded by feature
> bits and don't need a new device type.
>
> Also, many of the enhancements of the virtio standard are likely to be
> new device types and new features for existing device types. None of
> that is pci specific.
>
> So, what I'd propose is:
> - virtio-*-pci-standard: compliant with the virtio standard 1.0 or
> later; no legacy fallback
> - virtio-*-pci-transitional: compliant with the virtio standard 1.0 or
> later; fallback to legacy included, as specified by the standard
> (- virtio-*-pci-legacy: legacy devices, should we need that for compat
> reasons)
>
> We could also use '-virtio-1' instead of '-standard', if we do a major
> break with a 2.x standard (I don't see it yet). But having a new type
> for 1.1 sounds wrong.
That's true: adding a new type for 1.1 is probably going to be
wrong.
But how can I make any promises about the existing device type
being compatible with 1.1 (or 1.2, 1.3...), if the 1.1 (or 1.2,
1.3...) specification wasn't released yet?
Maybe using "-virtio-1" would be a good way to be clear we're
talking about virtio-1.x without making any promises about 1.1,
1.2, 1.3, etc.
>
> Also note that virtio-ccw does not need this (we managed to avoid many
> of the issues virtio-pci had due to being late to the party :). I'm not
> sure what virtio-mmio does, or if we even care.
>
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-16 6:48 ` Gerd Hoffmann
@ 2018-10-16 13:39 ` Eduardo Habkost
2018-10-16 18:42 ` Gerd Hoffmann
0 siblings, 1 reply; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-16 13:39 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara
On Tue, Oct 16, 2018 at 08:48:16AM +0200, Gerd Hoffmann wrote:
> Hi,
>
> > I don't know. The `disable-modern` option already exists but I
> > don't know who would want to use it.
>
> Compat property for old (2.6 & older) machine types.
>
> > > Right - maybe a flag to disable modern interface is enough.
> > > We thus get two types:
> > > - transitional (legacy)
> > > - non-transitional (modern)
> > > which matches spec nicely.
> >
> > I would agree with the removal of virtio-*-0.9 only if we decide
> > to drop support for disable-modern=true.
>
> See above. We can't drop disable-modern.
Good point. But this doesn't require it to be a supported device
option for users/management. Maybe we should rename it to
"x-disable-modern" (but that's a separate discussion).
>
> > If we still want to provide a legacy-only virtio device, it should be
> > a separate device type.
>
> I can't see a reason why we should provide legacy-only virtio devices.
If the only reason for disable-modern to exist is for internal
usage by machine-type code, I agree we don't need the -0.9 device
types.
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-16 13:32 ` Eduardo Habkost
@ 2018-10-16 15:51 ` Cornelia Huck
0 siblings, 0 replies; 41+ messages in thread
From: Cornelia Huck @ 2018-10-16 15:51 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, Caio Carrara, Fabian Deutsch,
Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel,
Gonglei, Gerd Hoffmann, Laine Stump, Cleber Rosa,
Andrea Bolognani, Philippe Mathieu-Daudé
On Tue, 16 Oct 2018 10:32:20 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 16, 2018 at 10:39:30AM +0200, Cornelia Huck wrote:
> > So, what I'd propose is:
> > - virtio-*-pci-standard: compliant with the virtio standard 1.0 or
> > later; no legacy fallback
> > - virtio-*-pci-transitional: compliant with the virtio standard 1.0 or
> > later; fallback to legacy included, as specified by the standard
> > (- virtio-*-pci-legacy: legacy devices, should we need that for compat
> > reasons)
> >
> > We could also use '-virtio-1' instead of '-standard', if we do a major
> > break with a 2.x standard (I don't see it yet). But having a new type
> > for 1.1 sounds wrong.
>
> That's true: adding a new type for 1.1 is probably going to be
> wrong.
>
> But how can I make any promises about the existing device type
> being compatible with 1.1 (or 1.2, 1.3...), if the 1.1 (or 1.2,
> 1.3...) specification wasn't released yet?
I think the *goal* is to keep them compatible. We'll probably want to
switch to virtio-2 should we want to do an explicit break in the future.
>
> Maybe using "-virtio-1" would be a good way to be clear we're
> talking about virtio-1.x without making any promises about 1.1,
> 1.2, 1.3, etc.
It would fit the way this is supposed to work, yes.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-15 18:14 ` Eduardo Habkost
2018-10-16 8:39 ` Cornelia Huck
@ 2018-10-16 17:02 ` Daniel P. Berrangé
2018-10-16 19:12 ` Laine Stump
1 sibling, 1 reply; 41+ messages in thread
From: Daniel P. Berrangé @ 2018-10-16 17:02 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara, Gerd Hoffmann, Andrea Bolognani,
Fabian Deutsch
On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
> On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > 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 is 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-0.9: legacy virtio device
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0: modern-only
> > > - Supports both Conventional PCI and PCI Express buses
> >
> > I would prefer a "modern" suffix since it will likely cover future
> > revisions as well.
>
> That's on purpose. The new device types don't cover future
> revisions, otherwise we'll have exactly the same ambiguity
> problems in the future.
>
> The moment we have a new virtio specification released, a device
> type called "modern" will automatically become ambiguous.
Agreed, I don't want to see us back in the same mess, if a
virtio-2.0 ever gets released with non-backcompatible
changes.
How about using only the major digit in the device names eg
'virtio-blk-0.x'
'virtio-blk-1.x'
to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
by the same device.
This assumes that virtio authors are indeed using semantic
versioning where minor digit indicates backcompat changes
and major digit indicates breaking compat changes.
> > Besides, I don't have a problem with this but I'd like an
> > ack from someone on the management side, confirming
> > these new interfaces are actually going to be used.
> >
> > Could you copy some relevant people as well pls?
>
> CCing Andrea, Daniel, and Laine from the libvirt side.
I don't have a objection from libvirt side.
Last time, I suggested/discussed this I was not convinced that the benefit
was compelling enough to justify the work across all levels in the stack.
Apps using the new device model names would either make themselves
incompatible with older libvirt/QEMU, or they would increase their
code complexity & testing matrix by having to support both old & new
names. The usage would also harm migration to older hosts.
This just to be able to switch from i440fx to q35 for OS which don't
support virtio-1.0, but for such old OS, q35 isn't offering any
compelling features, so they might as well stick with the thing that
is known to work well.
If QEMU supports this, we'd support it in libvirt, but my recommendation
to apps would still likely be to not use it and simply stick with i440fx
for such older OS.
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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-16 13:39 ` Eduardo Habkost
@ 2018-10-16 18:42 ` Gerd Hoffmann
2018-10-17 5:49 ` [Qemu-devel] "no-user" for properties (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: Gerd Hoffmann @ 2018-10-16 18:42 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Laine Stump, Caio Carrara
Hi,
> > See above. We can't drop disable-modern.
>
> Good point. But this doesn't require it to be a supported device
> option for users/management. Maybe we should rename it to
> "x-disable-modern" (but that's a separate discussion).
I think it would be more useful to allow properties being tagged as
"no-user", simliar to devices which we do not want be created via
-device.
> > > If we still want to provide a legacy-only virtio device, it should be
> > > a separate device type.
> >
> > I can't see a reason why we should provide legacy-only virtio devices.
>
> If the only reason for disable-modern to exist is for internal
> usage by machine-type code, I agree we don't need the -0.9 device
> types.
In the early virtio-1.0 days it was required for testing. We had one or
two releases with virtio-1.0 support merged but not (yet) turned on by
default, so you had to use disable-modern=off to enable virtio-1.0
support. That use case is gone though since we flipped the default.
cheers,
Gerd
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-16 17:02 ` Daniel P. Berrangé
@ 2018-10-16 19:12 ` Laine Stump
2018-10-17 5:57 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
2018-10-17 10:43 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Andrea Bolognani
0 siblings, 2 replies; 41+ messages in thread
From: Laine Stump @ 2018-10-16 19:12 UTC (permalink / raw)
To: Daniel P. Berrangé, Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Andrea Bolognani, Fabian Deutsch
On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote:
> On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
>> On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
>>> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
>>>> 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 is 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-0.9: legacy virtio device
>>>> - Supports Conventional PCI buses only, because
>>>> it has a PIO BAR
>>>> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
>>>> - Supports Conventional PCI buses only, because
>>>> it has a PIO BAR
>>>> - virtio-*-pci-1.0: modern-only
>>>> - Supports both Conventional PCI and PCI Express buses
>>>
>>> I would prefer a "modern" suffix since it will likely cover future
>>> revisions as well.
>>
>> That's on purpose. The new device types don't cover future
>> revisions, otherwise we'll have exactly the same ambiguity
>> problems in the future.
>>
>> The moment we have a new virtio specification released, a device
>> type called "modern" will automatically become ambiguous.
>
> Agreed, I don't want to see us back in the same mess, if a
> virtio-2.0 ever gets released with non-backcompatible
> changes.
>
> How about using only the major digit in the device names eg
>
> 'virtio-blk-0.x'
> 'virtio-blk-1.x'
>
> to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
> by the same device.
+1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern".
>
> This assumes that virtio authors are indeed using semantic
> versioning where minor digit indicates backcompat changes
> and major digit indicates breaking compat changes.
>
>
>>> Besides, I don't have a problem with this but I'd like an
>>> ack from someone on the management side, confirming
>>> these new interfaces are actually going to be used.
>>>
>>> Could you copy some relevant people as well pls?
>>
>> CCing Andrea, Daniel, and Laine from the libvirt side.
>
> I don't have a objection from libvirt side.
>
> Last time, I suggested/discussed this I was not convinced that the benefit
> was compelling enough to justify the work across all levels in the stack.
>
> Apps using the new device model names would either make themselves
> incompatible with older libvirt/QEMU, or they would increase their
> code complexity & testing matrix by having to support both old & new
> names. The usage would also harm migration to older hosts.
>
> This just to be able to switch from i440fx to q35 for OS which don't
> support virtio-1.0, but for such old OS, q35 isn't offering any
> compelling features, so they might as well stick with the thing that
> is known to work well.
The *current* compelling reason is to permit management apps to use Q35
for "old" OSes that don't have a driver for virtio-1.0, (and especially
*new* management apps that want to support only Q35 from the start), but
there are other future advantages that will make us appreciate that this
was done. For example, libosinfo currently reports separately that an
supports virtio-0.9 devices and/or virtio-1.0 devices, but a management
app would need to have extra logic to take account of the fact that the
only way to get a virtio-0.9 device would be to place it on a
conventional PCI bus; if qemu offers the two as separate devices then
all the management app has to do is use the device that libosinfo tells
it to use, and it will automatically be placed on the right kind of bus.
(and I've heard from Eduardo that eventually we'll be able to learn the
PCI ID of the devices from qmp introspection, so the management app will
be able to just look for a device ID that is on both the qemu and the OS
list, and use that).
Obviously using these devices will make it impossible to migrate a guest
that uses them to an older host that doesn't have "new" qemu + libvirt,
but if that's important to a management app, then they can just do
things in the more complicated manner needed by the "combined" virtio
device variants. (Again, if a management app is just being
designed/written now, it can assume these new devices from the start and
ignore the older combined device).
In the end, having a device that changed PCI ID depending on what kind
of slot it was plugged into was an idea "too clever for its own good",
should be avoided when new devices are added in the future, and we
should at least provide an alternative that doesn't do that for existing
devices.
>
> If QEMU supports this, we'd support it in libvirt, but my recommendation
> to apps would still likely be to not use it and simply stick with i440fx
> for such older OS.
*maybe* for existing apps. But for any new app that doesn't have older
versions of hosts to worry about, I would say using the new versioned
device names will make their code much simpler.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] "no-user" for properties (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices)
2018-10-16 18:42 ` Gerd Hoffmann
@ 2018-10-17 5:49 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2018-10-17 5:49 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Eduardo Habkost, Michael S. Tsirkin, Caio Carrara, qemu-devel,
Wainer dos Santos Moschetta, Gonglei, Laine Stump, Cleber Rosa,
Philippe Mathieu-Daudé
Gerd Hoffmann <kraxel@redhat.com> writes:
> Hi,
>
>> > See above. We can't drop disable-modern.
>>
>> Good point. But this doesn't require it to be a supported device
>> option for users/management. Maybe we should rename it to
>> "x-disable-modern" (but that's a separate discussion).
>
> I think it would be more useful to allow properties being tagged as
> "no-user", simliar to devices which we do not want be created via
> -device.
You mean user_creatable, which replaced
cannot_instantiate_with_device_add_yet, which used to be called no_user.
Yes, having something like that for properties would be a better user
interface than our current, lazy one "I'm showing you a bunch of stuff
you're supposed to ignore, and you're supposed to divine that from the
'x-' prefix". Patches?
[...]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices)
2018-10-16 19:12 ` Laine Stump
@ 2018-10-17 5:57 ` Markus Armbruster
2018-10-17 6:34 ` Gerd Hoffmann
2018-10-17 15:56 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Eduardo Habkost
2018-10-17 10:43 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Andrea Bolognani
1 sibling, 2 replies; 41+ messages in thread
From: Markus Armbruster @ 2018-10-17 5:57 UTC (permalink / raw)
To: Laine Stump
Cc: Daniel P. Berrangé,
Eduardo Habkost, Michael S. Tsirkin, Caio Carrara,
Fabian Deutsch, qemu-devel, Wainer dos Santos Moschetta, Gonglei,
Gerd Hoffmann, Andrea Bolognani, Cleber Rosa,
Philippe Mathieu-Daudé
Laine Stump <laine@redhat.com> writes:
[...]
> In the end, having a device that changed PCI ID depending on what kind
> of slot it was plugged into was an idea "too clever for its own good",
> should be avoided when new devices are added in the future, and we
> should at least provide an alternative that doesn't do that for existing
> devices.
That means for each chameleon PCI/PCIe device:
* create a pair of devices that can only go into one kind of slot
* deprecate the chameleon
Yes, please! Volunteers?
Do we have similar chameleons outside PCI?
[...]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices)
2018-10-17 5:57 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
@ 2018-10-17 6:34 ` Gerd Hoffmann
2018-10-17 11:56 ` [Qemu-devel] No more chameleon devices Markus Armbruster
2018-10-17 15:56 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Eduardo Habkost
1 sibling, 1 reply; 41+ messages in thread
From: Gerd Hoffmann @ 2018-10-17 6:34 UTC (permalink / raw)
To: Markus Armbruster
Cc: Laine Stump, Daniel P. Berrangé,
Eduardo Habkost, Michael S. Tsirkin, Caio Carrara,
Fabian Deutsch, qemu-devel, Wainer dos Santos Moschetta, Gonglei,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé
On Wed, Oct 17, 2018 at 07:57:39AM +0200, Markus Armbruster wrote:
> Laine Stump <laine@redhat.com> writes:
>
> [...]
> > In the end, having a device that changed PCI ID depending on what kind
> > of slot it was plugged into was an idea "too clever for its own good",
> > should be avoided when new devices are added in the future, and we
> > should at least provide an alternative that doesn't do that for existing
> > devices.
>
> That means for each chameleon PCI/PCIe device:
>
> * create a pair of devices that can only go into one kind of slot
> * deprecate the chameleon
I think virtio devices are the only ones which actually change the pci
id and have non-trivial differences.
qemu-xhci can likewise be plugged into both pci and pcie slots. When
plugged into a pcie slot it'll have pcie endpoint capability. That is
the only difference though. Do you consider that a chameleon device?
cheers,
Gerd
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-13 2:54 [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
` (2 preceding siblings ...)
2018-10-15 9:45 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Cornelia Huck
@ 2018-10-17 9:22 ` Stefan Hajnoczi
2018-10-17 15:10 ` Eduardo Habkost
3 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2018-10-17 9:22 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Caio Carrara, Markus Armbruster,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann, Laine Stump,
Cleber Rosa, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]
On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> 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-0.9: legacy virtio device
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0: modern-only
> - Supports both Conventional PCI and PCI Express buses
If new device types are being created, is it time to decouple the VIRTIO
PCI transport from the actual VIRTIO device (blk, net, scsi) like we
already have with virtio-mmio?
-device virtio-pci-1.0,id=virtio-pci-0
-device virtio-blk,bus=virtio-pci-0,drive=drive0,serial=mydisk
That way we avoid lots of boilerplate code and an explosion of new
device types.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-16 19:12 ` Laine Stump
2018-10-17 5:57 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
@ 2018-10-17 10:43 ` Andrea Bolognani
2018-10-17 15:01 ` Eduardo Habkost
1 sibling, 1 reply; 41+ messages in thread
From: Andrea Bolognani @ 2018-10-17 10:43 UTC (permalink / raw)
To: Laine Stump, Daniel P. Berrangé, Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Fabian Deutsch
On Tue, 2018-10-16 at 15:12 -0400, Laine Stump wrote:
> On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote:
> > On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
> > How about using only the major digit in the device names eg
> >
> > 'virtio-blk-0.x'
> > 'virtio-blk-1.x'
> >
> > to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
> > by the same device.
>
> +1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern".
Agreed on using the major version number rather than a non-specific
string, and since the number refers to the virtio protocol version
I would expect the result to be
virtio-0-blk-pci
virtio-1-blk-pci
and so on.
The proposal doesn't directly address the interaction between virtio
protocol version and slot type. Admittedly, I don't recall all the
details myself, but the point is that I would like to see the slot
type mentioned explicitly in the device name to avoid confusion, so
the above might end up looking more like
virtio-0-blk-pci
virtio-1-blk-pci
virtio-1-blk-pcie
with the last one very clearly not being usable on i440fx. I might
have gotten some details wrong in the example, but you get the idea.
[...]
> > Apps using the new device model names would either make themselves
> > incompatible with older libvirt/QEMU, or they would increase their
> > code complexity & testing matrix by having to support both old & new
> > names. The usage would also harm migration to older hosts.
> >
> > This just to be able to switch from i440fx to q35 for OS which don't
> > support virtio-1.0, but for such old OS, q35 isn't offering any
> > compelling features, so they might as well stick with the thing that
> > is known to work well.
>
> The *current* compelling reason is to permit management apps to use Q35
> for "old" OSes that don't have a driver for virtio-1.0, (and especially
> *new* management apps that want to support only Q35 from the start), but
> there are other future advantages that will make us appreciate that this
> was done. For example, libosinfo currently reports separately that an
> supports virtio-0.9 devices and/or virtio-1.0 devices, but a management
> app would need to have extra logic to take account of the fact that the
> only way to get a virtio-0.9 device would be to place it on a
> conventional PCI bus; if qemu offers the two as separate devices then
> all the management app has to do is use the device that libosinfo tells
> it to use, and it will automatically be placed on the right kind of bus.
> (and I've heard from Eduardo that eventually we'll be able to learn the
> PCI ID of the devices from qmp introspection, so the management app will
> be able to just look for a device ID that is on both the qemu and the OS
> list, and use that).
>
> Obviously using these devices will make it impossible to migrate a guest
> that uses them to an older host that doesn't have "new" qemu + libvirt,
> but if that's important to a management app, then they can just do
> things in the more complicated manner needed by the "combined" virtio
> device variants. (Again, if a management app is just being
> designed/written now, it can assume these new devices from the start and
> ignore the older combined device).
>
> In the end, having a device that changed PCI ID depending on what kind
> of slot it was plugged into was an idea "too clever for its own good",
> should be avoided when new devices are added in the future, and we
> should at least provide an alternative that doesn't do that for existing
> devices.
Agreed, the current situation is kind of a mess and taking steps
towards solving it will pay off in the long term.
At the same time, we should not fool ourselves into thinking it will
take less than *years* before applications such as virt-manager can
actually take advantage of the new devices without compromising their
support for old libvirt and QEMU versions too much.
So if we're doing this to rectify some questionable design choices
with the goal of having a better situation in the long run, then by
all means we should go ahead; but if we think this will allow us to
run RHEL 6 on q35 in the short term, then our efforts are probably
misguided.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-17 6:34 ` Gerd Hoffmann
@ 2018-10-17 11:56 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2018-10-17 11:56 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Eduardo Habkost, Michael S. Tsirkin, Cleber Rosa, Fabian Deutsch,
qemu-devel, Wainer dos Santos Moschetta, Andrea Bolognani,
Gonglei, Laine Stump, Caio Carrara, Philippe Mathieu-Daudé
Gerd Hoffmann <kraxel@redhat.com> writes:
> On Wed, Oct 17, 2018 at 07:57:39AM +0200, Markus Armbruster wrote:
>> Laine Stump <laine@redhat.com> writes:
>>
>> [...]
>> > In the end, having a device that changed PCI ID depending on what kind
>> > of slot it was plugged into was an idea "too clever for its own good",
>> > should be avoided when new devices are added in the future, and we
>> > should at least provide an alternative that doesn't do that for existing
>> > devices.
>>
>> That means for each chameleon PCI/PCIe device:
>>
>> * create a pair of devices that can only go into one kind of slot
>> * deprecate the chameleon
>
> I think virtio devices are the only ones which actually change the pci
> id and have non-trivial differences.
>
> qemu-xhci can likewise be plugged into both pci and pcie slots. When
> plugged into a pcie slot it'll have pcie endpoint capability. That is
> the only difference though. Do you consider that a chameleon device?
Since the enticing simplicity of letting PCI device models go into PCIe
slots as well is exactly what lured us into this mess, I do.
I believe all we actually saved by creating chameleons was a bunch of
QOM types. New, non-chameleon PCIe devices would have shared the actual
device model code with the existing, non-chameleon PCI devices.
Correcting the design mistake now involves yet another set of QOM types,
for backward compatibility.
A loss of simplicity we failed to consider properly back then was at the
external interface. Traditionally, a qdev has a bus type (printed by
-device help), and it can go into a slot provided by such a bus. But
chameleon PCI qdevs can also go into a slot provided by a PCIe bus. The
interface is now less regular, for no convincing reason.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-17 10:43 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Andrea Bolognani
@ 2018-10-17 15:01 ` Eduardo Habkost
2018-10-17 15:06 ` Michael S. Tsirkin
2018-10-18 10:25 ` Andrea Bolognani
0 siblings, 2 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-17 15:01 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Laine Stump, Daniel P. Berrangé,
Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Fabian Deutsch
On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-10-16 at 15:12 -0400, Laine Stump wrote:
> > On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote:
> > > On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
> > > How about using only the major digit in the device names eg
> > >
> > > 'virtio-blk-0.x'
> > > 'virtio-blk-1.x'
> > >
> > > to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
> > > by the same device.
> >
> > +1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern".
>
> Agreed on using the major version number rather than a non-specific
> string, and since the number refers to the virtio protocol version
> I would expect the result to be
>
> virtio-0-blk-pci
> virtio-1-blk-pci
>
> and so on.
>
> The proposal doesn't directly address the interaction between virtio
> protocol version and slot type. [...]
It does. See the interface names added to each device type.
> [...] Admittedly, I don't recall all the
> details myself, but the point is that I would like to see the slot
> type mentioned explicitly in the device name to avoid confusion, so
> the above might end up looking more like
>
> virtio-0-blk-pci
> virtio-1-blk-pci
> virtio-1-blk-pcie
>
> details myself, but the point is that I would like to see the slot
> type mentioned explicitly in the device name to avoid confusion, so
> the above might end up looking more like
>
> virtio-0-blk-pci
> virtio-1-blk-pci
> virtio-1-blk-pcie
>
> with the last one very clearly not being usable on i440fx. I might
> have gotten some details wrong in the example, but you get the idea.
The difference between the devices is not just the bus type: it
is a different type of device with different behavior. Using the
bus type to differentiate them would be misleading.
e.g. both modern and transitional virtio devices can be plugged
to Conventional PCI buses, but they have different PCI IDs.
I'm considering doing this in v2:
* Remove the -0.9 device type, because nobody seems to need it
* Add two device types:
* virtio-1-blk-pci-non-transitional
* virtio-1-blk-pci-transitional
This way, we:
* Include only the major version of the spec (so
we don't require new device types for virtio 1.1, 1.2, etc),
* Use terms that come from the Virtio spec itself, to avoid
ambiguity.
>
> [...]
> > > Apps using the new device model names would either make themselves
> > > incompatible with older libvirt/QEMU, or they would increase their
> > > code complexity & testing matrix by having to support both old & new
> > > names. The usage would also harm migration to older hosts.
> > >
> > > This just to be able to switch from i440fx to q35 for OS which don't
> > > support virtio-1.0, but for such old OS, q35 isn't offering any
> > > compelling features, so they might as well stick with the thing that
> > > is known to work well.
> >
> > The *current* compelling reason is to permit management apps to use Q35
> > for "old" OSes that don't have a driver for virtio-1.0, (and especially
> > *new* management apps that want to support only Q35 from the start), but
> > there are other future advantages that will make us appreciate that this
> > was done. For example, libosinfo currently reports separately that an
> > supports virtio-0.9 devices and/or virtio-1.0 devices, but a management
> > app would need to have extra logic to take account of the fact that the
> > only way to get a virtio-0.9 device would be to place it on a
> > conventional PCI bus; if qemu offers the two as separate devices then
> > all the management app has to do is use the device that libosinfo tells
> > it to use, and it will automatically be placed on the right kind of bus.
> > (and I've heard from Eduardo that eventually we'll be able to learn the
> > PCI ID of the devices from qmp introspection, so the management app will
> > be able to just look for a device ID that is on both the qemu and the OS
> > list, and use that).
> >
> > Obviously using these devices will make it impossible to migrate a guest
> > that uses them to an older host that doesn't have "new" qemu + libvirt,
> > but if that's important to a management app, then they can just do
> > things in the more complicated manner needed by the "combined" virtio
> > device variants. (Again, if a management app is just being
> > designed/written now, it can assume these new devices from the start and
> > ignore the older combined device).
> >
> > In the end, having a device that changed PCI ID depending on what kind
> > of slot it was plugged into was an idea "too clever for its own good",
> > should be avoided when new devices are added in the future, and we
> > should at least provide an alternative that doesn't do that for existing
> > devices.
>
> Agreed, the current situation is kind of a mess and taking steps
> towards solving it will pay off in the long term.
>
> At the same time, we should not fool ourselves into thinking it will
> take less than *years* before applications such as virt-manager can
> actually take advantage of the new devices without compromising their
> support for old libvirt and QEMU versions too much.
>
> So if we're doing this to rectify some questionable design choices
> with the goal of having a better situation in the long run, then by
> all means we should go ahead; but if we think this will allow us to
> run RHEL 6 on q35 in the short term, then our efforts are probably
> misguided.
Good point. You might be right about oVirt and OpenStack, but
I'm assuming at least some applications (maybe KubeVirt?) don't
care about supporting old libvirt/QEMU versions and won't have
that problem.
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-17 15:01 ` Eduardo Habkost
@ 2018-10-17 15:06 ` Michael S. Tsirkin
2018-10-17 15:57 ` Eduardo Habkost
2018-10-18 10:25 ` Andrea Bolognani
1 sibling, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2018-10-17 15:06 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Andrea Bolognani, Laine Stump, Daniel P. Berrangé,
qemu-devel, Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Fabian Deutsch
On Wed, Oct 17, 2018 at 12:01:37PM -0300, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote:
> > On Tue, 2018-10-16 at 15:12 -0400, Laine Stump wrote:
> > > On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote:
> > > > On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
> > > > How about using only the major digit in the device names eg
> > > >
> > > > 'virtio-blk-0.x'
> > > > 'virtio-blk-1.x'
> > > >
> > > > to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
> > > > by the same device.
> > >
> > > +1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern".
> >
> > Agreed on using the major version number rather than a non-specific
> > string, and since the number refers to the virtio protocol version
> > I would expect the result to be
> >
> > virtio-0-blk-pci
> > virtio-1-blk-pci
> >
> > and so on.
> >
> > The proposal doesn't directly address the interaction between virtio
> > protocol version and slot type. [...]
>
> It does. See the interface names added to each device type.
>
>
> > [...] Admittedly, I don't recall all the
> > details myself, but the point is that I would like to see the slot
> > type mentioned explicitly in the device name to avoid confusion, so
> > the above might end up looking more like
> >
> > virtio-0-blk-pci
> > virtio-1-blk-pci
> > virtio-1-blk-pcie
> >
> > details myself, but the point is that I would like to see the slot
> > type mentioned explicitly in the device name to avoid confusion, so
> > the above might end up looking more like
> >
> > virtio-0-blk-pci
> > virtio-1-blk-pci
> > virtio-1-blk-pcie
> >
> > with the last one very clearly not being usable on i440fx. I might
> > have gotten some details wrong in the example, but you get the idea.
>
> The difference between the devices is not just the bus type: it
> is a different type of device with different behavior. Using the
> bus type to differentiate them would be misleading.
>
> e.g. both modern and transitional virtio devices can be plugged
> to Conventional PCI buses, but they have different PCI IDs.
>
> I'm considering doing this in v2:
>
> * Remove the -0.9 device type, because nobody seems to need it
> * Add two device types:
> * virtio-1-blk-pci-non-transitional
> * virtio-1-blk-pci-transitional
>
> This way, we:
> * Include only the major version of the spec (so
> we don't require new device types for virtio 1.1, 1.2, etc),
> * Use terms that come from the Virtio spec itself, to avoid
> ambiguity.
I'd say just drop "1" completely then. E.g. transitional and legacy
have same ID's, differences are internal and not interesting to users.
If spec comes up with a new type of device it will come up with a new
term for it, I am sure.
> >
> > [...]
> > > > Apps using the new device model names would either make themselves
> > > > incompatible with older libvirt/QEMU, or they would increase their
> > > > code complexity & testing matrix by having to support both old & new
> > > > names. The usage would also harm migration to older hosts.
> > > >
> > > > This just to be able to switch from i440fx to q35 for OS which don't
> > > > support virtio-1.0, but for such old OS, q35 isn't offering any
> > > > compelling features, so they might as well stick with the thing that
> > > > is known to work well.
> > >
> > > The *current* compelling reason is to permit management apps to use Q35
> > > for "old" OSes that don't have a driver for virtio-1.0, (and especially
> > > *new* management apps that want to support only Q35 from the start), but
> > > there are other future advantages that will make us appreciate that this
> > > was done. For example, libosinfo currently reports separately that an
> > > supports virtio-0.9 devices and/or virtio-1.0 devices, but a management
> > > app would need to have extra logic to take account of the fact that the
> > > only way to get a virtio-0.9 device would be to place it on a
> > > conventional PCI bus; if qemu offers the two as separate devices then
> > > all the management app has to do is use the device that libosinfo tells
> > > it to use, and it will automatically be placed on the right kind of bus.
> > > (and I've heard from Eduardo that eventually we'll be able to learn the
> > > PCI ID of the devices from qmp introspection, so the management app will
> > > be able to just look for a device ID that is on both the qemu and the OS
> > > list, and use that).
> > >
> > > Obviously using these devices will make it impossible to migrate a guest
> > > that uses them to an older host that doesn't have "new" qemu + libvirt,
> > > but if that's important to a management app, then they can just do
> > > things in the more complicated manner needed by the "combined" virtio
> > > device variants. (Again, if a management app is just being
> > > designed/written now, it can assume these new devices from the start and
> > > ignore the older combined device).
> > >
> > > In the end, having a device that changed PCI ID depending on what kind
> > > of slot it was plugged into was an idea "too clever for its own good",
> > > should be avoided when new devices are added in the future, and we
> > > should at least provide an alternative that doesn't do that for existing
> > > devices.
> >
> > Agreed, the current situation is kind of a mess and taking steps
> > towards solving it will pay off in the long term.
> >
> > At the same time, we should not fool ourselves into thinking it will
> > take less than *years* before applications such as virt-manager can
> > actually take advantage of the new devices without compromising their
> > support for old libvirt and QEMU versions too much.
> >
> > So if we're doing this to rectify some questionable design choices
> > with the goal of having a better situation in the long run, then by
> > all means we should go ahead; but if we think this will allow us to
> > run RHEL 6 on q35 in the short term, then our efforts are probably
> > misguided.
>
> Good point. You might be right about oVirt and OpenStack, but
> I'm assuming at least some applications (maybe KubeVirt?) don't
> care about supporting old libvirt/QEMU versions and won't have
> that problem.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-17 9:22 ` Stefan Hajnoczi
@ 2018-10-17 15:10 ` Eduardo Habkost
2018-10-17 15:32 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-17 15:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Michael S. Tsirkin, qemu-devel, Caio Carrara, Markus Armbruster,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann, Laine Stump,
Cleber Rosa, Philippe Mathieu-Daudé
On Wed, Oct 17, 2018 at 10:22:37AM +0100, Stefan Hajnoczi wrote:
> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > 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-0.9: legacy virtio device
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0: modern-only
> > - Supports both Conventional PCI and PCI Express buses
>
> If new device types are being created, is it time to decouple the VIRTIO
> PCI transport from the actual VIRTIO device (blk, net, scsi) like we
> already have with virtio-mmio?
>
> -device virtio-pci-1.0,id=virtio-pci-0
> -device virtio-blk,bus=virtio-pci-0,drive=drive0,serial=mydisk
>
> That way we avoid lots of boilerplate code and an explosion of new
> device types.
I don't think that would work. This would in turn make
virtio-pci-1.0 a chameleon device that changes PCI ID depending
on the backend device plugged to it.
Also, PCI IDs are not the only information contained inside the
virtio-pci subclasses. See all the PCI-specific +
device-type-specific knowledge encoded inside the class_init and
instance_init functions of each virtio-pci subclass.
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-17 15:10 ` Eduardo Habkost
@ 2018-10-17 15:32 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2018-10-17 15:32 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Stefan Hajnoczi, qemu-devel, Caio Carrara, Markus Armbruster,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann, Laine Stump,
Cleber Rosa, Philippe Mathieu-Daudé
On Wed, Oct 17, 2018 at 12:10:04PM -0300, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 10:22:37AM +0100, Stefan Hajnoczi wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > 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-0.9: legacy virtio device
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0: modern-only
> > > - Supports both Conventional PCI and PCI Express buses
> >
> > If new device types are being created, is it time to decouple the VIRTIO
> > PCI transport from the actual VIRTIO device (blk, net, scsi) like we
> > already have with virtio-mmio?
> >
> > -device virtio-pci-1.0,id=virtio-pci-0
> > -device virtio-blk,bus=virtio-pci-0,drive=drive0,serial=mydisk
> >
> > That way we avoid lots of boilerplate code and an explosion of new
> > device types.
>
> I don't think that would work. This would in turn make
> virtio-pci-1.0 a chameleon device that changes PCI ID depending
> on the backend device plugged to it.
>
> Also, PCI IDs are not the only information contained inside the
> virtio-pci subclasses. See all the PCI-specific +
> device-type-specific knowledge encoded inside the class_init and
> instance_init functions of each virtio-pci subclass.
I think we need to draw the line somewhere. We can't make new
classes and bother users each time there is a behaviour
change in the device.
But I also think the transport/device split it also an internal virtio
thing, no one who has not read the spec cares about it.
> --
> Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices)
2018-10-17 5:57 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
2018-10-17 6:34 ` Gerd Hoffmann
@ 2018-10-17 15:56 ` Eduardo Habkost
2018-10-17 16:38 ` Michael S. Tsirkin
2018-10-18 14:11 ` [Qemu-devel] No more chameleon devices Marcel Apfelbaum
1 sibling, 2 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-17 15:56 UTC (permalink / raw)
To: Markus Armbruster
Cc: Laine Stump, Daniel P. Berrangé,
Michael S. Tsirkin, Caio Carrara, Fabian Deutsch, qemu-devel,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé,
Marcel Apfelbaum
(CCing Marcel, in case he has extra details on the complex
Conventional/Express bus/device plugging rules)
On Wed, Oct 17, 2018 at 07:57:39AM +0200, Markus Armbruster wrote:
> Laine Stump <laine@redhat.com> writes:
>
> [...]
> > In the end, having a device that changed PCI ID depending on what kind
> > of slot it was plugged into was an idea "too clever for its own good",
> > should be avoided when new devices are added in the future, and we
> > should at least provide an alternative that doesn't do that for existing
> > devices.
>
> That means for each chameleon PCI/PCIe device:
>
> * create a pair of devices that can only go into one kind of slot
>
> * deprecate the chameleon
>
> Yes, please! Volunteers?
>
> Do we have similar chameleons outside PCI?
I'm worried that we could be trying to address multiple issues at
the same time, and I'm not sure yet if we should address all of
them in one take.
Right now we need to differentiate non-transitional and
transitional virtio devices, for a few reasons:
* They have different PCI IDs;
* Legacy drivers don't work with non-transitional devices;
* Transitional virtio devices can be plugged to Conventional PCI
buses; non-transitional ones can't.
This patch addresses that problem.
You seem to be talking about a different issue:
* Some devices (including transitional virtio) can be plugged on
both Conventional PCI and PCI Express buses (I will call those
devices "hybrid PCI devices").
The former is a practical problem: management software needs to
be able to ask for a transitional virtio device, depending o the
guest OS being run.
Addressing the latter seems more complex (it would affect other
devices, not just virtio), and I don't see which practical
problems it would solve.
I see some problems it wouldn't solve, though: the system
wouldn't be able to represent the fact that transitional virtio
devices can still work on PCI Express buses, as long as they
support PIO bars; or that Conventional PCI devices can be plugged
to PCI Express root buses.
I don't see problems caused by hybrid conventional/express PCI
devices. The original problem with virtio devices was just not
being hybrid, it was lying about being hybrid: non-transitional
virtio devices are hybrid, but transitional virtio devices
aren't.
I wouldn't be against abolishing hybrid PCI devices completely if
somebody volunteers to do the work. I just don't see which
problems this would solve.
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-17 15:06 ` Michael S. Tsirkin
@ 2018-10-17 15:57 ` Eduardo Habkost
0 siblings, 0 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-17 15:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Bolognani, Laine Stump, Daniel P. Berrangé,
qemu-devel, Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Fabian Deutsch
On Wed, Oct 17, 2018 at 11:06:31AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 17, 2018 at 12:01:37PM -0300, Eduardo Habkost wrote:
[...]
> > I'm considering doing this in v2:
> >
> > * Remove the -0.9 device type, because nobody seems to need it
> > * Add two device types:
> > * virtio-1-blk-pci-non-transitional
> > * virtio-1-blk-pci-transitional
> >
> > This way, we:
> > * Include only the major version of the spec (so
> > we don't require new device types for virtio 1.1, 1.2, etc),
> > * Use terms that come from the Virtio spec itself, to avoid
> > ambiguity.
>
> I'd say just drop "1" completely then. E.g. transitional and legacy
> have same ID's, differences are internal and not interesting to users.
> If spec comes up with a new type of device it will come up with a new
> term for it, I am sure.
Sounds good to me. I'll do that in v2.
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices)
2018-10-17 15:56 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Eduardo Habkost
@ 2018-10-17 16:38 ` Michael S. Tsirkin
2018-10-17 19:19 ` Eduardo Habkost
2018-10-18 14:11 ` [Qemu-devel] No more chameleon devices Marcel Apfelbaum
1 sibling, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2018-10-17 16:38 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Markus Armbruster, Laine Stump, Daniel P. Berrangé,
Caio Carrara, Fabian Deutsch, qemu-devel,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé,
Marcel Apfelbaum
On Wed, Oct 17, 2018 at 12:56:37PM -0300, Eduardo Habkost wrote:
> (CCing Marcel, in case he has extra details on the complex
> Conventional/Express bus/device plugging rules)
>
> On Wed, Oct 17, 2018 at 07:57:39AM +0200, Markus Armbruster wrote:
> > Laine Stump <laine@redhat.com> writes:
> >
> > [...]
> > > In the end, having a device that changed PCI ID depending on what kind
> > > of slot it was plugged into was an idea "too clever for its own good",
> > > should be avoided when new devices are added in the future, and we
> > > should at least provide an alternative that doesn't do that for existing
> > > devices.
> >
> > That means for each chameleon PCI/PCIe device:
> >
> > * create a pair of devices that can only go into one kind of slot
> >
> > * deprecate the chameleon
> >
> > Yes, please! Volunteers?
> >
> > Do we have similar chameleons outside PCI?
>
> I'm worried that we could be trying to address multiple issues at
> the same time, and I'm not sure yet if we should address all of
> them in one take.
>
> Right now we need to differentiate non-transitional and
> transitional virtio devices, for a few reasons:
> * They have different PCI IDs;
> * Legacy drivers don't work with non-transitional devices;
> * Transitional virtio devices can be plugged to Conventional PCI
> buses; non-transitional ones can't.
No that last point isn't true at all.
> This patch addresses that problem.
>
> You seem to be talking about a different issue:
> * Some devices (including transitional virtio) can be plugged on
> both Conventional PCI and PCI Express buses (I will call those
> devices "hybrid PCI devices").
>
> The former is a practical problem: management software needs to
> be able to ask for a transitional virtio device, depending o the
> guest OS being run.
>
> Addressing the latter seems more complex (it would affect other
> devices, not just virtio), and I don't see which practical
> problems it would solve.
>
> I see some problems it wouldn't solve, though: the system
> wouldn't be able to represent the fact that transitional virtio
> devices can still work on PCI Express buses, as long as they
> support PIO bars; or that Conventional PCI devices can be plugged
> to PCI Express root buses.
>
> I don't see problems caused by hybrid conventional/express PCI
> devices. The original problem with virtio devices was just not
> being hybrid, it was lying about being hybrid: non-transitional
> virtio devices are hybrid, but transitional virtio devices
> aren't.
>
> I wouldn't be against abolishing hybrid PCI devices completely if
> somebody volunteers to do the work. I just don't see which
> problems this would solve.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices)
2018-10-17 16:38 ` Michael S. Tsirkin
@ 2018-10-17 19:19 ` Eduardo Habkost
0 siblings, 0 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-17 19:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Markus Armbruster, Laine Stump, Daniel P. Berrangé,
Caio Carrara, Fabian Deutsch, qemu-devel,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé,
Marcel Apfelbaum
On Wed, Oct 17, 2018 at 12:38:29PM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 17, 2018 at 12:56:37PM -0300, Eduardo Habkost wrote:
> > (CCing Marcel, in case he has extra details on the complex
> > Conventional/Express bus/device plugging rules)
> >
> > On Wed, Oct 17, 2018 at 07:57:39AM +0200, Markus Armbruster wrote:
> > > Laine Stump <laine@redhat.com> writes:
> > >
> > > [...]
> > > > In the end, having a device that changed PCI ID depending on what kind
> > > > of slot it was plugged into was an idea "too clever for its own good",
> > > > should be avoided when new devices are added in the future, and we
> > > > should at least provide an alternative that doesn't do that for existing
> > > > devices.
> > >
> > > That means for each chameleon PCI/PCIe device:
> > >
> > > * create a pair of devices that can only go into one kind of slot
> > >
> > > * deprecate the chameleon
> > >
> > > Yes, please! Volunteers?
> > >
> > > Do we have similar chameleons outside PCI?
> >
> > I'm worried that we could be trying to address multiple issues at
> > the same time, and I'm not sure yet if we should address all of
> > them in one take.
> >
> > Right now we need to differentiate non-transitional and
> > transitional virtio devices, for a few reasons:
> > * They have different PCI IDs;
> > * Legacy drivers don't work with non-transitional devices;
> > * Transitional virtio devices can be plugged to Conventional PCI
> > buses; non-transitional ones can't.
>
> No that last point isn't true at all.
Sorry, I got it reversed:
* Non-transitional virtio devices can be plugged to PCI Express
buses; transitional ones can't[1].
[1] Well, theoretically, under a few circumstances they could.
We could extend the QMP interfaces in the future to cover
that use case, but I don't think it's worth the effort.
>
> > This patch addresses that problem.
> >
> > You seem to be talking about a different issue:
> > * Some devices (including transitional virtio) can be plugged on
> > both Conventional PCI and PCI Express buses (I will call those
> > devices "hybrid PCI devices").
> >
> > The former is a practical problem: management software needs to
> > be able to ask for a transitional virtio device, depending o the
> > guest OS being run.
> >
> > Addressing the latter seems more complex (it would affect other
> > devices, not just virtio), and I don't see which practical
> > problems it would solve.
> >
> > I see some problems it wouldn't solve, though: the system
> > wouldn't be able to represent the fact that transitional virtio
> > devices can still work on PCI Express buses, as long as they
> > support PIO bars; or that Conventional PCI devices can be plugged
> > to PCI Express root buses.
> >
> > I don't see problems caused by hybrid conventional/express PCI
> > devices. The original problem with virtio devices was just not
> > being hybrid, it was lying about being hybrid: non-transitional
> > virtio devices are hybrid, but transitional virtio devices
> > aren't.
> >
> > I wouldn't be against abolishing hybrid PCI devices completely if
> > somebody volunteers to do the work. I just don't see which
> > problems this would solve.
> >
> > --
> > Eduardo
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-17 15:01 ` Eduardo Habkost
2018-10-17 15:06 ` Michael S. Tsirkin
@ 2018-10-18 10:25 ` Andrea Bolognani
2018-10-18 10:27 ` Daniel P. Berrangé
2018-11-14 18:20 ` Eduardo Habkost
1 sibling, 2 replies; 41+ messages in thread
From: Andrea Bolognani @ 2018-10-18 10:25 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Laine Stump, Daniel P. Berrangé,
Michael S. Tsirkin, qemu-devel, Gonglei,
Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Fabian Deutsch
On Wed, 2018-10-17 at 12:01 -0300, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote:
> > The proposal doesn't directly address the interaction between virtio
> > protocol version and slot type. [...]
>
> It does. See the interface names added to each device type.
Sorry, I might be blind but I'm still not seeing it... I see a bunch
of *-pci devices and exactly zero *-pcie devices. See below.
[...]
> The difference between the devices is not just the bus type: it
> is a different type of device with different behavior. Using the
> bus type to differentiate them would be misleading.
I'm not arguing that we should use the bus type *only* to
differentiate them, but rather that we should *also* differentiate
them by bus type; failing to do so will mean that...
> e.g. both modern and transitional virtio devices can be plugged
> to Conventional PCI buses, but they have different PCI IDs.
... even people who should be very familiar with the topic by now,
like you and me, will get it wrong from time to time, as Michael
helpfully pointed out ;)
> I'm considering doing this in v2:
>
> * Remove the -0.9 device type, because nobody seems to need it
> * Add two device types:
> * virtio-1-blk-pci-non-transitional
> * virtio-1-blk-pci-transitional
>
> This way, we:
> * Include only the major version of the spec (so
> we don't require new device types for virtio 1.1, 1.2, etc),
> * Use terms that come from the Virtio spec itself, to avoid
> ambiguity.
That's quite a mouthful :)
Anyway, whether a device or not is transitional should go next to
the spec version rather than at the end: this will also help with
consistency because we will need -device and -ccw variants of all
these, no?
Can we assume if and when virtio 2.0 comes around it will also have
both a pure variant and a transitional one which is compatible with
virtio 1.0? If so, I would suggest the names should be
virtio-1-blk-pcie (1.x only, PCIe slot)
virtio-1-transitional-blk-pci (transitional, PCI slot)
virtio-1-transitional-blk-pcie (transitional, PCIe slot)
[...]
> > At the same time, we should not fool ourselves into thinking it will
> > take less than *years* before applications such as virt-manager can
> > actually take advantage of the new devices without compromising their
> > support for old libvirt and QEMU versions too much.
> >
> > So if we're doing this to rectify some questionable design choices
> > with the goal of having a better situation in the long run, then by
> > all means we should go ahead; but if we think this will allow us to
> > run RHEL 6 on q35 in the short term, then our efforts are probably
> > misguided.
>
> Good point. You might be right about oVirt and OpenStack, but
> I'm assuming at least some applications (maybe KubeVirt?) don't
> care about supporting old libvirt/QEMU versions and won't have
> that problem.
AFAIK oVirt and OpenStack have been bumping their requirements quite
aggressively with each subsequent release, so it might not take them
that much time to catch up; I was thinking more about applications
like virt-manager, gnome-boxes and libguestfs, which historically
have maintained compatibility with old libvirt/QEMU releases for the
longest time.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-18 10:25 ` Andrea Bolognani
@ 2018-10-18 10:27 ` Daniel P. Berrangé
2018-11-14 18:20 ` Eduardo Habkost
1 sibling, 0 replies; 41+ messages in thread
From: Daniel P. Berrangé @ 2018-10-18 10:27 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Eduardo Habkost, Laine Stump, Michael S. Tsirkin, qemu-devel,
Gonglei, Philippe Mathieu-Daudé,
Wainer dos Santos Moschetta, Cleber Rosa, Markus Armbruster,
Caio Carrara, Gerd Hoffmann, Fabian Deutsch
On Thu, Oct 18, 2018 at 12:25:12PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-10-17 at 12:01 -0300, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote:
> > > The proposal doesn't directly address the interaction between virtio
> > > protocol version and slot type. [...]
> >
> > It does. See the interface names added to each device type.
>
> Sorry, I might be blind but I'm still not seeing it... I see a bunch
> of *-pci devices and exactly zero *-pcie devices. See below.
>
> [...]
> > The difference between the devices is not just the bus type: it
> > is a different type of device with different behavior. Using the
> > bus type to differentiate them would be misleading.
>
> I'm not arguing that we should use the bus type *only* to
> differentiate them, but rather that we should *also* differentiate
> them by bus type; failing to do so will mean that...
>
> > e.g. both modern and transitional virtio devices can be plugged
> > to Conventional PCI buses, but they have different PCI IDs.
>
> ... even people who should be very familiar with the topic by now,
> like you and me, will get it wrong from time to time, as Michael
> helpfully pointed out ;)
>
> > I'm considering doing this in v2:
> >
> > * Remove the -0.9 device type, because nobody seems to need it
> > * Add two device types:
> > * virtio-1-blk-pci-non-transitional
> > * virtio-1-blk-pci-transitional
> >
> > This way, we:
> > * Include only the major version of the spec (so
> > we don't require new device types for virtio 1.1, 1.2, etc),
> > * Use terms that come from the Virtio spec itself, to avoid
> > ambiguity.
>
> That's quite a mouthful :)
>
> Anyway, whether a device or not is transitional should go next to
> the spec version rather than at the end: this will also help with
> consistency because we will need -device and -ccw variants of all
> these, no?
>
> Can we assume if and when virtio 2.0 comes around it will also have
> both a pure variant and a transitional one which is compatible with
> virtio 1.0? If so, I would suggest the names should be
>
> virtio-1-blk-pcie (1.x only, PCIe slot)
> virtio-1-transitional-blk-pci (transitional, PCI slot)
> virtio-1-transitional-blk-pcie (transitional, PCIe slot)
>
> [...]
> > > At the same time, we should not fool ourselves into thinking it will
> > > take less than *years* before applications such as virt-manager can
> > > actually take advantage of the new devices without compromising their
> > > support for old libvirt and QEMU versions too much.
> > >
> > > So if we're doing this to rectify some questionable design choices
> > > with the goal of having a better situation in the long run, then by
> > > all means we should go ahead; but if we think this will allow us to
> > > run RHEL 6 on q35 in the short term, then our efforts are probably
> > > misguided.
> >
> > Good point. You might be right about oVirt and OpenStack, but
> > I'm assuming at least some applications (maybe KubeVirt?) don't
> > care about supporting old libvirt/QEMU versions and won't have
> > that problem.
>
> AFAIK oVirt and OpenStack have been bumping their requirements quite
> aggressively with each subsequent release, so it might not take them
> that much time to catch up; I was thinking more about applications
> like virt-manager, gnome-boxes and libguestfs, which historically
> have maintained compatibility with old libvirt/QEMU releases for the
> longest time.
OpenStack isn't that aggressive - its min is libvirt 1.3.1 and
QEMU 2.5.0
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] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-17 15:56 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Eduardo Habkost
2018-10-17 16:38 ` Michael S. Tsirkin
@ 2018-10-18 14:11 ` Marcel Apfelbaum
2018-10-18 14:15 ` Peter Maydell
1 sibling, 1 reply; 41+ messages in thread
From: Marcel Apfelbaum @ 2018-10-18 14:11 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster
Cc: Laine Stump, Daniel P. Berrangé,
Michael S. Tsirkin, Caio Carrara, Fabian Deutsch, qemu-devel,
Wainer dos Santos Moschetta, Gonglei, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé
Hi Eduardo,
On 10/17/18 6:56 PM, Eduardo Habkost wrote:
> (CCing Marcel, in case he has extra details on the complex
> Conventional/Express bus/device plugging rules)
>
> On Wed, Oct 17, 2018 at 07:57:39AM +0200, Markus Armbruster wrote:
>> Laine Stump <laine@redhat.com> writes:
>>
>> [...]
>>> In the end, having a device that changed PCI ID depending on what kind
>>> of slot it was plugged into was an idea "too clever for its own good",
>>> should be avoided when new devices are added in the future, and we
>>> should at least provide an alternative that doesn't do that for existing
>>> devices.
>> That means for each chameleon PCI/PCIe device:
>>
>> * create a pair of devices that can only go into one kind of slot
>>
>> * deprecate the chameleon
>>
>> Yes, please! Volunteers?
>>
>> Do we have similar chameleons outside PCI?
> I'm worried that we could be trying to address multiple issues at
> the same time, and I'm not sure yet if we should address all of
> them in one take.
>
> Right now we need to differentiate non-transitional and
> transitional virtio devices, for a few reasons:
> * They have different PCI IDs;
> * Legacy drivers don't work with non-transitional devices;
> * Transitional virtio devices can be plugged to Conventional PCI
> buses; non-transitional ones can't.
>
> This patch addresses that problem.
>
> You seem to be talking about a different issue:
> * Some devices (including transitional virtio) can be plugged on
> both Conventional PCI and PCI Express buses (I will call those
> devices "hybrid PCI devices").
>
> The former is a practical problem: management software needs to
> be able to ask for a transitional virtio device, depending o the
> guest OS being run.
>
> Addressing the latter seems more complex (it would affect other
> devices, not just virtio), and I don't see which practical
> problems it would solve.
>
> I see some problems it wouldn't solve, though: the system
> wouldn't be able to represent the fact that transitional virtio
> devices can still work on PCI Express buses, as long as they
> support PIO bars; or that Conventional PCI devices can be plugged
> to PCI Express root buses.
>
> I don't see problems caused by hybrid conventional/express PCI
> devices. The original problem with virtio devices was just not
> being hybrid, it was lying about being hybrid: non-transitional
> virtio devices are hybrid, but transitional virtio devices
> aren't.
>
> I wouldn't be against abolishing hybrid PCI devices completely if
> somebody volunteers to do the work.
+1 -- GSOC Project?
> I just don't see which
> problems this would solve.
Maybe would be a step toward a clean "socket-device" modeling (what goes
where)
and also QEMU emulation would be cleaner since in bare metal you cannot
plug a PCIe device into a PCI slot and vice-versa or have the same device ID
for both a PCI and a PCIe device.
Thanks,
Marcel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:11 ` [Qemu-devel] No more chameleon devices Marcel Apfelbaum
@ 2018-10-18 14:15 ` Peter Maydell
2018-10-18 14:38 ` Daniel P. Berrangé
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2018-10-18 14:15 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: Eduardo Habkost, Markus Armbruster, Michael S. Tsirkin,
Cleber Rosa, Fabian Deutsch, QEMU Developers,
Wainer dos Santos Moschetta, Andrea Bolognani, Gonglei,
Gerd Hoffmann, Laine Stump, Caio Carrara,
Philippe Mathieu-Daudé
On 18 October 2018 at 15:11, Marcel Apfelbaum
<marcel.apfelbaum@gmail.com> wrote:
> Maybe would be a step toward a clean "socket-device" modeling (what goes
> where)
> and also QEMU emulation would be cleaner since in bare metal you cannot
> plug a PCIe device into a PCI slot and vice-versa or have the same device ID
> for both a PCI and a PCIe device.
So the command line would then distinguish "-device ne2k-pci" and
"-device ne2k-pcie", and users need to know whether the machine they're
using implements PCI or PCIe, and use the right device name accordingly?
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:15 ` Peter Maydell
@ 2018-10-18 14:38 ` Daniel P. Berrangé
2018-10-18 14:41 ` Peter Maydell
2018-10-18 18:07 ` Eduardo Habkost
0 siblings, 2 replies; 41+ messages in thread
From: Daniel P. Berrangé @ 2018-10-18 14:38 UTC (permalink / raw)
To: Peter Maydell
Cc: Marcel Apfelbaum, Eduardo Habkost, Michael S. Tsirkin,
Caio Carrara, Fabian Deutsch, QEMU Developers,
Wainer dos Santos Moschetta, Markus Armbruster, Gonglei,
Laine Stump, Gerd Hoffmann, Andrea Bolognani, Cleber Rosa,
Philippe Mathieu-Daudé
On Thu, Oct 18, 2018 at 03:15:31PM +0100, Peter Maydell wrote:
> On 18 October 2018 at 15:11, Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com> wrote:
> > Maybe would be a step toward a clean "socket-device" modeling (what goes
> > where)
> > and also QEMU emulation would be cleaner since in bare metal you cannot
> > plug a PCIe device into a PCI slot and vice-versa or have the same device ID
> > for both a PCI and a PCIe device.
>
> So the command line would then distinguish "-device ne2k-pci" and
> "-device ne2k-pcie", and users need to know whether the machine they're
> using implements PCI or PCIe, and use the right device name accordingly?
I can understand the rational for splitting the virtio devices, because
of the way they completely change their functionality, even PCI device ID,
depending on whether plugged into a pci or pcie slot.
I'm not seeing the real world benefit of creating -pci vs -pcie for all
the other non-virtio devices. AFAIK, the existing devices work the same
regardless of what bus they are plugged into. So why would a user/app
want to use such devices ? It feels like extra work for no clear benefit
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] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:38 ` Daniel P. Berrangé
@ 2018-10-18 14:41 ` Peter Maydell
2018-10-18 14:45 ` Marcel Apfelbaum
2018-10-18 18:07 ` Eduardo Habkost
1 sibling, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2018-10-18 14:41 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marcel Apfelbaum, Eduardo Habkost, Michael S. Tsirkin,
Caio Carrara, Fabian Deutsch, QEMU Developers,
Wainer dos Santos Moschetta, Markus Armbruster, Gonglei,
Laine Stump, Gerd Hoffmann, Andrea Bolognani, Cleber Rosa,
Philippe Mathieu-Daudé
On 18 October 2018 at 15:38, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Oct 18, 2018 at 03:15:31PM +0100, Peter Maydell wrote:
>> On 18 October 2018 at 15:11, Marcel Apfelbaum
>> <marcel.apfelbaum@gmail.com> wrote:
>> > Maybe would be a step toward a clean "socket-device" modeling (what goes
>> > where)
>> > and also QEMU emulation would be cleaner since in bare metal you cannot
>> > plug a PCIe device into a PCI slot and vice-versa or have the same device ID
>> > for both a PCI and a PCIe device.
>>
>> So the command line would then distinguish "-device ne2k-pci" and
>> "-device ne2k-pcie", and users need to know whether the machine they're
>> using implements PCI or PCIe, and use the right device name accordingly?
>
> I can understand the rational for splitting the virtio devices, because
> of the way they completely change their functionality, even PCI device ID,
> depending on whether plugged into a pci or pcie slot.
>
> I'm not seeing the real world benefit of creating -pci vs -pcie for all
> the other non-virtio devices. AFAIK, the existing devices work the same
> regardless of what bus they are plugged into. So why would a user/app
> want to use such devices ? It feels like extra work for no clear benefit
Well, I don't see the benefit either, which is why I wanted to check
whether that was what was being proposed here. Conversely, if we're
happy that the ne2k-pci device should be allowed to plug into either
PCI or PCIe, it seems wrong to prohibit a virtio PCI device from
also being flexible that way, because then virtio would be a weird special
case as far as the user is concerned (as the only device where they need
to care about PCI vs PCIe).
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:41 ` Peter Maydell
@ 2018-10-18 14:45 ` Marcel Apfelbaum
2018-10-18 14:48 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Marcel Apfelbaum @ 2018-10-18 14:45 UTC (permalink / raw)
To: Peter Maydell, Daniel P. Berrangé
Cc: Eduardo Habkost, Michael S. Tsirkin, Caio Carrara,
Fabian Deutsch, QEMU Developers, Wainer dos Santos Moschetta,
Markus Armbruster, Gonglei, Laine Stump, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé
On 10/18/18 5:41 PM, Peter Maydell wrote:
> On 18 October 2018 at 15:38, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> On Thu, Oct 18, 2018 at 03:15:31PM +0100, Peter Maydell wrote:
>>> On 18 October 2018 at 15:11, Marcel Apfelbaum
>>> <marcel.apfelbaum@gmail.com> wrote:
>>>> Maybe would be a step toward a clean "socket-device" modeling (what goes
>>>> where)
>>>> and also QEMU emulation would be cleaner since in bare metal you cannot
>>>> plug a PCIe device into a PCI slot and vice-versa or have the same device ID
>>>> for both a PCI and a PCIe device.
>>> So the command line would then distinguish "-device ne2k-pci" and
>>> "-device ne2k-pcie", and users need to know whether the machine they're
>>> using implements PCI or PCIe, and use the right device name accordingly?
>> I can understand the rational for splitting the virtio devices, because
>> of the way they completely change their functionality, even PCI device ID,
>> depending on whether plugged into a pci or pcie slot.
>>
>> I'm not seeing the real world benefit of creating -pci vs -pcie for all
>> the other non-virtio devices. AFAIK, the existing devices work the same
>> regardless of what bus they are plugged into. So why would a user/app
>> want to use such devices ? It feels like extra work for no clear benefit
> Well, I don't see the benefit either, which is why I wanted to check
> whether that was what was being proposed here. Conversely, if we're
> happy that the ne2k-pci device should be allowed to plug into either
> PCI or PCIe, it seems wrong to prohibit a virtio PCI device from
> also being flexible that way, because then virtio would be a weird special
> case as far as the user is concerned (as the only device where they need
> to care about PCI vs PCIe).
They would need to know which device to use.
PCIe machines support PCI devices while PCI machines do not support
PCIe devices.
I think users of the PCI machines would benefit from having a clear list of
supported devices, rather than have to find out if their device
1. Is a hybrid device - QEMU will 'make' it PCI/PCIe
2. Is a PCIe device - would not work on a PCI machine
3. Is a PCI device - all good
But maybe is too much trouble for a not-so-important issue.
Thanks,
Marcel
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:45 ` Marcel Apfelbaum
@ 2018-10-18 14:48 ` Peter Maydell
2018-10-18 15:39 ` Marcel Apfelbaum
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2018-10-18 14:48 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: Daniel P. Berrangé,
Eduardo Habkost, Michael S. Tsirkin, Caio Carrara,
Fabian Deutsch, QEMU Developers, Wainer dos Santos Moschetta,
Markus Armbruster, Gonglei, Laine Stump, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé
On 18 October 2018 at 15:45, Marcel Apfelbaum
<marcel.apfelbaum@gmail.com> wrote:
> PCIe machines support PCI devices while PCI machines do not support
> PCIe devices.
>
> I think users of the PCI machines would benefit from having a clear list of
> supported devices, rather than have to find out if their device
> 1. Is a hybrid device - QEMU will 'make' it PCI/PCIe
> 2. Is a PCIe device - would not work on a PCI machine
> 3. Is a PCI device - all good
Ah, I see. So, today, of all the devices labelled as "pci" which
QEMU has, how many of them are in which of those categories?
(I guess I'm mostly interested in whether what we have is
mostly really-PCI devices with one or two PCIe devices, or
if we're currently (mis-)modelling some PCIe devices as hybrid.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:48 ` Peter Maydell
@ 2018-10-18 15:39 ` Marcel Apfelbaum
0 siblings, 0 replies; 41+ messages in thread
From: Marcel Apfelbaum @ 2018-10-18 15:39 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrangé,
Eduardo Habkost, Michael S. Tsirkin, Caio Carrara,
Fabian Deutsch, QEMU Developers, Wainer dos Santos Moschetta,
Markus Armbruster, Gonglei, Laine Stump, Gerd Hoffmann,
Andrea Bolognani, Cleber Rosa, Philippe Mathieu-Daudé
On 10/18/18 5:48 PM, Peter Maydell wrote:
> On 18 October 2018 at 15:45, Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com> wrote:
>> PCIe machines support PCI devices while PCI machines do not support
>> PCIe devices.
>>
>> I think users of the PCI machines would benefit from having a clear list of
>> supported devices, rather than have to find out if their device
>> 1. Is a hybrid device - QEMU will 'make' it PCI/PCIe
>> 2. Is a PCIe device - would not work on a PCI machine
>> 3. Is a PCI device - all good
> Ah, I see. So, today, of all the devices labelled as "pci" which
> QEMU has, how many of them are in which of those categories?
> (I guess I'm mostly interested in whether what we have is
> mostly really-PCI devices with one or two PCIe devices, or
> if we're currently (mis-)modelling some PCIe devices as hybrid.)
Eduardo added 'marker' interfaces to all PCI/PCIe devices
making it easier to answer the question.
A 'hybrid' devices implements both:
* INTERFACE_CONVENTIONAL_PCI_DEVICE
* INTERFACE_PCIE_DEVICE
interfaces, please see the complete list:
https://paste.fedoraproject.org/paste/vgOo7n7PKi3Wxij~5dZxQQ
The list is not complete as we cannot see devices of derived classes,
like virtio-*-pci, but is good enough.
Thanks,
Marcel
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] No more chameleon devices
2018-10-18 14:38 ` Daniel P. Berrangé
2018-10-18 14:41 ` Peter Maydell
@ 2018-10-18 18:07 ` Eduardo Habkost
1 sibling, 0 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-10-18 18:07 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Maydell, Marcel Apfelbaum, Michael S. Tsirkin,
Caio Carrara, Fabian Deutsch, QEMU Developers,
Wainer dos Santos Moschetta, Markus Armbruster, Gonglei,
Laine Stump, Gerd Hoffmann, Andrea Bolognani, Cleber Rosa,
Philippe Mathieu-Daudé
On Thu, Oct 18, 2018 at 03:38:58PM +0100, Daniel P. Berrangé wrote:
> On Thu, Oct 18, 2018 at 03:15:31PM +0100, Peter Maydell wrote:
> > On 18 October 2018 at 15:11, Marcel Apfelbaum
> > <marcel.apfelbaum@gmail.com> wrote:
> > > Maybe would be a step toward a clean "socket-device" modeling (what goes
> > > where)
> > > and also QEMU emulation would be cleaner since in bare metal you cannot
> > > plug a PCIe device into a PCI slot and vice-versa or have the same device ID
> > > for both a PCI and a PCIe device.
> >
> > So the command line would then distinguish "-device ne2k-pci" and
> > "-device ne2k-pcie", and users need to know whether the machine they're
> > using implements PCI or PCIe, and use the right device name accordingly?
>
> I can understand the rational for splitting the virtio devices, because
> of the way they completely change their functionality, even PCI device ID,
> depending on whether plugged into a pci or pcie slot.
>
> I'm not seeing the real world benefit of creating -pci vs -pcie for all
> the other non-virtio devices. AFAIK, the existing devices work the same
> regardless of what bus they are plugged into. So why would a user/app
> want to use such devices ? It feels like extra work for no clear benefit
Agreed.
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
2018-10-18 10:25 ` Andrea Bolognani
2018-10-18 10:27 ` Daniel P. Berrangé
@ 2018-11-14 18:20 ` Eduardo Habkost
1 sibling, 0 replies; 41+ messages in thread
From: Eduardo Habkost @ 2018-11-14 18:20 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Michael S. Tsirkin, Caio Carrara, Fabian Deutsch, qemu-devel,
Wainer dos Santos Moschetta, Markus Armbruster, Gonglei,
Gerd Hoffmann, Laine Stump, Cleber Rosa,
Philippe Mathieu-Daudé
On Thu, Oct 18, 2018 at 12:25:12PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-10-17 at 12:01 -0300, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote:
> > > The proposal doesn't directly address the interaction between virtio
> > > protocol version and slot type. [...]
> >
> > It does. See the interface names added to each device type.
>
> Sorry, I might be blind but I'm still not seeing it... I see a bunch
> of *-pci devices and exactly zero *-pcie devices. See below.
>
> [...]
> > The difference between the devices is not just the bus type: it
> > is a different type of device with different behavior. Using the
> > bus type to differentiate them would be misleading.
>
> I'm not arguing that we should use the bus type *only* to
> differentiate them, but rather that we should *also* differentiate
> them by bus type; failing to do so will mean that...
>
> > e.g. both modern and transitional virtio devices can be plugged
> > to Conventional PCI buses, but they have different PCI IDs.
>
> ... even people who should be very familiar with the topic by now,
> like you and me, will get it wrong from time to time, as Michael
> helpfully pointed out ;)
>
> > I'm considering doing this in v2:
> >
> > * Remove the -0.9 device type, because nobody seems to need it
> > * Add two device types:
> > * virtio-1-blk-pci-non-transitional
> > * virtio-1-blk-pci-transitional
> >
> > This way, we:
> > * Include only the major version of the spec (so
> > we don't require new device types for virtio 1.1, 1.2, etc),
> > * Use terms that come from the Virtio spec itself, to avoid
> > ambiguity.
>
> That's quite a mouthful :)
>
> Anyway, whether a device or not is transitional should go next to
> the spec version rather than at the end: this will also help with
> consistency because we will need -device and -ccw variants of all
> these, no?
Answering this question: we won't need this on the other variants
they don't have the code that silently breaks compatibility with
legacy drivers depending on the bus it's plugged. The device is
either always transitional or always non-transitional.
The root of the problem is in virtio-pci. More precisely, it
begins at virtio_pci_realize():
if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}
combined with virtio_pci_device_plugged():
if (legacy) {
[...]
pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
} else {
pci_set_word(config + PCI_VENDOR_ID,
PCI_VENDOR_ID_REDHAT_QUMRANET);
pci_set_word(config + PCI_DEVICE_ID,
0x1040 + virtio_bus_get_vdev_id(bus));
pci_config_set_revision(config, 1);
}
>
> Can we assume if and when virtio 2.0 comes around it will also have
> both a pure variant and a transitional one which is compatible with
> virtio 1.0? If so, I would suggest the names should be
>
> virtio-1-blk-pcie (1.x only, PCIe slot)
> virtio-1-transitional-blk-pci (transitional, PCI slot)
> virtio-1-transitional-blk-pcie (transitional, PCIe slot)
I don't see any need to make separate transitional-pci and
transitional-pcie device types. A -transitional device type that
supports both Conventional PCI and PCI Express will work and I
don't see any problem with that.
For reference, the next version of this patch will have 3 device
variants:
* virtio-blk-pci (existing device, for compatibility. Contains
magic transitional/non-transitional logic depending on bus)
* virtio-blk-pci-transitional (1.x transitional, supports legacy
drivers, Conventional PCI only)
* virtio-blk-pci-non-transitional (1.x non-transitional, no
legacy driver support, supports both Conventional PCI and PCI
Express)
--
Eduardo
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2018-11-14 18:20 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-13 2:54 [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Eduardo Habkost
2018-10-14 21:35 ` Michael S. Tsirkin
2018-10-15 18:14 ` Eduardo Habkost
2018-10-16 8:39 ` Cornelia Huck
2018-10-16 13:32 ` Eduardo Habkost
2018-10-16 15:51 ` Cornelia Huck
2018-10-16 17:02 ` Daniel P. Berrangé
2018-10-16 19:12 ` Laine Stump
2018-10-17 5:57 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
2018-10-17 6:34 ` Gerd Hoffmann
2018-10-17 11:56 ` [Qemu-devel] No more chameleon devices Markus Armbruster
2018-10-17 15:56 ` [Qemu-devel] No more chameleon devices (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Eduardo Habkost
2018-10-17 16:38 ` Michael S. Tsirkin
2018-10-17 19:19 ` Eduardo Habkost
2018-10-18 14:11 ` [Qemu-devel] No more chameleon devices Marcel Apfelbaum
2018-10-18 14:15 ` Peter Maydell
2018-10-18 14:38 ` Daniel P. Berrangé
2018-10-18 14:41 ` Peter Maydell
2018-10-18 14:45 ` Marcel Apfelbaum
2018-10-18 14:48 ` Peter Maydell
2018-10-18 15:39 ` Marcel Apfelbaum
2018-10-18 18:07 ` Eduardo Habkost
2018-10-17 10:43 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Andrea Bolognani
2018-10-17 15:01 ` Eduardo Habkost
2018-10-17 15:06 ` Michael S. Tsirkin
2018-10-17 15:57 ` Eduardo Habkost
2018-10-18 10:25 ` Andrea Bolognani
2018-10-18 10:27 ` Daniel P. Berrangé
2018-11-14 18:20 ` Eduardo Habkost
2018-10-15 8:16 ` Gerd Hoffmann
2018-10-15 10:27 ` Michael S. Tsirkin
2018-10-15 23:03 ` Eduardo Habkost
2018-10-16 6:48 ` Gerd Hoffmann
2018-10-16 13:39 ` Eduardo Habkost
2018-10-16 18:42 ` Gerd Hoffmann
2018-10-17 5:49 ` [Qemu-devel] "no-user" for properties (was: [PATCH] virtio: Provide version-specific variants of virtio PCI devices) Markus Armbruster
2018-10-15 9:45 ` [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices Cornelia Huck
2018-10-15 23:32 ` Eduardo Habkost
2018-10-17 9:22 ` Stefan Hajnoczi
2018-10-17 15:10 ` Eduardo Habkost
2018-10-17 15:32 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.