Some features are not always available with vhost. Legacy features are
not available with vp_vdpa in particular. virtio devices used to disable
them when not available even if the corresponding properties were
explicitly set to "on".
QEMU already has OnOffAuto type, which includes the "auto" value to let
it automatically decide the effective value. Convert feature properties
to OnOffAuto and set them "auto" by default to utilize it. This allows
QEMU to report an error if they are set "on" and the corresponding
features are not available.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/hw/virtio/virtio.h | 38 +++++++++++++++++++++-----------------
hw/core/machine.c | 4 +++-
hw/virtio/virtio-bus.c | 14 ++++++++++++--
hw/virtio/virtio.c | 4 +++-
4 files changed, 39 insertions(+), 21 deletions(-)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 638691028050..b854c2cb1d04 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -113,7 +113,8 @@ struct VirtIODevice
uint16_t queue_sel;
/**
* These fields represent a set of VirtIO features at various
- * levels of the stack. @host_features indicates the complete
+ * levels of the stack. @requested_features indicates the feature
+ * set the user requested. @host_features indicates the complete
* feature set the VirtIO device can offer to the driver.
* @guest_features indicates which features the VirtIO driver has
* selected by writing to the feature register. Finally
@@ -121,6 +122,7 @@ struct VirtIODevice
* backend (e.g. vhost) and could potentially be a subset of the
* total feature set offered by QEMU.
*/
+ OnOffAutoBit64 requested_features;
uint64_t host_features;
uint64_t guest_features;
uint64_t backend_features;
@@ -149,6 +151,7 @@ struct VirtIODevice
bool started;
bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
bool disable_legacy_check;
+ bool force_features_auto;
bool vhost_started;
VMChangeStateEntry *vmstate;
char *bus_name;
@@ -376,22 +379,23 @@ typedef struct VirtIOSCSIConf VirtIOSCSIConf;
typedef struct VirtIORNGConf VirtIORNGConf;
#define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
- DEFINE_PROP_BIT64("indirect_desc", _state, _field, \
- VIRTIO_RING_F_INDIRECT_DESC, true), \
- DEFINE_PROP_BIT64("event_idx", _state, _field, \
- VIRTIO_RING_F_EVENT_IDX, true), \
- DEFINE_PROP_BIT64("notify_on_empty", _state, _field, \
- VIRTIO_F_NOTIFY_ON_EMPTY, true), \
- DEFINE_PROP_BIT64("any_layout", _state, _field, \
- VIRTIO_F_ANY_LAYOUT, true), \
- DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
- VIRTIO_F_IOMMU_PLATFORM, false), \
- DEFINE_PROP_BIT64("packed", _state, _field, \
- VIRTIO_F_RING_PACKED, false), \
- DEFINE_PROP_BIT64("queue_reset", _state, _field, \
- VIRTIO_F_RING_RESET, true), \
- DEFINE_PROP_BIT64("in_order", _state, _field, \
- VIRTIO_F_IN_ORDER, false)
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("indirect_desc", _state, _field, \
+ VIRTIO_RING_F_INDIRECT_DESC, \
+ ON_OFF_AUTO_AUTO), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("event_idx", _state, _field, \
+ VIRTIO_RING_F_EVENT_IDX, ON_OFF_AUTO_AUTO), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("notify_on_empty", _state, _field, \
+ VIRTIO_F_NOTIFY_ON_EMPTY, ON_OFF_AUTO_AUTO), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("any_layout", _state, _field, \
+ VIRTIO_F_ANY_LAYOUT, ON_OFF_AUTO_AUTO), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("iommu_platform", _state, _field, \
+ VIRTIO_F_IOMMU_PLATFORM, ON_OFF_AUTO_OFF), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("packed", _state, _field, \
+ VIRTIO_F_RING_PACKED, ON_OFF_AUTO_OFF), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("queue_reset", _state, _field, \
+ VIRTIO_F_RING_RESET, ON_OFF_AUTO_AUTO), \
+ DEFINE_PROP_ON_OFF_AUTO_BIT64("in_order", _state, _field, \
+ VIRTIO_F_IN_ORDER, ON_OFF_AUTO_OFF)
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c949af97668d..bff26b95dd74 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -36,7 +36,9 @@
#include "hw/virtio/virtio-iommu.h"
#include "audio/audio.h"
-GlobalProperty hw_compat_9_2[] = {};
+GlobalProperty hw_compat_9_2[] = {
+ { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
+};
const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
GlobalProperty hw_compat_9_1[] = {
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 896feb37a1ca..75d433b252d5 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -50,6 +50,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
bool vdev_has_iommu;
Error *local_err = NULL;
+ uint64_t features;
DPRINTF("%s: plug device.\n", qbus->name);
@@ -63,13 +64,22 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
/* Get the features of the plugged device. */
assert(vdc->get_features != NULL);
- vdev->host_features = vdc->get_features(vdev, vdev->host_features,
- &local_err);
+ features = vdev->host_features | vdev->requested_features.auto_bits |
+ vdev->requested_features.on_bits;
+ features = vdc->get_features(vdev, features, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
+ if (!vdev->force_features_auto &&
+ (features & vdev->requested_features.on_bits) != vdev->requested_features.on_bits) {
+ error_setg(errp, "A requested feature is not supported by the device");
+ return;
+ }
+
+ vdev->host_features = features;
+
if (klass->device_plugged != NULL) {
klass->device_plugged(qbus->parent, &local_err);
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 85110bce3744..83f803fc703d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4013,11 +4013,13 @@ static void virtio_device_instance_finalize(Object *obj)
}
static const Property virtio_properties[] = {
- DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
+ DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, requested_features),
DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
disable_legacy_check, false),
+ DEFINE_PROP_BOOL("x-force-features-auto", VirtIODevice,
+ force_features_auto, false),
};
static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
--
2.47.1Hi Akihiko
I hit qemu core dump when I tested this series of patches with
virtio-net regression tests, and based on the test result this bug can
be reproduced by just booting a guest. For the details of core dump
info please review the attachment.
Regards
Lei
On Sat, Jan 4, 2025 at 3:37 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Some features are not always available with vhost. Legacy features are
> not available with vp_vdpa in particular. virtio devices used to disable
> them when not available even if the corresponding properties were
> explicitly set to "on".
>
> QEMU already has OnOffAuto type, which includes the "auto" value to let
> it automatically decide the effective value. Convert feature properties
> to OnOffAuto and set them "auto" by default to utilize it. This allows
> QEMU to report an error if they are set "on" and the corresponding
> features are not available.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> include/hw/virtio/virtio.h | 38 +++++++++++++++++++++-----------------
> hw/core/machine.c | 4 +++-
> hw/virtio/virtio-bus.c | 14 ++++++++++++--
> hw/virtio/virtio.c | 4 +++-
> 4 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 638691028050..b854c2cb1d04 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -113,7 +113,8 @@ struct VirtIODevice
> uint16_t queue_sel;
> /**
> * These fields represent a set of VirtIO features at various
> - * levels of the stack. @host_features indicates the complete
> + * levels of the stack. @requested_features indicates the feature
> + * set the user requested. @host_features indicates the complete
> * feature set the VirtIO device can offer to the driver.
> * @guest_features indicates which features the VirtIO driver has
> * selected by writing to the feature register. Finally
> @@ -121,6 +122,7 @@ struct VirtIODevice
> * backend (e.g. vhost) and could potentially be a subset of the
> * total feature set offered by QEMU.
> */
> + OnOffAutoBit64 requested_features;
> uint64_t host_features;
> uint64_t guest_features;
> uint64_t backend_features;
> @@ -149,6 +151,7 @@ struct VirtIODevice
> bool started;
> bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
> bool disable_legacy_check;
> + bool force_features_auto;
> bool vhost_started;
> VMChangeStateEntry *vmstate;
> char *bus_name;
> @@ -376,22 +379,23 @@ typedef struct VirtIOSCSIConf VirtIOSCSIConf;
> typedef struct VirtIORNGConf VirtIORNGConf;
>
> #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
> - DEFINE_PROP_BIT64("indirect_desc", _state, _field, \
> - VIRTIO_RING_F_INDIRECT_DESC, true), \
> - DEFINE_PROP_BIT64("event_idx", _state, _field, \
> - VIRTIO_RING_F_EVENT_IDX, true), \
> - DEFINE_PROP_BIT64("notify_on_empty", _state, _field, \
> - VIRTIO_F_NOTIFY_ON_EMPTY, true), \
> - DEFINE_PROP_BIT64("any_layout", _state, _field, \
> - VIRTIO_F_ANY_LAYOUT, true), \
> - DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> - VIRTIO_F_IOMMU_PLATFORM, false), \
> - DEFINE_PROP_BIT64("packed", _state, _field, \
> - VIRTIO_F_RING_PACKED, false), \
> - DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> - VIRTIO_F_RING_RESET, true), \
> - DEFINE_PROP_BIT64("in_order", _state, _field, \
> - VIRTIO_F_IN_ORDER, false)
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("indirect_desc", _state, _field, \
> + VIRTIO_RING_F_INDIRECT_DESC, \
> + ON_OFF_AUTO_AUTO), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("event_idx", _state, _field, \
> + VIRTIO_RING_F_EVENT_IDX, ON_OFF_AUTO_AUTO), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("notify_on_empty", _state, _field, \
> + VIRTIO_F_NOTIFY_ON_EMPTY, ON_OFF_AUTO_AUTO), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("any_layout", _state, _field, \
> + VIRTIO_F_ANY_LAYOUT, ON_OFF_AUTO_AUTO), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("iommu_platform", _state, _field, \
> + VIRTIO_F_IOMMU_PLATFORM, ON_OFF_AUTO_OFF), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("packed", _state, _field, \
> + VIRTIO_F_RING_PACKED, ON_OFF_AUTO_OFF), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("queue_reset", _state, _field, \
> + VIRTIO_F_RING_RESET, ON_OFF_AUTO_AUTO), \
> + DEFINE_PROP_ON_OFF_AUTO_BIT64("in_order", _state, _field, \
> + VIRTIO_F_IN_ORDER, ON_OFF_AUTO_OFF)
>
> hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c949af97668d..bff26b95dd74 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -36,7 +36,9 @@
> #include "hw/virtio/virtio-iommu.h"
> #include "audio/audio.h"
>
> -GlobalProperty hw_compat_9_2[] = {};
> +GlobalProperty hw_compat_9_2[] = {
> + { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
> +};
> const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
>
> GlobalProperty hw_compat_9_1[] = {
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 896feb37a1ca..75d433b252d5 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -50,6 +50,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> bool vdev_has_iommu;
> Error *local_err = NULL;
> + uint64_t features;
>
> DPRINTF("%s: plug device.\n", qbus->name);
>
> @@ -63,13 +64,22 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>
> /* Get the features of the plugged device. */
> assert(vdc->get_features != NULL);
> - vdev->host_features = vdc->get_features(vdev, vdev->host_features,
> - &local_err);
> + features = vdev->host_features | vdev->requested_features.auto_bits |
> + vdev->requested_features.on_bits;
> + features = vdc->get_features(vdev, features, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> + if (!vdev->force_features_auto &&
> + (features & vdev->requested_features.on_bits) != vdev->requested_features.on_bits) {
> + error_setg(errp, "A requested feature is not supported by the device");
> + return;
> + }
> +
> + vdev->host_features = features;
> +
> if (klass->device_plugged != NULL) {
> klass->device_plugged(qbus->parent, &local_err);
> }
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 85110bce3744..83f803fc703d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -4013,11 +4013,13 @@ static void virtio_device_instance_finalize(Object *obj)
> }
>
> static const Property virtio_properties[] = {
> - DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
> + DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, requested_features),
> DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
> DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
> DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
> disable_legacy_check, false),
> + DEFINE_PROP_BOOL("x-force-features-auto", VirtIODevice,
> + force_features_auto, false),
> };
>
> static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
>
> --
> 2.47.1
>
>
(gdb) bt full
#0 0x00007f1b5082c7ac in __pthread_kill_implementation () at /lib64/libc.so.6
#1 0x00007f1b507d6a36 in raise () at /lib64/libc.so.6
#2 0x00007f1b507be8fa in abort () at /lib64/libc.so.6
#3 0x00007f1b507be81e in __assert_fail_base.cold () at /lib64/libc.so.6
#4 0x00007f1b507cef76 in __assert_fail () at /lib64/libc.so.6
#5 0x000055a648eb5b42 in qobject_input_try_get_object (qiv=0x55a659f5e050, name=0x55a659f5df90 "write-cache", consume=true) at ../qapi/qobject-input-visitor.c:143
removed = false
tos = 0x55a659f5e150
qobj = 0x55a659f5cef0
ret = 0x55a6588d4920
__PRETTY_FUNCTION__ = "qobject_input_try_get_object"
#6 0x000055a648eb5c48 in qobject_input_get_object (qiv=0x55a659f5e050, name=0x55a659f5df90 "write-cache", consume=true, errp=0x7ffe9c392230) at ../qapi/qobject-input-visitor.c:168
obj = 0x55a659f5e050
__func__ = "qobject_input_get_object"
#7 0x000055a648eb6e09 in qobject_input_type_str (v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", obj=0x7ffe9c391f38, errp=0x7ffe9c392230) at ../qapi/qobject-input-visitor.c:528
qiv = 0x55a659f5e050
qobj = 0x7ffe9c391f38
qstr = 0x55a659f5df90
__func__ = "qobject_input_type_str"
#8 0x000055a648eb4f70 in visit_type_str (v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", obj=0x7ffe9c391f38, errp=0x7ffe9c392230) at ../qapi/qapi-visit-core.c:349
ok = false
__PRETTY_FUNCTION__ = "visit_type_str"
#9 0x000055a648eb52c0 in input_type_enum (v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", obj=0x55a659f5cd14, lookup=0x55a649ed0690 <OnOffAuto_lookup>, errp=0x7ffe9c392230) at ../qapi/qapi-visit-core.c:401
value = 0
enum_str = 0x0
__func__ = "input_type_enum"
#10 0x000055a648eb54a9 in visit_type_enum (v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", obj=0x55a659f5cd14, lookup=0x55a649ed0690 <OnOffAuto_lookup>, errp=0x7ffe9c392230) at ../qapi/qapi-visit-core.c:431
__PRETTY_FUNCTION__ = "visit_type_enum"
#11 0x000055a648c55f87 in qdev_propinfo_set_enum (obj=0x55a659f5cc30, v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", opaque=0x55a649f01f38 <scsi_hd_properties+504>, errp=0x7ffe9c392230) at ../hw/core/qdev-properties.c:115
prop = 0x55a649f01f38 <scsi_hd_properties+504>
ptr = 0x55a659f5cd14
#12 0x000055a648c56fe7 in set_on_off_auto (obj=0x55a659f5cc30, v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", opaque=0x55a649f01f38 <scsi_hd_properties+504>, errp=0x7ffe9c392230) at ../hw/core/qdev-properties.c:570
prop = 0x55a649f01f38 <scsi_hd_properties+504>
ptr = 0x55a659f5cd14
value = false
#13 0x000055a648c55e77 in field_prop_set (obj=0x55a659f5cc30, v=0x55a659f5e050, name=0x55a659f5df90 "write-cache", opaque=0x55a649f01f38 <scsi_hd_properties+504>, errp=0x7ffe9c392230) at ../hw/core/qdev-properties.c:87
prop = 0x55a649f01f38 <scsi_hd_properties+504>
#14 0x000055a648c62f09 in object_property_set (obj=0x55a659f5cc30, name=0x55a659f5df90 "write-cache", v=0x55a659f5e050, errp=0x7ffe9c392230) at ../qom/object.c:1450
_auto_errp_prop = {local_err = 0x0, errp = 0x7ffe9c392230}
prop = 0x55a6589fe240
__func__ = "object_property_set"
#15 0x000055a648c66ce7 in object_set_properties_from_qdict (obj=0x55a659f5cc30, qdict=0x55a659f5cef0, v=0x55a659f5e050, errp=0x7ffe9c392230) at ../qom/object_interfaces.c:55
e = 0x55a659f5df60
#16 0x000055a648c66de2 in object_set_properties_from_keyval (obj=0x55a659f5cc30, qdict=0x55a659f5cef0, from_json=true, errp=0x7ffe9c392230) at ../qom/object_interfaces.c:73
v = 0x55a659f5e050
#17 0x000055a64892c996 in qdev_device_add_from_qdict (opts=0x55a6588d8cd0, from_json=true, errp=0x7ffe9c392230) at ../system/qdev-monitor.c:721
_auto_errp_prop = {local_err = 0x0, errp = 0x55a64a0667c0 <error_fatal>}
dc = 0x55a6589fd360
driver = 0x55a6588d89d0 "scsi-hd"
path = 0x0
--Type <RET> for more, q to quit, c to continue without paging--
id = 0x55a659f26910 "image1"
dev = 0x55a659f5cc30
bus = 0x55a659d8ea20
properties = 0x55a659f5cef0
__func__ = "qdev_device_add_from_qdict"
#18 0x000055a64892d182 in qmp_device_add (qdict=0x55a6588d8cd0, ret_data=0x7ffe9c3922b8, errp=0x55a64a0667c0 <error_fatal>) at ../system/qdev-monitor.c:862
dev = 0x0
#19 0x000055a64893b70c in qemu_create_cli_devices () at ../system/vl.c:2685
ret_data = 0x0
opt = 0x55a6588d7590
__PRETTY_FUNCTION__ = "qemu_create_cli_devices"
#20 0x000055a64893b94b in qmp_x_exit_preconfig (errp=0x55a64a0667c0 <error_fatal>) at ../system/vl.c:2742
__func__ = "qmp_x_exit_preconfig"
#21 0x000055a64893e46b in qemu_init (argc=101, argv=0x7ffe9c392628) at ../system/vl.c:3779
opts = 0x55a6588dced0
icount_opts = 0x0
accel_opts = 0x0
olist = 0x55a649f3c3e0 <qemu_action_opts>
optind = 101
optarg = 0x7ffe9c394297 "{\"id\": \"pcie_extra_root_port_0\", \"driver\": \"pcie-root-port\", \"multifunction\": true, \"bus\": \"pcie.0\", \"addr\": \"0x3\", \"chassis\": 5}"
machine_class = 0x55a6589de620
userconfig = true
vmstate_dump_file = 0x0
__PRETTY_FUNCTION__ = "qemu_init"
#22 0x000055a648df7c26 in main (argc=101, argv=0x7ffe9c392628) at ../system/main.c:68
__func__ = "main"
© 2016 - 2025 Red Hat, Inc.