[libvirt] [PATCH for-4.0 v2] virtio: Provide version-specific variants of virtio PCI devices
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology) That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus. The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged. This patch adds separate device types for each of those virtio device flavors: - virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers - Supports Conventional PCI buses only, because it has a PIO BAR - virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses 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. Note that devices that are always non-transitional won't have the -non-transitional variants added, because it would be redundant. 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 Changes v1 -> v2: * Removed *-0.9 devices. Nobody will want to use them, if transitional devices work with legacy drivers (Gerd Hoffmann, Michael S. Tsirkin) * Drop virtio version from name: rename -1.0-transitional to -transitional (Michael S. Tsirkin) * Renamed -1.0 to -non-transitional * Don't add any extra variants to modern-only device types (they don't need it) * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by Cornelia Huck) * No need to change cast macros for modern-only devices * Rename virtio_register_types() to virtio_pci_types_register() Note about points discussed in the v1 thread: Andrea suggested making separate transitional Conventional PCi and transitional PCI Express device types[1]. I didn't do that because I don't see which problems this solves. We have many other device types that are hybrid (support both PCI Express and Conventional PCI) and this was never a problem for us[2]. [1] http://mid.mail-archive.com/6f8d59b8ee2d92d73d2957b520bd8bac989fc796.camel@r... [2] http://mid.mail-archive.com/20181017155637.GC31060@habkost.net --- hw/virtio/virtio-pci.h | 80 +++++++++-- hw/display/virtio-gpu-pci.c | 8 +- hw/display/virtio-vga.c | 8 +- hw/virtio/virtio-crypto-pci.c | 8 +- hw/virtio/virtio-pci.c | 215 ++++++++++++++++++++--------- tests/acceptance/virtio_version.py | 177 ++++++++++++++++++++++++ 6 files changed, 406 insertions(+), 90 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..1d2a11504f 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) @@ -365,7 +375,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_PCI_PREFIX "-base") #define VIRTIO_INPUT_HOST_PCI(obj) \ OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI) @@ -392,7 +403,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) @@ -417,4 +429,48 @@ 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_pci_types_register() + * for details. + */ +typedef struct VirtioPCIDeviceTypeInfo { + /* Prefix for the class 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> type 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 3 flavors: + * - <prefix>-transitional: modern device supporting legacy drivers + * - Supports Conventional PCI buses only + * - <prefix>-non-transitional: modern-only + * - Supports Conventional PCI and PCI Express buses + * - <prefix>: virtio version configurable, legacy driver support + * automatically selected when device is plugged + * _ This was the only flavor supported until QEMU 3.1 + * - Supports Conventional PCI and PCI Express buses + * + * All the types above will inherit from "<prefix>-base", so generic + * code can use "<prefix>-base" on type casts. + * + * We need a separate "<prefix>-base" type instead of making all types inherit + * from <prefix> for two reasons: + * 1) <prefix> will implement INTERFACE_PCIE_DEVICE, but + * <prefix>-transitional won't. + * 2) <prefix> will have the "disable-legacy" and "disable-modern" properties, + * <prefix>-transitional and <prefix>-non-transitional won't. + */ +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t); + #endif diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c index cece4aa495..ca47063b70 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, + .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_pci_types_register(&virtio_gpu_pci_info); } type_init(virtio_gpu_pci_register_types) diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c index ab2e369b28..aaa42a8c31 100644 --- a/hw/display/virtio-vga.c +++ b/hw/display/virtio-vga.c @@ -207,9 +207,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, + .modern_only = true, .instance_size = sizeof(struct VirtIOVGA), .instance_init = virtio_vga_inst_initfn, .class_init = virtio_vga_class_init, @@ -217,7 +217,7 @@ static TypeInfo virtio_vga_info = { static void virtio_vga_register_types(void) { - type_register_static(&virtio_vga_info); + virtio_pci_types_register(&virtio_vga_info); } type_init(virtio_vga_register_types) diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c index bf64996e48..c41dc11f0d 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, + .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_pci_types_register(&virtio_crypto_pci_info); } type_init(virtio_crypto_pci_register_types) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index a954799267..0fa248d68c 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, @@ -1939,13 +1935,104 @@ static const TypeInfo virtio_pci_info = { .class_init = virtio_pci_class_init, .class_size = sizeof(VirtioPCIClass), .abstract = true, - .interfaces = (InterfaceInfo[]) { - { INTERFACE_PCIE_DEVICE }, - { INTERFACE_CONVENTIONAL_PCI_DEVICE }, - { } - }, }; +static Property virtio_pci_generic_properties[] = { + DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy, + ON_OFF_AUTO_AUTO), + DEFINE_PROP_BOOL("disable-modern", VirtIOPCIProxy, disable_modern, false), + DEFINE_PROP_END_OF_LIST(), +}; + +static void virtio_pci_generic_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->props = virtio_pci_generic_properties; +} + +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_pci_types_register(const VirtioPCIDeviceTypeInfo *t) +{ + char *base_name = g_strdup_printf("%s-base", t->name_prefix); + const TypeInfo base_type_info = { + .name = base_name, + .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_name, + .class_init = virtio_pci_generic_class_init, + .interfaces = (InterfaceInfo[]) { + { INTERFACE_PCIE_DEVICE }, + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + }, + }; + + type_register(&base_type_info); + type_register(&generic_type_info); + + if (!t->modern_only) { + char *non_transitional_name = + g_strdup_printf("%s-non-transitional", t->name_prefix); + char *transitional_name = + g_strdup_printf("%s-transitional", t->name_prefix); + const TypeInfo non_transitional_type_info = { + .name = non_transitional_name, + .parent = base_name, + .instance_init = virtio_pci_1_0_instance_init, + .interfaces = (InterfaceInfo[]) { + { INTERFACE_PCIE_DEVICE }, + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + }, + }; + const TypeInfo transitional_type_info = { + .name = transitional_name, + .parent = base_name, + .instance_init = virtio_pci_transitional_instance_init, + .interfaces = (InterfaceInfo[]) { + /* + * Transitional virtio devices work only as Conventional PCI + * devices because they require PIO ports. + */ + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + }, + }; + + type_register(&non_transitional_type_info); + type_register(&transitional_type_info); + + g_free(non_transitional_name); + g_free(transitional_name); + } + + g_free(base_name); +} + /* virtio-blk-pci */ static Property virtio_blk_pci_properties[] = { @@ -1995,9 +2082,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 +2137,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 +2204,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 +2258,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 +2312,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 +2359,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 +2415,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 +2487,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 +2541,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 +2591,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 +2682,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, .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, .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, .parent = TYPE_VIRTIO_INPUT_HID_PCI, + .modern_only = true, .instance_size = sizeof(VirtIOInputHIDPCI), .instance_init = virtio_tablet_initfn, }; @@ -2637,8 +2717,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 +2772,39 @@ static const TypeInfo virtio_pci_bus_info = { static void virtio_pci_register_types(void) { - type_register_static(&virtio_rng_pci_info); + /* Base types: */ + type_register_static(&virtio_pci_bus_info); + type_register_static(&virtio_pci_info); type_register_static(&virtio_input_pci_info); type_register_static(&virtio_input_hid_pci_info); - type_register_static(&virtio_keyboard_pci_info); - type_register_static(&virtio_mouse_pci_info); - type_register_static(&virtio_tablet_pci_info); + + /* Implementations: */ + virtio_pci_types_register(&virtio_rng_pci_info); + virtio_pci_types_register(&virtio_keyboard_pci_info); + virtio_pci_types_register(&virtio_mouse_pci_info); + virtio_pci_types_register(&virtio_tablet_pci_info); #ifdef CONFIG_LINUX - type_register_static(&virtio_host_pci_info); + virtio_pci_types_register(&virtio_host_pci_info); #endif - type_register_static(&virtio_pci_bus_info); - type_register_static(&virtio_pci_info); #ifdef CONFIG_VIRTFS - type_register_static(&virtio_9p_pci_info); + virtio_pci_types_register(&virtio_9p_pci_info); #endif - type_register_static(&virtio_blk_pci_info); + virtio_pci_types_register(&virtio_blk_pci_info); #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX) - type_register_static(&vhost_user_blk_pci_info); + virtio_pci_types_register(&vhost_user_blk_pci_info); #endif - type_register_static(&virtio_scsi_pci_info); - type_register_static(&virtio_balloon_pci_info); - type_register_static(&virtio_serial_pci_info); - type_register_static(&virtio_net_pci_info); + virtio_pci_types_register(&virtio_scsi_pci_info); + virtio_pci_types_register(&virtio_balloon_pci_info); + virtio_pci_types_register(&virtio_serial_pci_info); + virtio_pci_types_register(&virtio_net_pci_info); #ifdef CONFIG_VHOST_SCSI - type_register_static(&vhost_scsi_pci_info); + virtio_pci_types_register(&vhost_scsi_pci_info); #endif #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX) - type_register_static(&vhost_user_scsi_pci_info); + virtio_pci_types_register(&vhost_user_scsi_pci_info); #endif #ifdef CONFIG_VHOST_VSOCK - type_register_static(&vhost_vsock_pci_info); + virtio_pci_types_register(&vhost_vsock_pci_info); #endif } diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py new file mode 100644 index 0000000000..a8d0b20b75 --- /dev/null +++ b/tests/acceptance/virtio_version.py @@ -0,0 +1,177 @@ +""" +Check compatibility of virtio device types +""" +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <ehabkost@redhat.com> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. +import sys +import os + +sys.path.append(os.path.join(os.path.dirname(__file__), "..", "..", "scripts")) +from qemu import QEMUMachine +from avocado_qemu import Test + +# Virtio Device IDs: +VIRTIO_NET = 1 +VIRTIO_BLOCK = 2 +VIRTIO_CONSOLE = 3 +VIRTIO_RNG = 4 +VIRTIO_BALLOON = 5 +VIRTIO_RPMSG = 7 +VIRTIO_SCSI = 8 +VIRTIO_9P = 9 +VIRTIO_RPROC_SERIAL = 11 +VIRTIO_CAIF = 12 +VIRTIO_GPU = 16 +VIRTIO_INPUT = 18 +VIRTIO_VSOCK = 19 +VIRTIO_CRYPTO = 20 + +PCI_VENDOR_ID_REDHAT_QUMRANET = 0x1af4 + +# Device IDs for legacy/transitional devices: +PCI_LEGACY_DEVICE_IDS = { + VIRTIO_NET: 0x1000, + VIRTIO_BLOCK: 0x1001, + VIRTIO_BALLOON: 0x1002, + VIRTIO_CONSOLE: 0x1003, + VIRTIO_SCSI: 0x1004, + VIRTIO_RNG: 0x1005, + VIRTIO_9P: 0x1009, + VIRTIO_VSOCK: 0x1012, +} + +def pci_modern_device_id(virtio_devid): + return virtio_devid + 0x1040 + +def devtype_implements(vm, devtype, implements): + return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)] + +def get_interfaces(vm, devtype, interfaces=['pci-express-device', 'conventional-pci-device']): + return [i for i in interfaces if devtype_implements(vm, devtype, i)] + +class VirtioVersionCheck(Test): + """ + Check if virtio-version-specific device types result in the + same device tree created by `disable-modern` and + `disable-legacy`. + + :avocado: enable + :avocado: tags=x86_64 + """ + + # just in case there are failures, show larger diff: + maxDiff = 4096 + + def run_device(self, devtype, opts=None, machine='pc'): + """ + Run QEMU with `-device DEVTYPE`, return device info from `query-pci` + """ + with QEMUMachine(self.qemu_bin) as vm: + vm.set_machine(machine) + if opts: + devtype += ',' + opts + vm.add_args('-device', '%s,id=devfortest' % (devtype)) + vm.add_args('-S') + vm.launch() + + pcibuses = vm.command('query-pci') + alldevs = [dev for bus in pcibuses for dev in bus['devices']] + import pprint + pprint.pprint(alldevs) + devfortest = [dev for dev in alldevs + if dev['qdev_id'] == 'devfortest'] + return devfortest[0], get_interfaces(vm, devtype) + + + def assert_devids(self, dev, devid, non_transitional=False): + self.assertEqual(dev['id']['vendor'], PCI_VENDOR_ID_REDHAT_QUMRANET) + self.assertEqual(dev['id']['device'], devid) + if non_transitional: + self.assertTrue(0x1040 <= dev['id']['device'] <= 0x107f) + self.assertGreaterEqual(dev['id']['subsystem'], 0x40) + + def check_all_variants(self, qemu_devtype, virtio_devid): + """Check if a virtio device type and its variants behave as expected""" + # Force modern mode: + dev_modern,_ = self.run_device(qemu_devtype, + 'disable-modern=off,disable-legacy=on') + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid), + non_transitional=True) + + # <prefix>-non-transitional device types should be 100% equivalent to + # <prefix>,disable-modern=off,disable-legacy=on + dev_1_0,nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype)) + self.assertEqual(dev_modern, dev_1_0) + + # Force transitional mode: + dev_trans,_ = self.run_device(qemu_devtype, + 'disable-modern=off,disable-legacy=off') + self.assert_devids(dev_trans, PCI_LEGACY_DEVICE_IDS[virtio_devid]) + + # Force legacy mode: + dev_legacy,_ = self.run_device(qemu_devtype, + 'disable-modern=on,disable-legacy=off') + self.assert_devids(dev_legacy, PCI_LEGACY_DEVICE_IDS[virtio_devid]) + + # No options: default to transitional on PC machine-type: + no_opts_pc,generic_ifaces = self.run_device(qemu_devtype) + self.assertEqual(dev_trans, no_opts_pc) + + #TODO: check if plugging on a PCI Express bus will make the + # device non-transitional + #no_opts_q35 = self.run_device(qemu_devtype, machine='q35') + #self.assertEqual(dev_modern, no_opts_q35) + + # <prefix>-transitional device types should be 100% equivalent to + # <prefix>,disable-modern=off,disable-legacy=off + dev_trans,trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype)) + self.assertEqual(dev_trans, dev_trans) + + # ensure the interface information is correct: + self.assertIn('conventional-pci-device', generic_ifaces) + self.assertIn('pci-express-device', generic_ifaces) + + self.assertIn('conventional-pci-device', nt_ifaces) + self.assertIn('pci-express-device', nt_ifaces) + + self.assertIn('conventional-pci-device', trans_ifaces) + self.assertNotIn('pci-express-device', trans_ifaces) + + + def test_conventional_devs(self): + self.check_all_variants('virtio-net-pci', VIRTIO_NET) + # virtio-blk requires 'driver' parameter + #self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK) + self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE) + self.check_all_variants('virtio-rng-pci', VIRTIO_RNG) + self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON) + self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI) + # virtio-9p requires 'fsdev' parameter + #self.check_all_variants('virtio-9p-pci', VIRTIO_9P) + + def check_modern_only(self, qemu_devtype, virtio_devid): + """Check if a modern-only virtio device type behaves as expected""" + # Force modern mode: + dev_modern,_ = self.run_device(qemu_devtype, + 'disable-modern=off,disable-legacy=on') + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid), + non_transitional=True) + + # No options: should be modern anyway + dev_no_opts,ifaces = self.run_device(qemu_devtype) + self.assertEqual(dev_modern, dev_no_opts) + + self.assertIn('conventional-pci-device', ifaces) + self.assertIn('pci-express-device', ifaces) + + def test_modern_only_devs(self): + self.check_modern_only('virtio-vga', VIRTIO_GPU) + self.check_modern_only('virtio-gpu-pci', VIRTIO_GPU) + self.check_modern_only('virtio-mouse-pci', VIRTIO_INPUT) + self.check_modern_only('virtio-tablet-pci', VIRTIO_INPUT) + self.check_modern_only('virtio-keyboard-pci', VIRTIO_INPUT) -- 2.18.0.rc1.1.g3f1ff2140
Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20181114233831.10374-1-ehabkost@redhat.com Type: series Subject: [Qemu-devel] [PATCH for-4.0 v2] virtio: Provide version-specific variants of virtio PCI devices === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 20332af virtio: Provide version-specific variants of virtio PCI devices === OUTPUT BEGIN === Checking PATCH 1/1: virtio: Provide version-specific variants of virtio PCI devices... WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #668: new file mode 100644 ERROR: line over 90 characters #724: FILE: tests/acceptance/virtio_version.py:52: + return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)] ERROR: line over 90 characters #726: FILE: tests/acceptance/virtio_version.py:54: +def get_interfaces(vm, devtype, interfaces=['pci-express-device', 'conventional-pci-device']): WARNING: line over 80 characters #780: FILE: tests/acceptance/virtio_version.py:108: + dev_1_0,nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype)) WARNING: line over 80 characters #804: FILE: tests/acceptance/virtio_version.py:132: + dev_trans,trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype)) total: 2 errors, 3 warnings, 732 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers - Supports Conventional PCI buses only, because it has a PIO BAR
Am I right in thinking that this is basically identical to virtio-*-pci, aside from only being valid for PCI buses ? IOW, libvirt can expose this device even if QEMU does not support it, by simply using the existing device type and only ever placing it in a PCI bus ? If libvirt did this compatibility approach, can you confirm this would be live migration state compatible. ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
Reference to previous discussion that originated this idea: https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
Changes v1 -> v2: * Removed *-0.9 devices. Nobody will want to use them, if transitional devices work with legacy drivers (Gerd Hoffmann, Michael S. Tsirkin) * Drop virtio version from name: rename -1.0-transitional to -transitional (Michael S. Tsirkin) * Renamed -1.0 to -non-transitional * Don't add any extra variants to modern-only device types (they don't need it) * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by Cornelia Huck) * No need to change cast macros for modern-only devices * Rename virtio_register_types() to virtio_pci_types_register()
Note about points discussed in the v1 thread:
Andrea suggested making separate transitional Conventional PCi and transitional PCI Express device types[1]. I didn't do that because I don't see which problems this solves. We have many other device types that are hybrid (support both PCI Express and Conventional PCI) and this was never a problem for us[2].
[1] http://mid.mail-archive.com/6f8d59b8ee2d92d73d2957b520bd8bac989fc796.camel@r... [2] http://mid.mail-archive.com/20181017155637.GC31060@habkost.net --- hw/virtio/virtio-pci.h | 80 +++++++++-- hw/display/virtio-gpu-pci.c | 8 +- hw/display/virtio-vga.c | 8 +- hw/virtio/virtio-crypto-pci.c | 8 +- hw/virtio/virtio-pci.c | 215 ++++++++++++++++++++--------- tests/acceptance/virtio_version.py | 177 ++++++++++++++++++++++++ 6 files changed, 406 insertions(+), 90 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..1d2a11504f 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)
@@ -365,7 +375,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_PCI_PREFIX "-base") #define VIRTIO_INPUT_HOST_PCI(obj) \ OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
@@ -392,7 +403,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)
@@ -417,4 +429,48 @@ 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_pci_types_register() + * for details. + */ +typedef struct VirtioPCIDeviceTypeInfo { + /* Prefix for the class 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> type 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 3 flavors: + * - <prefix>-transitional: modern device supporting legacy drivers + * - Supports Conventional PCI buses only + * - <prefix>-non-transitional: modern-only + * - Supports Conventional PCI and PCI Express buses + * - <prefix>: virtio version configurable, legacy driver support + * automatically selected when device is plugged + * _ This was the only flavor supported until QEMU 3.1 + * - Supports Conventional PCI and PCI Express buses + * + * All the types above will inherit from "<prefix>-base", so generic + * code can use "<prefix>-base" on type casts. + * + * We need a separate "<prefix>-base" type instead of making all types inherit + * from <prefix> for two reasons: + * 1) <prefix> will implement INTERFACE_PCIE_DEVICE, but + * <prefix>-transitional won't. + * 2) <prefix> will have the "disable-legacy" and "disable-modern" properties, + * <prefix>-transitional and <prefix>-non-transitional won't. + */ +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t); + #endif diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c index cece4aa495..ca47063b70 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, + .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_pci_types_register(&virtio_gpu_pci_info); } type_init(virtio_gpu_pci_register_types) diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c index ab2e369b28..aaa42a8c31 100644 --- a/hw/display/virtio-vga.c +++ b/hw/display/virtio-vga.c @@ -207,9 +207,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, + .modern_only = true, .instance_size = sizeof(struct VirtIOVGA), .instance_init = virtio_vga_inst_initfn, .class_init = virtio_vga_class_init, @@ -217,7 +217,7 @@ static TypeInfo virtio_vga_info = {
static void virtio_vga_register_types(void) { - type_register_static(&virtio_vga_info); + virtio_pci_types_register(&virtio_vga_info); }
type_init(virtio_vga_register_types) diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c index bf64996e48..c41dc11f0d 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, + .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_pci_types_register(&virtio_crypto_pci_info); } type_init(virtio_crypto_pci_register_types) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index a954799267..0fa248d68c 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, @@ -1939,13 +1935,104 @@ static const TypeInfo virtio_pci_info = { .class_init = virtio_pci_class_init, .class_size = sizeof(VirtioPCIClass), .abstract = true, - .interfaces = (InterfaceInfo[]) { - { INTERFACE_PCIE_DEVICE }, - { INTERFACE_CONVENTIONAL_PCI_DEVICE }, - { } - }, };
+static Property virtio_pci_generic_properties[] = { + DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy, + ON_OFF_AUTO_AUTO), + DEFINE_PROP_BOOL("disable-modern", VirtIOPCIProxy, disable_modern, false), + DEFINE_PROP_END_OF_LIST(), +}; + +static void virtio_pci_generic_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->props = virtio_pci_generic_properties; +} + +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_pci_types_register(const VirtioPCIDeviceTypeInfo *t) +{ + char *base_name = g_strdup_printf("%s-base", t->name_prefix); + const TypeInfo base_type_info = { + .name = base_name, + .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_name, + .class_init = virtio_pci_generic_class_init, + .interfaces = (InterfaceInfo[]) { + { INTERFACE_PCIE_DEVICE }, + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + }, + }; + + type_register(&base_type_info); + type_register(&generic_type_info); + + if (!t->modern_only) { + char *non_transitional_name = + g_strdup_printf("%s-non-transitional", t->name_prefix); + char *transitional_name = + g_strdup_printf("%s-transitional", t->name_prefix); + const TypeInfo non_transitional_type_info = { + .name = non_transitional_name, + .parent = base_name, + .instance_init = virtio_pci_1_0_instance_init, + .interfaces = (InterfaceInfo[]) { + { INTERFACE_PCIE_DEVICE }, + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + }, + }; + const TypeInfo transitional_type_info = { + .name = transitional_name, + .parent = base_name, + .instance_init = virtio_pci_transitional_instance_init, + .interfaces = (InterfaceInfo[]) { + /* + * Transitional virtio devices work only as Conventional PCI + * devices because they require PIO ports. + */ + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + }, + }; + + type_register(&non_transitional_type_info); + type_register(&transitional_type_info); + + g_free(non_transitional_name); + g_free(transitional_name); + } + + g_free(base_name); +} + /* virtio-blk-pci */
static Property virtio_blk_pci_properties[] = { @@ -1995,9 +2082,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 +2137,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 +2204,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 +2258,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 +2312,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 +2359,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 +2415,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 +2487,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 +2541,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 +2591,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 +2682,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, .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, .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, .parent = TYPE_VIRTIO_INPUT_HID_PCI, + .modern_only = true, .instance_size = sizeof(VirtIOInputHIDPCI), .instance_init = virtio_tablet_initfn, }; @@ -2637,8 +2717,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 +2772,39 @@ static const TypeInfo virtio_pci_bus_info = {
static void virtio_pci_register_types(void) { - type_register_static(&virtio_rng_pci_info); + /* Base types: */ + type_register_static(&virtio_pci_bus_info); + type_register_static(&virtio_pci_info); type_register_static(&virtio_input_pci_info); type_register_static(&virtio_input_hid_pci_info); - type_register_static(&virtio_keyboard_pci_info); - type_register_static(&virtio_mouse_pci_info); - type_register_static(&virtio_tablet_pci_info); + + /* Implementations: */ + virtio_pci_types_register(&virtio_rng_pci_info); + virtio_pci_types_register(&virtio_keyboard_pci_info); + virtio_pci_types_register(&virtio_mouse_pci_info); + virtio_pci_types_register(&virtio_tablet_pci_info); #ifdef CONFIG_LINUX - type_register_static(&virtio_host_pci_info); + virtio_pci_types_register(&virtio_host_pci_info); #endif - type_register_static(&virtio_pci_bus_info); - type_register_static(&virtio_pci_info); #ifdef CONFIG_VIRTFS - type_register_static(&virtio_9p_pci_info); + virtio_pci_types_register(&virtio_9p_pci_info); #endif - type_register_static(&virtio_blk_pci_info); + virtio_pci_types_register(&virtio_blk_pci_info); #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX) - type_register_static(&vhost_user_blk_pci_info); + virtio_pci_types_register(&vhost_user_blk_pci_info); #endif - type_register_static(&virtio_scsi_pci_info); - type_register_static(&virtio_balloon_pci_info); - type_register_static(&virtio_serial_pci_info); - type_register_static(&virtio_net_pci_info); + virtio_pci_types_register(&virtio_scsi_pci_info); + virtio_pci_types_register(&virtio_balloon_pci_info); + virtio_pci_types_register(&virtio_serial_pci_info); + virtio_pci_types_register(&virtio_net_pci_info); #ifdef CONFIG_VHOST_SCSI - type_register_static(&vhost_scsi_pci_info); + virtio_pci_types_register(&vhost_scsi_pci_info); #endif #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX) - type_register_static(&vhost_user_scsi_pci_info); + virtio_pci_types_register(&vhost_user_scsi_pci_info); #endif #ifdef CONFIG_VHOST_VSOCK - type_register_static(&vhost_vsock_pci_info); + virtio_pci_types_register(&vhost_vsock_pci_info); #endif }
diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py new file mode 100644 index 0000000000..a8d0b20b75 --- /dev/null +++ b/tests/acceptance/virtio_version.py @@ -0,0 +1,177 @@ +""" +Check compatibility of virtio device types +""" +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <ehabkost@redhat.com> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. +import sys +import os + +sys.path.append(os.path.join(os.path.dirname(__file__), "..", "..", "scripts")) +from qemu import QEMUMachine +from avocado_qemu import Test + +# Virtio Device IDs: +VIRTIO_NET = 1 +VIRTIO_BLOCK = 2 +VIRTIO_CONSOLE = 3 +VIRTIO_RNG = 4 +VIRTIO_BALLOON = 5 +VIRTIO_RPMSG = 7 +VIRTIO_SCSI = 8 +VIRTIO_9P = 9 +VIRTIO_RPROC_SERIAL = 11 +VIRTIO_CAIF = 12 +VIRTIO_GPU = 16 +VIRTIO_INPUT = 18 +VIRTIO_VSOCK = 19 +VIRTIO_CRYPTO = 20 + +PCI_VENDOR_ID_REDHAT_QUMRANET = 0x1af4 + +# Device IDs for legacy/transitional devices: +PCI_LEGACY_DEVICE_IDS = { + VIRTIO_NET: 0x1000, + VIRTIO_BLOCK: 0x1001, + VIRTIO_BALLOON: 0x1002, + VIRTIO_CONSOLE: 0x1003, + VIRTIO_SCSI: 0x1004, + VIRTIO_RNG: 0x1005, + VIRTIO_9P: 0x1009, + VIRTIO_VSOCK: 0x1012, +} + +def pci_modern_device_id(virtio_devid): + return virtio_devid + 0x1040 + +def devtype_implements(vm, devtype, implements): + return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)] + +def get_interfaces(vm, devtype, interfaces=['pci-express-device', 'conventional-pci-device']): + return [i for i in interfaces if devtype_implements(vm, devtype, i)] + +class VirtioVersionCheck(Test): + """ + Check if virtio-version-specific device types result in the + same device tree created by `disable-modern` and + `disable-legacy`. + + :avocado: enable + :avocado: tags=x86_64 + """ + + # just in case there are failures, show larger diff: + maxDiff = 4096 + + def run_device(self, devtype, opts=None, machine='pc'): + """ + Run QEMU with `-device DEVTYPE`, return device info from `query-pci` + """ + with QEMUMachine(self.qemu_bin) as vm: + vm.set_machine(machine) + if opts: + devtype += ',' + opts + vm.add_args('-device', '%s,id=devfortest' % (devtype)) + vm.add_args('-S') + vm.launch() + + pcibuses = vm.command('query-pci') + alldevs = [dev for bus in pcibuses for dev in bus['devices']] + import pprint + pprint.pprint(alldevs) + devfortest = [dev for dev in alldevs + if dev['qdev_id'] == 'devfortest'] + return devfortest[0], get_interfaces(vm, devtype) + + + def assert_devids(self, dev, devid, non_transitional=False): + self.assertEqual(dev['id']['vendor'], PCI_VENDOR_ID_REDHAT_QUMRANET) + self.assertEqual(dev['id']['device'], devid) + if non_transitional: + self.assertTrue(0x1040 <= dev['id']['device'] <= 0x107f) + self.assertGreaterEqual(dev['id']['subsystem'], 0x40) + + def check_all_variants(self, qemu_devtype, virtio_devid): + """Check if a virtio device type and its variants behave as expected""" + # Force modern mode: + dev_modern,_ = self.run_device(qemu_devtype, + 'disable-modern=off,disable-legacy=on') + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid), + non_transitional=True) + + # <prefix>-non-transitional device types should be 100% equivalent to + # <prefix>,disable-modern=off,disable-legacy=on + dev_1_0,nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype)) + self.assertEqual(dev_modern, dev_1_0) + + # Force transitional mode: + dev_trans,_ = self.run_device(qemu_devtype, + 'disable-modern=off,disable-legacy=off') + self.assert_devids(dev_trans, PCI_LEGACY_DEVICE_IDS[virtio_devid]) + + # Force legacy mode: + dev_legacy,_ = self.run_device(qemu_devtype, + 'disable-modern=on,disable-legacy=off') + self.assert_devids(dev_legacy, PCI_LEGACY_DEVICE_IDS[virtio_devid]) + + # No options: default to transitional on PC machine-type: + no_opts_pc,generic_ifaces = self.run_device(qemu_devtype) + self.assertEqual(dev_trans, no_opts_pc) + + #TODO: check if plugging on a PCI Express bus will make the + # device non-transitional + #no_opts_q35 = self.run_device(qemu_devtype, machine='q35') + #self.assertEqual(dev_modern, no_opts_q35) + + # <prefix>-transitional device types should be 100% equivalent to + # <prefix>,disable-modern=off,disable-legacy=off + dev_trans,trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype)) + self.assertEqual(dev_trans, dev_trans) + + # ensure the interface information is correct: + self.assertIn('conventional-pci-device', generic_ifaces) + self.assertIn('pci-express-device', generic_ifaces) + + self.assertIn('conventional-pci-device', nt_ifaces) + self.assertIn('pci-express-device', nt_ifaces) + + self.assertIn('conventional-pci-device', trans_ifaces) + self.assertNotIn('pci-express-device', trans_ifaces) + + + def test_conventional_devs(self): + self.check_all_variants('virtio-net-pci', VIRTIO_NET) + # virtio-blk requires 'driver' parameter + #self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK) + self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE) + self.check_all_variants('virtio-rng-pci', VIRTIO_RNG) + self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON) + self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI) + # virtio-9p requires 'fsdev' parameter + #self.check_all_variants('virtio-9p-pci', VIRTIO_9P) + + def check_modern_only(self, qemu_devtype, virtio_devid): + """Check if a modern-only virtio device type behaves as expected""" + # Force modern mode: + dev_modern,_ = self.run_device(qemu_devtype, + 'disable-modern=off,disable-legacy=on') + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid), + non_transitional=True) + + # No options: should be modern anyway + dev_no_opts,ifaces = self.run_device(qemu_devtype) + self.assertEqual(dev_modern, dev_no_opts) + + self.assertIn('conventional-pci-device', ifaces) + self.assertIn('pci-express-device', ifaces) + + def test_modern_only_devs(self): + self.check_modern_only('virtio-vga', VIRTIO_GPU) + self.check_modern_only('virtio-gpu-pci', VIRTIO_GPU) + self.check_modern_only('virtio-mouse-pci', VIRTIO_INPUT) + self.check_modern_only('virtio-tablet-pci', VIRTIO_INPUT) + self.check_modern_only('virtio-keyboard-pci', VIRTIO_INPUT) -- 2.18.0.rc1.1.g3f1ff2140
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 :|
On Thu, 15 Nov 2018 10:05:59 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
It's a virtio-1 (not 1.0) device. Otherwise, I like this terminology better.
- Supports Conventional PCI buses only, because it has a PIO BAR
Am I right in thinking that this is basically identical to virtio-*-pci, aside from only being valid for PCI buses ?
IOW, libvirt can expose this device even if QEMU does not support it, by simply using the existing device type and only ever placing it in a PCI bus ?
If libvirt did this compatibility approach, can you confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor disable-modern is set. Then this would have a compatible state AFAICS.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes. [Out of curiosity, libvirt does not do anything with virtio-ccw's max revision attribute, does it? QEMU uses this on a machine-type level for compat handling, but I don't think it is useful beyond that. Fortunately, virtio-ccw does not have complications like the PCI/PCI-Express bus dependency.]
On Thu, Nov 15, 2018 at 11:50:56AM +0100, Cornelia Huck wrote:
On Thu, 15 Nov 2018 10:05:59 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
It's a virtio-1 (not 1.0) device. Otherwise, I like this terminology better.
- Supports Conventional PCI buses only, because it has a PIO BAR
Am I right in thinking that this is basically identical to virtio-*-pci, aside from only being valid for PCI buses ?
IOW, libvirt can expose this device even if QEMU does not support it, by simply using the existing device type and only ever placing it in a PCI bus ?
If libvirt did this compatibility approach, can you confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor disable-modern is set. Then this would have a compatible state AFAICS.
That's ok, as libvirt doesn't expose disable-modern or disable-legacy right now.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes.
[Out of curiosity, libvirt does not do anything with virtio-ccw's max revision attribute, does it? QEMU uses this on a machine-type level for compat handling, but I don't think it is useful beyond that. Fortunately, virtio-ccw does not have complications like the PCI/PCI-Express bus dependency.]
I don't believe we ever set max revision. 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 :|
On Thu, Nov 15, 2018 at 11:50:56AM +0100, Cornelia Huck wrote:
On Thu, 15 Nov 2018 10:05:59 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
It's a virtio-1 (not 1.0) device. Otherwise, I like this terminology better.
- Supports Conventional PCI buses only, because it has a PIO BAR
Am I right in thinking that this is basically identical to virtio-*-pci, aside from only being valid for PCI buses ?
IOW, libvirt can expose this device even if QEMU does not support it, by simply using the existing device type and only ever placing it in a PCI bus ?
If libvirt did this compatibility approach, can you confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor disable-modern is set. Then this would have a compatible state AFAICS.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes.
This is exactly how it is implemented internally, but I'm not promising that this will be kept compatible forever. And I wouldn't like to make that promise unless there's an important use case for that. We could easily ensure it will be compatible in pc-4.0 and older, though. Would that be enough for the use case we have in mind? -- Eduardo
On Mon, Nov 19, 2018 at 10:44:54PM -0200, Eduardo Habkost wrote:
On Thu, Nov 15, 2018 at 11:50:56AM +0100, Cornelia Huck wrote:
On Thu, 15 Nov 2018 10:05:59 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
It's a virtio-1 (not 1.0) device. Otherwise, I like this terminology better.
- Supports Conventional PCI buses only, because it has a PIO BAR
Am I right in thinking that this is basically identical to virtio-*-pci, aside from only being valid for PCI buses ?
IOW, libvirt can expose this device even if QEMU does not support it, by simply using the existing device type and only ever placing it in a PCI bus ?
If libvirt did this compatibility approach, can you confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor disable-modern is set. Then this would have a compatible state AFAICS.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes.
This is exactly how it is implemented internally, but I'm not promising that this will be kept compatible forever. And I wouldn't like to make that promise unless there's an important use case for that.
We could easily ensure it will be compatible in pc-4.0 and older, though. Would that be enough for the use case we have in mind?
I guess that as long as it is compat in all existing machine types, it doesn't matter what new machine types do. Libvirt would only use the back compat stuff with QEMU < 4.0, in which case you can guarantee a machine type < pc-4.0. If libvirt saw a QEMU >= 4.0, it would never use the back compat approach, so it wouldn't matter if >= pc-4.0 were not compatible. ie for live migration it would be a case of QEMU-3.1 + pc-3.1 -----> QEMU_4.0 + pc-3.1 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 :|
On Mon, 19 Nov 2018 22:44:54 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 11:50:56AM +0100, Cornelia Huck wrote:
On Thu, 15 Nov 2018 10:05:59 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
If libvirt did this compatibility approach, can you confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor disable-modern is set. Then this would have a compatible state AFAICS.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes.
This is exactly how it is implemented internally, but I'm not promising that this will be kept compatible forever. And I wouldn't like to make that promise unless there's an important use case for that.
Shouldn't we be able to ensure compatibility by normal virtio feature bit handling, as we have already done in the past?
We could easily ensure it will be compatible in pc-4.0 and older, though. Would that be enough for the use case we have in mind?
On Tue, Nov 20, 2018 at 11:52:27AM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 22:44:54 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 11:50:56AM +0100, Cornelia Huck wrote:
On Thu, 15 Nov 2018 10:05:59 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
If libvirt did this compatibility approach, can you confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional, provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor disable-modern is set. Then this would have a compatible state AFAICS.
- virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old QEMU by simply using the existing device type and setting disable-legacy ? Can you confirm this would be live migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes.
This is exactly how it is implemented internally, but I'm not promising that this will be kept compatible forever. And I wouldn't like to make that promise unless there's an important use case for that.
Shouldn't we be able to ensure compatibility by normal virtio feature bit handling, as we have already done in the past?
Ensuring compatibility is possible, and even likely. I just want to avoid spending effort keeping such a promise unless it's really necessary.
We could easily ensure it will be compatible in pc-4.0 and older, though. Would that be enough for the use case we have in mind?
-- Eduardo
On Wed, 14 Nov 2018 21:38:31 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 813082b0d7..1d2a11504f 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h
(...)
+/** + * VirtioPCIDeviceTypeInfo: + * + * Template for virtio PCI device types. See virtio_pci_types_register() + * for details. + */ +typedef struct VirtioPCIDeviceTypeInfo { + /* Prefix for the class names */ + const char *name_prefix;
<bikeshed>Maybe call this the vpci_name instead? It's not really a prefix in the way I would usually use the term, but rather a type name with a possible suffix tacked onto it.</bikeshed>
+ /* Parent type. If NULL, TYPE_VIRTIO_PCI is used */ + const char *parent; + /* If modern_only is true, only <name_prefix> type 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 3 flavors: + * - <prefix>-transitional: modern device supporting legacy drivers + * - Supports Conventional PCI buses only + * - <prefix>-non-transitional: modern-only + * - Supports Conventional PCI and PCI Express buses + * - <prefix>: virtio version configurable, legacy driver support + * automatically selected when device is plugged + * _ This was the only flavor supported until QEMU 3.1
s/_/-/ s/until/up to/ ?
+ * - Supports Conventional PCI and PCI Express buses + * + * All the types above will inherit from "<prefix>-base", so generic + * code can use "<prefix>-base" on type casts. + * + * We need a separate "<prefix>-base" type instead of making all types inherit + * from <prefix> for two reasons: + * 1) <prefix> will implement INTERFACE_PCIE_DEVICE, but + * <prefix>-transitional won't. + * 2) <prefix> will have the "disable-legacy" and "disable-modern" properties, + * <prefix>-transitional and <prefix>-non-transitional won't.
<bikeshed>I'd formulate this rather as "x implements/has something, y and z do not", as the code actually does that (and does not just intend to do so :)</bikeshed>
+ */ +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t); + #endif
(...)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index a954799267..0fa248d68c 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c
(...)
+static void virtio_pci_1_0_instance_init(Object *obj)
Ditch the _0? I don't expect this to change the name when virtio 1.1 arrives.
+{ + VirtIOPCIProxy *proxy = VIRTIO_PCI(obj); + + proxy->disable_legacy = ON_OFF_AUTO_ON; + proxy->disable_modern = false; +}
(...) After a quick look, this seems fine; have not actually tried to run it yet.
On Thu, Nov 15, 2018 at 12:21:55PM +0100, Cornelia Huck wrote:
On Wed, 14 Nov 2018 21:38:31 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 813082b0d7..1d2a11504f 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h
(...)
+/** + * VirtioPCIDeviceTypeInfo: + * + * Template for virtio PCI device types. See virtio_pci_types_register() + * for details. + */ +typedef struct VirtioPCIDeviceTypeInfo { + /* Prefix for the class names */ + const char *name_prefix;
<bikeshed>Maybe call this the vpci_name instead? It's not really a prefix in the way I would usually use the term, but rather a type name with a possible suffix tacked onto it.</bikeshed>
I'm considering getting rid of all magic type name generation, and make the struct explicit list all the type names to be registered. This way people won't be confused when grepping the code for the type names.
+ /* Parent type. If NULL, TYPE_VIRTIO_PCI is used */ + const char *parent; + /* If modern_only is true, only <name_prefix> type 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 3 flavors: + * - <prefix>-transitional: modern device supporting legacy drivers + * - Supports Conventional PCI buses only + * - <prefix>-non-transitional: modern-only + * - Supports Conventional PCI and PCI Express buses + * - <prefix>: virtio version configurable, legacy driver support + * automatically selected when device is plugged + * _ This was the only flavor supported until QEMU 3.1
s/_/-/ s/until/up to/ ?
Done. Thanks!
+ * - Supports Conventional PCI and PCI Express buses + * + * All the types above will inherit from "<prefix>-base", so generic + * code can use "<prefix>-base" on type casts. + * + * We need a separate "<prefix>-base" type instead of making all types inherit + * from <prefix> for two reasons: + * 1) <prefix> will implement INTERFACE_PCIE_DEVICE, but + * <prefix>-transitional won't. + * 2) <prefix> will have the "disable-legacy" and "disable-modern" properties, + * <prefix>-transitional and <prefix>-non-transitional won't.
<bikeshed>I'd formulate this rather as "x implements/has something, y and z do not", as the code actually does that (and does not just intend to do so :)</bikeshed>
Done. Thanks!
+ */ +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t); + #endif
(...)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index a954799267..0fa248d68c 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c
(...)
+static void virtio_pci_1_0_instance_init(Object *obj)
Ditch the _0? I don't expect this to change the name when virtio 1.1 arrives.
I was going to rename this to virtio_pci_non_transitional_instance_init() for consistency, but I forgot. Thanks for noting.
+{ + VirtIOPCIProxy *proxy = VIRTIO_PCI(obj); + + proxy->disable_legacy = ON_OFF_AUTO_ON; + proxy->disable_modern = false; +}
(...)
After a quick look, this seems fine; have not actually tried to run it yet.
-- Eduardo
On Wed, 2018-11-14 at 21:38 -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers - Supports Conventional PCI buses only, because it has a PIO BAR - virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
So, my understanding was that transitional devices would be suitable for both PCI and PCIe slots and non-transitional devices could only work in PCIe slots, but based on the above it looks like I got it pretty much completely wrong? I'm not too surprised that would be the case, to be honest: keeping this stuff straight in my head has always been a bit of a challenge, so I can't possibly not welcome a proposal like this, which will spell it out a bit more :) Let me try to map the interactions out: * virtio-*-pci-transitional + plugged into PCI slot - shows up as vendor1/device1 + plugged into PCIe slot - doesn't work * virtio-*-pci-non-transitional + plugged into PCI slot - shows up as vendor2/device2 + plugged into PCIe slot - shows up as vendor2/device2 * virtio-*-pci + plugged into PCI slot - shows up as vendor1/device1 (same as virtio-*-pci-transitional) + plugged into PCIe slot - shows up as vendor2/device2 (same as virtio-*-pci-non-transitional) Does that look about right? Once all the various pieces have fallen into place, when adding a device to a guest running a modern OS we would find out through libosinfo that it supports vendor2/device2 (and vendor1/device1 too, I guess?) so we would choose the non-transitional variant and plug it into PCIe when possible (q35) or PCI otherwise (pc); on the other hand, an older guest OS like CentOS 6 will only advertise support for vendor1/device1, so we'd have to use the transitional variant instead and plug it into a PCI slot regardless of the machine type, which more specifically means building a pcie.0 <- pcie-root-port <- pcie-pci-bridge topology for q35 guests. If all of the above is correct, then it sounds like a feasible enough plan to me, though of course it be a long time before users and management applications can rely on these new device types being available in downstream distributions... One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it. And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
On Wed, 2018-11-14 at 21:38 -0200, Eduardo Habkost wrote:
Many of the current virtio-*-pci device types actually represent 3 different types of devices: * virtio 1.0 non-transitional devices * virtio 1.0 transitional devices * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus compatibility QMP interfaces. With this multi-purpose device type, there's no way to tell management software that transitional devices and legacy devices require a Conventional PCI bus.
The multi-purpose device types would also prevent us from telling management software what's the PCI vendor/device ID for them, because their PCI IDs change at runtime depending on the bus where they were plugged.
This patch adds separate device types for each of those virtio device flavors:
- virtio-*-pci: the existing multi-purpose device types - Configurable using `disable-legacy` and `disable-modern` properties - Legacy driver support is automatically enabled/disabled depending on the bus where it is plugged - Supports Conventional PCI and PCI Express buses (but Conventional PCI is incompatible with disable-legacy=off) - Changes PCI vendor/device IDs at runtime - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers - Supports Conventional PCI buses only, because it has a PIO BAR - virtio-*-pci-non-transitional: modern-only - Supports both Conventional PCI and PCI Express buses
So, my understanding was that transitional devices would be suitable for both PCI and PCIe slots and non-transitional devices could only work in PCIe slots, but based on the above it looks like I got it pretty much completely wrong? I'm not too surprised that would be the case, to be honest: keeping this stuff straight in my head has always been a bit of a challenge, so I can't possibly not welcome a proposal like this, which will spell it out a bit more :)
That's possibly my fault. I described it completely wrong in one message in the v1 thread.
Let me try to map the interactions out:
* virtio-*-pci-transitional + plugged into PCI slot - shows up as vendor1/device1 + plugged into PCIe slot - doesn't work
* virtio-*-pci-non-transitional + plugged into PCI slot - shows up as vendor2/device2 + plugged into PCIe slot - shows up as vendor2/device2
* virtio-*-pci + plugged into PCI slot - shows up as vendor1/device1 (same as virtio-*-pci-transitional) + plugged into PCIe slot - shows up as vendor2/device2 (same as virtio-*-pci-non-transitional)
Does that look about right?
Exactly.
Once all the various pieces have fallen into place, when adding a device to a guest running a modern OS we would find out through libosinfo that it supports vendor2/device2 (and vendor1/device1 too, I guess?) so we would choose the non-transitional variant and plug it into PCIe when possible (q35) or PCI otherwise (pc); on the other hand, an older guest OS like CentOS 6 will only advertise support for vendor1/device1, so we'd have to use the transitional variant instead and plug it into a PCI slot regardless of the machine type, which more specifically means building a pcie.0 <- pcie-root-port <- pcie-pci-bridge topology for q35 guests.
If all of the above is correct, then it sounds like a feasible enough plan to me, though of course it be a long time before users and management applications can rely on these new device types being available in downstream distributions...
One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it.
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3. -- Eduardo
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it.
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
On Mon, Nov 19, 2018 at 11:41:05AM +0100, Cornelia Huck wrote:
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it.
That's not the claim. In fact the reverse is true - a major revision can come at any point. The claim is that virtio compatibility is not based on version numbers. And another claim is that you can trust the virtio TC not to overload terminology that it put in the spec. So use that and you should be fine. Come up with your own and end up writing another spec just to document it.
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
Yes I think adding a version there is a mistake. transitional/legacy/non-transitional are spec terms so they are unlikely to change abruptly. OTOH virtio TC can just decide next version is 2.0 on a drop of a hat. And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect. So all these names are there primarily for libvirt's benefit. And the only input from libvirt devs so far has been that it's unclear how does cross version migration work. That needs to be addressed in some way. So can we maybe start with a libvirt domain xml patch, with an implementation for existing QEMU, get that acked, and then just map it back to qemu command line as directly as possible? -- MST
On Mon, 19 Nov 2018 13:07:59 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
On Mon, Nov 19, 2018 at 11:41:05AM +0100, Cornelia Huck wrote:
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
Yes I think adding a version there is a mistake. transitional/legacy/non-transitional are spec terms so they are unlikely to change abruptly. OTOH virtio TC can just decide next version is 2.0 on a drop of a hat.
I don't really expect that to happen on a drop of a hat, though :) It's probably more likely when we either have some really major change (and we messed up if we can't handle that via the virtio compatibility mechanism), or we go up from 1.x because x is getting too large (won't happen in the near future.) But yes, we should be able to handle further updates without any change like the ones complicating things now for virtio-pci, as that was mostly the transition to a proper standard while flushing out some design problems that you only become aware of later.
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect.
I'd argue that it's problematic on platforms where you have different options for virtio transports. What does "model=virtio" mean? Always virtio-pci, if available? Or rather virtio-<transport>, with <transport> being the best option for the machine?
So all these names are there primarily for libvirt's benefit. And the only input from libvirt devs so far has been that it's unclear how does cross version migration work. That needs to be addressed in some way.
So can we maybe start with a libvirt domain xml patch, with an implementation for existing QEMU, get that acked, and then just map it back to qemu command line as directly as possible?
On Mon, Nov 19, 2018 at 07:32:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:07:59 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
On Mon, Nov 19, 2018 at 11:41:05AM +0100, Cornelia Huck wrote:
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
Yes I think adding a version there is a mistake. transitional/legacy/non-transitional are spec terms so they are unlikely to change abruptly. OTOH virtio TC can just decide next version is 2.0 on a drop of a hat.
I don't really expect that to happen on a drop of a hat, though :) It's probably more likely when we either have some really major change (and we messed up if we can't handle that via the virtio compatibility mechanism), or we go up from 1.x because x is getting too large (won't happen in the near future.)
But yes, we should be able to handle further updates without any change like the ones complicating things now for virtio-pci, as that was mostly the transition to a proper standard while flushing out some design problems that you only become aware of later.
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect.
I'd argue that it's problematic on platforms where you have different options for virtio transports. What does "model=virtio" mean? Always virtio-pci, if available? Or rather virtio-<transport>, with <transport> being the best option for the machine?
Most people don't care, for them it's "whatever works". We have this assumption that if we force a choice then people will choose the right thing but in practice they will do what we all do, play with it until it kind of works and leave well alone afterwards. That's at best - at worst give up and use an easier tool.
So all these names are there primarily for libvirt's benefit. And the only input from libvirt devs so far has been that it's unclear how does cross version migration work. That needs to be addressed in some way.
So can we maybe start with a libvirt domain xml patch, with an implementation for existing QEMU, get that acked, and then just map it back to qemu command line as directly as possible?
On Mon, 19 Nov 2018 13:42:58 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
On Mon, Nov 19, 2018 at 07:32:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:07:59 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect.
I'd argue that it's problematic on platforms where you have different options for virtio transports. What does "model=virtio" mean? Always virtio-pci, if available? Or rather virtio-<transport>, with <transport> being the best option for the machine?
Most people don't care, for them it's "whatever works".
We have this assumption that if we force a choice then people will choose the right thing but in practice they will do what we all do, play with it until it kind of works and leave well alone afterwards. That's at best - at worst give up and use an easier tool.
That implies that we (the developers) need to care and make sure that "model=virtio" gets them the best possible transport (i.e. on s390x, that would be ccw unless the user explicitly requests pci; I'm not sure what the situation with mmio is -- probably "use pci whenever possible"?) I think that's what libvirt already gives us today (I hope.) What makes it messy on the pci side is that the "best option" actually depends on what kind of guest the user wants to run (if the guest is too old, you're stuck with transitional; if you want to reap the benefits of PCIe, you need non-transitional...)
On Mon, Nov 19, 2018 at 07:56:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:42:58 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
On Mon, Nov 19, 2018 at 07:32:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:07:59 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect.
I'd argue that it's problematic on platforms where you have different options for virtio transports. What does "model=virtio" mean? Always virtio-pci, if available? Or rather virtio-<transport>, with <transport> being the best option for the machine?
Most people don't care, for them it's "whatever works".
We have this assumption that if we force a choice then people will choose the right thing but in practice they will do what we all do, play with it until it kind of works and leave well alone afterwards. That's at best - at worst give up and use an easier tool.
That implies that we (the developers) need to care and make sure that "model=virtio" gets them the best possible transport (i.e. on s390x, that would be ccw unless the user explicitly requests pci; I'm not sure what the situation with mmio is -- probably "use pci whenever possible"?) I think that's what libvirt already gives us today (I hope.)
What makes it messy on the pci side is that the "best option" actually depends on what kind of guest the user wants to run (if the guest is too old, you're stuck with transitional; if you want to reap the benefits of PCIe, you need non-transitional...)
Well it works now - connect it to a bus and it figures out whether it should do transitional or not. You can force transitional in PCIe anyway but then you are limited to about 15 devices - probably sufficient for most people ... -- MST
On Mon, 2018-11-19 at 14:14 -0500, Michael S. Tsirkin wrote:
On Mon, Nov 19, 2018 at 07:56:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:42:58 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
We have this assumption that if we force a choice then people will choose the right thing but in practice they will do what we all do, play with it until it kind of works and leave well alone afterwards. That's at best - at worst give up and use an easier tool.
That implies that we (the developers) need to care and make sure that "model=virtio" gets them the best possible transport (i.e. on s390x, that would be ccw unless the user explicitly requests pci; I'm not sure what the situation with mmio is -- probably "use pci whenever possible"?) I think that's what libvirt already gives us today (I hope.)
The interface at the libvirt level is exactly "model=virtio", with that ultimately translating to virtio-*-pci or virtio-*-ccw or virtio-*-device or whatever else based on the architecture, machine type and other information about the guest.
What makes it messy on the pci side is that the "best option" actually depends on what kind of guest the user wants to run (if the guest is too old, you're stuck with transitional; if you want to reap the benefits of PCIe, you need non-transitional...)
Well it works now - connect it to a bus and it figures out whether it should do transitional or not. You can force transitional in PCIe anyway but then you are limited to about 15 devices - probably sufficient for most people ...
From the management point of view that's problematic, because libvirt (which takes care of the virtual hardware, including assigning PCI addresses to devices) has no knowledge of the guest OS running on said hardware, and management apps (which know about the guest OS and can figure out its capabilities using libosinfo) don't want to be in
That's not how it works, though: current virtio-*-pci devices will be transitional (and thus support older guest OS) or not based on the kind of slot you plug them into. the business of assigning PCI addresses themselves. Having separate transitional and non-transitional variants solves the issue because now management apps can query libosinfo to figure out whether the guest OS supports non-transitional virtio devices, and based on that they can ask libvirt to use either the transitional or non-transitional variant; from that, libvirt will be able to choose the correct slot for the device. None of the above quite works if we have a single variant that morphs based on the slot, as we have today. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Nov 20, 2018 at 01:27:05PM +0100, Andrea Bolognani wrote:
On Mon, 2018-11-19 at 14:14 -0500, Michael S. Tsirkin wrote:
On Mon, Nov 19, 2018 at 07:56:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:42:58 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
We have this assumption that if we force a choice then people will choose the right thing but in practice they will do what we all do, play with it until it kind of works and leave well alone afterwards. That's at best - at worst give up and use an easier tool.
That implies that we (the developers) need to care and make sure that "model=virtio" gets them the best possible transport (i.e. on s390x, that would be ccw unless the user explicitly requests pci; I'm not sure what the situation with mmio is -- probably "use pci whenever possible"?) I think that's what libvirt already gives us today (I hope.)
The interface at the libvirt level is exactly "model=virtio", with that ultimately translating to virtio-*-pci or virtio-*-ccw or virtio-*-device or whatever else based on the architecture, machine type and other information about the guest.
What makes it messy on the pci side is that the "best option" actually depends on what kind of guest the user wants to run (if the guest is too old, you're stuck with transitional; if you want to reap the benefits of PCIe, you need non-transitional...)
Well it works now - connect it to a bus and it figures out whether it should do transitional or not. You can force transitional in PCIe anyway but then you are limited to about 15 devices - probably sufficient for most people ...
That's not how it works, though: current virtio-*-pci devices will be transitional (and thus support older guest OS) or not based on the kind of slot you plug them into.
From the management point of view that's problematic, because libvirt (which takes care of the virtual hardware, including assigning PCI addresses to devices) has no knowledge of the guest OS running on said hardware, and management apps (which know about the guest OS and can figure out its capabilities using libosinfo) don't want to be in the business of assigning PCI addresses themselves.
Having separate transitional and non-transitional variants solves the issue because now management apps can query libosinfo to figure out whether the guest OS supports non-transitional virtio devices, and based on that they can ask libvirt to use either the transitional or non-transitional variant; from that, libvirt will be able to choose the correct slot for the device.
None of the above quite works if we have a single variant that morphs based on the slot, as we have today.
So can we get an ack on the patchset then?
-- Andrea Bolognani / Red Hat / Virtualization
On Tue, 2018-11-20 at 14:14 -0500, Michael S. Tsirkin wrote:
On Tue, Nov 20, 2018 at 01:27:05PM +0100, Andrea Bolognani wrote:
On Mon, 2018-11-19 at 14:14 -0500, Michael S. Tsirkin wrote:
Well it works now - connect it to a bus and it figures out whether it should do transitional or not. You can force transitional in PCIe anyway but then you are limited to about 15 devices - probably sufficient for most people ...
That's not how it works, though: current virtio-*-pci devices will be transitional (and thus support older guest OS) or not based on the kind of slot you plug them into.
From the management point of view that's problematic, because libvirt (which takes care of the virtual hardware, including assigning PCI addresses to devices) has no knowledge of the guest OS running on said hardware, and management apps (which know about the guest OS and can figure out its capabilities using libosinfo) don't want to be in the business of assigning PCI addresses themselves.
Having separate transitional and non-transitional variants solves the issue because now management apps can query libosinfo to figure out whether the guest OS supports non-transitional virtio devices, and based on that they can ask libvirt to use either the transitional or non-transitional variant; from that, libvirt will be able to choose the correct slot for the device.
None of the above quite works if we have a single variant that morphs based on the slot, as we have today.
So can we get an ack on the patchset then?
Sure thing - whatever it might be worth :) Acked-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Nov 19, 2018 at 07:56:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:42:58 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
On Mon, Nov 19, 2018 at 07:32:38PM +0100, Cornelia Huck wrote:
On Mon, 19 Nov 2018 13:07:59 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote:
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect.
I'd argue that it's problematic on platforms where you have different options for virtio transports. What does "model=virtio" mean? Always virtio-pci, if available? Or rather virtio-<transport>, with <transport> being the best option for the machine?
Most people don't care, for them it's "whatever works".
We have this assumption that if we force a choice then people will choose the right thing but in practice they will do what we all do, play with it until it kind of works and leave well alone afterwards. That's at best - at worst give up and use an easier tool.
That implies that we (the developers) need to care and make sure that "model=virtio" gets them the best possible transport (i.e. on s390x, that would be ccw unless the user explicitly requests pci; I'm not sure what the situation with mmio is -- probably "use pci whenever possible"?) I think that's what libvirt already gives us today (I hope.)
What makes it messy on the pci side is that the "best option" actually depends on what kind of guest the user wants to run (if the guest is too old, you're stuck with transitional; if you want to reap the benefits of PCIe, you need non-transitional...)
Yeah, sometimes we can't make everything work out of the box on the default case. But I think in this case we can make the QEMU command-line a bit more usable if we just cover more recent OSes. However, I wish this kind of usability magic didn't automatically imposed us the burden of keeping guest ABI compatibility too. Keeping ABI compatibility on the machine-friendly device types and interfaces is already hard enough. We already have aliases that automatically select a virtio device type at qdev-monitor.c:qdev_alias_table[], and I don't know if they are supposed to keep a stable guest ABI. -- Eduardo
On Mon, 19 Nov 2018 19:32:32 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
However, I wish this kind of usability magic didn't automatically imposed us the burden of keeping guest ABI compatibility too. Keeping ABI compatibility on the machine-friendly device types and interfaces is already hard enough.
Hm, what are you thinking about here? We can fence off new features in the hw compat section (and we already do so today), so the remaining compat woe is what you are addressing with this patch set, isn't it?
We already have aliases that automatically select a virtio device type at qdev-monitor.c:qdev_alias_table[], and I don't know if they are supposed to keep a stable guest ABI.
As the comment there states, it was a bad idea... when you are using the alias, you can't pass any properties that are unique to the transport anyway. Hopefully the compat code dealt with the disable-modern for old machine types correctly in this case; but I don't think that helps for the bus dilemma. I'm not sure we want to worry about the aliases too much; it might even be a good idea to deprecate them.
On Mon, Nov 19, 2018 at 01:07:59PM -0500, Michael S. Tsirkin wrote: n
On Mon, Nov 19, 2018 at 11:41:05AM +0100, Cornelia Huck wrote:
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it.
That's not the claim. In fact the reverse is true - a major revision can come at any point. The claim is that virtio compatibility is not based on version numbers. And another claim is that you can trust the virtio TC not to overload terminology that it put in the spec. So use that and you should be fine. Come up with your own and end up writing another spec just to document it.
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
Yes I think adding a version there is a mistake. transitional/legacy/non-transitional are spec terms so they are unlikely to change abruptly. OTOH virtio TC can just decide next version is 2.0 on a drop of a hat.
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect. So all these names are there primarily for libvirt's benefit. And the only input from libvirt devs so far has been that it's unclear how does cross version migration work. That needs to be addressed in some way.
What still needs to be addressed? Just keep the existing device types on migration. We could make additional promises about compatibility with the disable-modern and disable-legacy properties, but I don't see why we need to do it unless we start deprecating the old device types.
So can we maybe start with a libvirt domain xml patch, with an implementation for existing QEMU, get that acked, and then just map it back to qemu command line as directly as possible?
I don't know what you mean here by "libvirt domain XML patch". Do you mean solving the problems only on the libvirt side, keeping the existing device types? Why would we do that? It would be a hack making the situation even messier. If libvirt needs us to provide better interfaces, let's cooperate with them. I'd like us to avoid having yet another "the problem must be solved in the other layer first" deadlock here. -- Eduardo
On Mon, Nov 19, 2018 at 07:47:40PM -0200, Eduardo Habkost wrote:
On Mon, Nov 19, 2018 at 01:07:59PM -0500, Michael S. Tsirkin wrote: n
On Mon, Nov 19, 2018 at 11:41:05AM +0100, Cornelia Huck wrote:
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it.
That's not the claim. In fact the reverse is true - a major revision can come at any point. The claim is that virtio compatibility is not based on version numbers. And another claim is that you can trust the virtio TC not to overload terminology that it put in the spec. So use that and you should be fine. Come up with your own and end up writing another spec just to document it.
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
Yes I think adding a version there is a mistake. transitional/legacy/non-transitional are spec terms so they are unlikely to change abruptly. OTOH virtio TC can just decide next version is 2.0 on a drop of a hat.
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect. So all these names are there primarily for libvirt's benefit. And the only input from libvirt devs so far has been that it's unclear how does cross version migration work. That needs to be addressed in some way.
What still needs to be addressed?
I don't belive you answered Daniel's question.
Just keep the existing device types on migration. We could make additional promises about compatibility with the disable-modern and disable-legacy properties, but I don't see why we need to do it unless we start deprecating the old device types.
Then the answer seems to be in the negative?
So can we maybe start with a libvirt domain xml patch, with an implementation for existing QEMU, get that acked, and then just map it back to qemu command line as directly as possible?
I don't know what you mean here by "libvirt domain XML patch".
Do you mean solving the problems only on the libvirt side, keeping the existing device types? Why would we do that? It would be a hack making the situation even messier.
If libvirt needs us to provide better interfaces, let's cooperate with them. I'd like us to avoid having yet another "the problem must be solved in the other layer first" deadlock here.
I mean IIUC libvirt is the solve user that will benefit from this patch. Let's at least get an ack confirming it does make their lives easier.
-- Eduardo
On Mon, Nov 19, 2018 at 10:08:42PM -0500, Michael S. Tsirkin wrote:
On Mon, Nov 19, 2018 at 07:47:40PM -0200, Eduardo Habkost wrote:
On Mon, Nov 19, 2018 at 01:07:59PM -0500, Michael S. Tsirkin wrote: n
On Mon, Nov 19, 2018 at 11:41:05AM +0100, Cornelia Huck wrote:
On Fri, 16 Nov 2018 01:45:51 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote:
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
One thing that I'm very much not convinced about is the naming, specifically leaving the virtio revision out: I get it that we Should Never Need™ another major version of the spec, but I'm afraid discounting the possibility outright might prove to be shortsighted and come back to bite us later, so I'd much rather keep it.
That's not the claim. In fact the reverse is true - a major revision can come at any point. The claim is that virtio compatibility is not based on version numbers. And another claim is that you can trust the virtio TC not to overload terminology that it put in the spec. So use that and you should be fine. Come up with your own and end up writing another spec just to document it.
And once that's done, "non-transitional" (while matching the language of the spec) starts to look a bit unnecessary when you could simply have
virtio-*-pci virtio-*-pci-1 virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit _and_ shorter. I'll do that in v3.
OTOH, that would mean we'd need to introduce new device types if we ever start to support a virtio 2.x standard. My understanding was that we'll want separate device types for transitional and non-transitional for two reasons: the bus which a device can be plugged into, and changing ids. Do we really expect huge changes in a possible 2.x standard that affect virtio-pci only, and not other virtio transports as well?
Yes I think adding a version there is a mistake. transitional/legacy/non-transitional are spec terms so they are unlikely to change abruptly. OTOH virtio TC can just decide next version is 2.0 on a drop of a hat.
And I strongly believe command line users really really do not want all this mess. Even adding "pci" is the name confuses people (what are the other options?). For command line model=virtio is pretty much perfect. So all these names are there primarily for libvirt's benefit. And the only input from libvirt devs so far has been that it's unclear how does cross version migration work. That needs to be addressed in some way.
What still needs to be addressed?
I don't belive you answered Daniel's question.
Just keep the existing device types on migration. We could make additional promises about compatibility with the disable-modern and disable-legacy properties, but I don't see why we need to do it unless we start deprecating the old device types.
Then the answer seems to be in the negative?
If the question is about the current state, it's "yes". If it's about promises about the future, then we need to understand what kind of promise is expected.
So can we maybe start with a libvirt domain xml patch, with an implementation for existing QEMU, get that acked, and then just map it back to qemu command line as directly as possible?
I don't know what you mean here by "libvirt domain XML patch".
Do you mean solving the problems only on the libvirt side, keeping the existing device types? Why would we do that? It would be a hack making the situation even messier.
If libvirt needs us to provide better interfaces, let's cooperate with them. I'd like us to avoid having yet another "the problem must be solved in the other layer first" deadlock here.
I mean IIUC libvirt is the solve user that will benefit from this patch. Let's at least get an ack confirming it does make their lives easier.
I understand this as an ACK; https://www.mail-archive.com/qemu-devel@nongnu.org/msg567161.html Quoted below: | 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. Re: making their lives easier: easier than what? -- Eduardo
participants (6)
-
Andrea Bolognani -
Cornelia Huck -
Daniel P. Berrangé -
Eduardo Habkost -
Michael S. Tsirkin -
no-reply＠patchew.org