* [PATCH v5 1/4] qapi: Do not consume a value if failed
2025-02-08 7:51 [PATCH v5 0/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
@ 2025-02-08 7:51 ` Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 2/4] qapi: Accept bool for OnOffAuto and OnOffSplit Akihiko Odaki
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2025-02-08 7:51 UTC (permalink / raw)
To: Jason Wang, Dmitry Fleytman, Sriram Yagnaraman,
Michael S. Tsirkin, Luigi Rizzo, Giuseppe Lettieri,
Vincenzo Maffione, Andrew Melnychenko, Yuri Benditovich,
Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost,
Markus Armbruster, Michael Roth, Marcel Apfelbaum,
Philippe Mathieu-Daudé, Yanan Wang, Zhao Liu, Lei Yang,
BALATON Zoltan
Cc: qemu-devel, Akihiko Odaki
Do not consume a value if interpreting one failed so that we can
reinterpret the value with a different type.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
1 file changed, 69 insertions(+), 34 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f110a804b2ae0f3f75122775ddbc5ec7cc5de230..799c1c9bd6bde0676d6b028b485de13cb4884395 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -116,9 +116,8 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
return full_name_nth(qiv, name, 0);
}
-static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
- const char *name,
- bool consume)
+static QObject *qobject_input_try_get_object(const QObjectInputVisitor *qiv,
+ const char *name)
{
StackObject *tos;
QObject *qobj;
@@ -138,34 +137,19 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
if (qobject_type(qobj) == QTYPE_QDICT) {
assert(name);
ret = qdict_get(qobject_to(QDict, qobj), name);
- if (tos->h && consume && ret) {
- bool removed = g_hash_table_remove(tos->h, name);
- assert(removed);
- }
} else {
assert(qobject_type(qobj) == QTYPE_QLIST);
assert(!name);
- if (tos->entry) {
- ret = qlist_entry_obj(tos->entry);
- if (consume) {
- tos->entry = qlist_next(tos->entry);
- }
- } else {
- ret = NULL;
- }
- if (consume) {
- tos->index++;
- }
+ ret = tos->entry ? qlist_entry_obj(tos->entry) : NULL;
}
return ret;
}
static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
- const char *name,
- bool consume, Error **errp)
+ const char *name, Error **errp)
{
- QObject *obj = qobject_input_try_get_object(qiv, name, consume);
+ QObject *obj = qobject_input_try_get_object(qiv, name);
if (!obj) {
error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
@@ -173,6 +157,38 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
return obj;
}
+static void qobject_input_consume_object(QObjectInputVisitor *qiv,
+ const char *name)
+{
+ StackObject *tos;
+ QObject *qobj;
+
+ if (QSLIST_EMPTY(&qiv->stack)) {
+ /* Starting at root, name is ignored. */
+ return;
+ }
+
+ /* We are in a container; find the next element. */
+ tos = QSLIST_FIRST(&qiv->stack);
+ qobj = tos->obj;
+ assert(qobj);
+
+ if (qobject_type(qobj) == QTYPE_QDICT) {
+ assert(name);
+ if (tos->h) {
+ bool removed = g_hash_table_remove(tos->h, name);
+ assert(removed);
+ }
+ } else {
+ assert(qobject_type(qobj) == QTYPE_QLIST);
+ assert(!name);
+ if (tos->entry) {
+ tos->entry = qlist_next(tos->entry);
+ }
+ tos->index++;
+ }
+}
+
static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
const char *name,
Error **errp)
@@ -180,7 +196,7 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
QObject *qobj;
QString *qstr;
- qobj = qobject_input_get_object(qiv, name, true, errp);
+ qobj = qobject_input_get_object(qiv, name, errp);
if (!qobj) {
return NULL;
}
@@ -233,6 +249,7 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
tos->index = -1;
}
+ qobject_input_consume_object(qiv, name);
QSLIST_INSERT_HEAD(&qiv->stack, tos, node);
return tos->entry;
}
@@ -279,7 +296,7 @@ static bool qobject_input_start_struct(Visitor *v, const char *name, void **obj,
size_t size, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
if (obj) {
*obj = NULL;
@@ -316,7 +333,7 @@ static bool qobject_input_start_list(Visitor *v, const char *name,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
const QListEntry *entry;
if (list) {
@@ -382,7 +399,7 @@ static bool qobject_input_start_alternate(Visitor *v, const char *name,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, false, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
if (!qobj) {
*obj = NULL;
@@ -397,7 +414,7 @@ static bool qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
QNum *qnum;
if (!qobj) {
@@ -409,6 +426,7 @@ static bool qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj,
full_name(qiv, name));
return false;
}
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -428,6 +446,7 @@ static bool qobject_input_type_int64_keyval(Visitor *v, const char *name,
full_name(qiv, name), "integer");
return false;
}
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -435,7 +454,7 @@ static bool qobject_input_type_uint64(Visitor *v, const char *name,
uint64_t *obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
QNum *qnum;
int64_t val;
@@ -448,12 +467,14 @@ static bool qobject_input_type_uint64(Visitor *v, const char *name,
}
if (qnum_get_try_uint(qnum, obj)) {
+ qobject_input_consume_object(qiv, name);
return true;
}
/* Need to accept negative values for backward compatibility */
if (qnum_get_try_int(qnum, &val)) {
*obj = val;
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -479,6 +500,7 @@ static bool qobject_input_type_uint64_keyval(Visitor *v, const char *name,
full_name(qiv, name), "integer");
return false;
}
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -486,7 +508,7 @@ static bool qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
QBool *qbool;
if (!qobj) {
@@ -500,6 +522,7 @@ static bool qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
}
*obj = qbool_get_bool(qbool);
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -518,6 +541,7 @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name,
full_name(qiv, name), "'on' or 'off'");
return false;
}
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -525,7 +549,7 @@ static bool qobject_input_type_str(Visitor *v, const char *name, char **obj,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
QString *qstr;
*obj = NULL;
@@ -540,6 +564,7 @@ static bool qobject_input_type_str(Visitor *v, const char *name, char **obj,
}
*obj = g_strdup(qstring_get_str(qstr));
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -549,15 +574,20 @@ static bool qobject_input_type_str_keyval(Visitor *v, const char *name,
QObjectInputVisitor *qiv = to_qiv(v);
const char *str = qobject_input_get_keyval(qiv, name, errp);
+ if (!str) {
+ return false;
+ }
+
*obj = g_strdup(str);
- return !!str;
+ qobject_input_consume_object(qiv, name);
+ return true;
}
static bool qobject_input_type_number(Visitor *v, const char *name, double *obj,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
QNum *qnum;
if (!qobj) {
@@ -571,6 +601,7 @@ static bool qobject_input_type_number(Visitor *v, const char *name, double *obj,
}
*obj = qnum_get_double(qnum);
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -593,6 +624,7 @@ static bool qobject_input_type_number_keyval(Visitor *v, const char *name,
}
*obj = val;
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -600,7 +632,7 @@ static bool qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
*obj = NULL;
if (!qobj) {
@@ -608,6 +640,7 @@ static bool qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
}
*obj = qobject_ref(qobj);
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -615,7 +648,7 @@ static bool qobject_input_type_null(Visitor *v, const char *name,
QNull **obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, errp);
*obj = NULL;
if (!qobj) {
@@ -628,6 +661,7 @@ static bool qobject_input_type_null(Visitor *v, const char *name,
return false;
}
*obj = qnull();
+ qobject_input_consume_object(qiv, name);
return true;
}
@@ -647,13 +681,14 @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name,
full_name(qiv, name), "size");
return false;
}
+ qobject_input_consume_object(qiv, name);
return true;
}
static void qobject_input_optional(Visitor *v, const char *name, bool *present)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_try_get_object(qiv, name, false);
+ QObject *qobj = qobject_input_try_get_object(qiv, name);
if (!qobj) {
*present = false;
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v5 2/4] qapi: Accept bool for OnOffAuto and OnOffSplit
2025-02-08 7:51 [PATCH v5 0/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 1/4] qapi: Do not consume a value if failed Akihiko Odaki
@ 2025-02-08 7:51 ` Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 3/4] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64() Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 4/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
3 siblings, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2025-02-08 7:51 UTC (permalink / raw)
To: Jason Wang, Dmitry Fleytman, Sriram Yagnaraman,
Michael S. Tsirkin, Luigi Rizzo, Giuseppe Lettieri,
Vincenzo Maffione, Andrew Melnychenko, Yuri Benditovich,
Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost,
Markus Armbruster, Michael Roth, Marcel Apfelbaum,
Philippe Mathieu-Daudé, Yanan Wang, Zhao Liu, Lei Yang,
BALATON Zoltan
Cc: qemu-devel, Akihiko Odaki
bool has representations of "on" and "off" different from
OnOffAuto/OnOffSplit:
- The command line syntax accepts on/yes/true/y and off/no/false/n for
bool but only on and off for OnOffAuto.
- JSON uses true/false for bool but "on" and "off" for
OnOffAuto/OnOffSplit.
This inconsistency causes some problems:
- Users need to take the underlying type into consideration to determine
what literal to specify, increasing cognitive loads for human users
and complexity for programs invoking QEMU.
- Converting an existing bool property to OnOffAuto/OnOffSplit will
break compatibility.
Fix these problems by accepting bool literals for OnOffAuto/OnOffSplit.
This change is specific to OnOffAuto/OnOffSplit; types added in the
future may be defined as an alternate of bool and enum to avoid the
mentioned problems in the first place.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
hw/core/qdev-properties.c | 17 ++++++++++++++++-
scripts/qapi/visit.py | 24 ++++++++++++++++++++++++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 434a76f5036edd2091a9c79525b8e102582637be..073902431213c5be47197cb0d993d60cc2562501 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -2,6 +2,7 @@
#include "hw/qdev-properties.h"
#include "qapi/error.h"
#include "qapi/qapi-types-misc.h"
+#include "qapi/qapi-visit-common.h"
#include "qapi/qmp/qlist.h"
#include "qemu/ctype.h"
#include "qemu/error-report.h"
@@ -493,12 +494,26 @@ const PropertyInfo qdev_prop_string = {
/* --- on/off/auto --- */
+static void set_on_off_auto(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ Property *prop = opaque;
+ int *ptr = object_field_prop_ptr(obj, prop);
+ OnOffAuto value;
+
+ if (!visit_type_OnOffAuto(v, name, &value, errp)) {
+ return;
+ }
+
+ *ptr = value;
+}
+
const PropertyInfo qdev_prop_on_off_auto = {
.name = "OnOffAuto",
.description = "on/off/auto",
.enum_table = &OnOffAuto_lookup,
.get = qdev_propinfo_get_enum,
- .set = qdev_propinfo_set_enum,
+ .set = set_on_off_auto,
.set_default_value = qdev_propinfo_set_default_value_enum,
};
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 12f92e429f6bafc091f74af88c1b837d08c7f733..221373b165aa95bceb4eb50a557edf0e5b4c01f7 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -209,6 +209,29 @@ def gen_visit_list(name: str, element_type: QAPISchemaType) -> str:
def gen_visit_enum(name: str) -> str:
+ if name in ('OnOffAuto', 'OnOffSplit'):
+ return mcgen('''
+
+bool visit_type_%(c_name)s(Visitor *v, const char *name,
+ %(c_name)s *obj, Error **errp)
+{
+ bool b;
+ int i;
+
+ if (v->type == VISITOR_INPUT && visit_type_bool(v, name, &b, NULL)) {
+ *obj = b ? %(on)s : %(off)s;
+ return true;
+ }
+
+ b = visit_type_enum(v, name, &i, &%(c_name)s_lookup, errp);
+ *obj = i;
+
+ return b;
+}
+''',
+ c_name=c_name(name),
+ on=c_enum_const(name, 'on'), off=c_enum_const(name, 'off'))
+
return mcgen('''
bool visit_type_%(c_name)s(Visitor *v, const char *name,
@@ -359,6 +382,7 @@ def _begin_user_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/visitor-impl.h"
#include "%(visit)s.h"
''',
visit=visit))
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v5 3/4] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
2025-02-08 7:51 [PATCH v5 0/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 1/4] qapi: Do not consume a value if failed Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 2/4] qapi: Accept bool for OnOffAuto and OnOffSplit Akihiko Odaki
@ 2025-02-08 7:51 ` Akihiko Odaki
2025-02-08 7:51 ` [PATCH v5 4/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
3 siblings, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2025-02-08 7:51 UTC (permalink / raw)
To: Jason Wang, Dmitry Fleytman, Sriram Yagnaraman,
Michael S. Tsirkin, Luigi Rizzo, Giuseppe Lettieri,
Vincenzo Maffione, Andrew Melnychenko, Yuri Benditovich,
Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost,
Markus Armbruster, Michael Roth, Marcel Apfelbaum,
Philippe Mathieu-Daudé, Yanan Wang, Zhao Liu, Lei Yang,
BALATON Zoltan
Cc: qemu-devel, Akihiko Odaki
DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO()
as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference
is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of
bool.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/hw/qdev-properties.h | 18 ++++++++++++
hw/core/qdev-properties.c | 66 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 83 insertions(+), 1 deletion(-)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index bf27375a3ccdb238ef3327dd85d3d0a1431cbfbf..0d161325e8dc92d0e0e5aa9a1e2dd734f7a55cae 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -43,11 +43,22 @@ struct PropertyInfo {
ObjectPropertyRelease *release;
};
+/**
+ * struct OnOffAutoBit64 - OnOffAuto storage with 64 elements.
+ * @on_bits: Bitmap of elements with "on".
+ * @auto_bits: Bitmap of elements with "auto".
+ */
+typedef struct OnOffAutoBit64 {
+ uint64_t on_bits;
+ uint64_t auto_bits;
+} OnOffAutoBit64;
+
/*** qdev-properties.c ***/
extern const PropertyInfo qdev_prop_bit;
extern const PropertyInfo qdev_prop_bit64;
+extern const PropertyInfo qdev_prop_on_off_auto_bit64;
extern const PropertyInfo qdev_prop_bool;
extern const PropertyInfo qdev_prop_enum;
extern const PropertyInfo qdev_prop_uint8;
@@ -100,6 +111,13 @@ extern const PropertyInfo qdev_prop_link;
.set_default = true, \
.defval.u = (bool)_defval)
+#define DEFINE_PROP_ON_OFF_AUTO_BIT64(_name, _state, _field, _bit, _defval) \
+ DEFINE_PROP(_name, _state, _field, qdev_prop_on_off_auto_bit64, \
+ OnOffAutoBit64, \
+ .bitnr = (_bit), \
+ .set_default = true, \
+ .defval.i = (OnOffAuto)_defval)
+
#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) \
DEFINE_PROP(_name, _state, _field, qdev_prop_bool, bool, \
.set_default = true, \
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 073902431213c5be47197cb0d993d60cc2562501..cfab7b97091ad704b7f43d6ba6fcd8937ca5dfe3 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -188,7 +188,8 @@ const PropertyInfo qdev_prop_bit = {
static uint64_t qdev_get_prop_mask64(const Property *prop)
{
- assert(prop->info == &qdev_prop_bit64);
+ assert(prop->info == &qdev_prop_bit64 ||
+ prop->info == &qdev_prop_on_off_auto_bit64);
return 0x1ull << prop->bitnr;
}
@@ -233,6 +234,69 @@ const PropertyInfo qdev_prop_bit64 = {
.set_default_value = set_default_value_bool,
};
+static void prop_get_on_off_auto_bit64(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ Property *prop = opaque;
+ OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop);
+ OnOffAuto value;
+ uint64_t mask = qdev_get_prop_mask64(prop);
+
+ if (p->auto_bits & mask) {
+ value = ON_OFF_AUTO_AUTO;
+ } else if (p->on_bits & mask) {
+ value = ON_OFF_AUTO_ON;
+ } else {
+ value = ON_OFF_AUTO_OFF;
+ }
+
+ visit_type_OnOffAuto(v, name, &value, errp);
+}
+
+static void prop_set_on_off_auto_bit64(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ Property *prop = opaque;
+ OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop);
+ OnOffAuto value;
+ uint64_t mask = qdev_get_prop_mask64(prop);
+
+ if (!visit_type_OnOffAuto(v, name, &value, errp)) {
+ return;
+ }
+
+ switch (value) {
+ case ON_OFF_AUTO_AUTO:
+ p->on_bits &= ~mask;
+ p->auto_bits |= mask;
+ break;
+
+ case ON_OFF_AUTO_ON:
+ p->on_bits |= mask;
+ p->auto_bits &= ~mask;
+ break;
+
+ case ON_OFF_AUTO_OFF:
+ p->on_bits &= ~mask;
+ p->auto_bits &= ~mask;
+ break;
+
+ case ON_OFF_AUTO__MAX:
+ g_assert_not_reached();
+ }
+}
+
+const PropertyInfo qdev_prop_on_off_auto_bit64 = {
+ .name = "OnOffAuto",
+ .description = "on/off/auto",
+ .enum_table = &OnOffAuto_lookup,
+ .get = prop_get_on_off_auto_bit64,
+ .set = prop_set_on_off_auto_bit64,
+ .set_default_value = qdev_propinfo_set_default_value_enum,
+};
+
/* --- bool --- */
static void get_bool(Object *obj, Visitor *v, const char *name, void *opaque,
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v5 4/4] virtio: Convert feature properties to OnOffAuto
2025-02-08 7:51 [PATCH v5 0/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
` (2 preceding siblings ...)
2025-02-08 7:51 ` [PATCH v5 3/4] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64() Akihiko Odaki
@ 2025-02-08 7:51 ` Akihiko Odaki
2025-02-20 15:46 ` Michael S. Tsirkin
3 siblings, 1 reply; 7+ messages in thread
From: Akihiko Odaki @ 2025-02-08 7:51 UTC (permalink / raw)
To: Jason Wang, Dmitry Fleytman, Sriram Yagnaraman,
Michael S. Tsirkin, Luigi Rizzo, Giuseppe Lettieri,
Vincenzo Maffione, Andrew Melnychenko, Yuri Benditovich,
Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost,
Markus Armbruster, Michael Roth, Marcel Apfelbaum,
Philippe Mathieu-Daudé, Yanan Wang, Zhao Liu, Lei Yang,
BALATON Zoltan
Cc: qemu-devel, Akihiko Odaki
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 | 1 +
hw/virtio/virtio-bus.c | 14 ++++++++++++--
hw/virtio/virtio.c | 4 +++-
4 files changed, 37 insertions(+), 20 deletions(-)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 638691028050d2599592d8c7e95c75ac3913fbdd..b854c2cb1d04da0a35165289c28f87e8cb869df6 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 c23b39949649054ac59d2a9b497f34e1b7bd8d6c..0de04baa61735ff02f797f778c626ef690625ce3 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -38,6 +38,7 @@
GlobalProperty hw_compat_9_2[] = {
{"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"},
+ { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
};
const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 896feb37a1caa805543e971c150d3673675b9a6b..75d433b252d5337d91616a2847b3dc12e811c2da 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 85110bce37443bb46c4159761af112d0dba466b4..83f803fc703da6257608e21476305c8e9c6a8b07 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.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v5 4/4] virtio: Convert feature properties to OnOffAuto
2025-02-08 7:51 ` [PATCH v5 4/4] virtio: Convert feature properties to OnOffAuto Akihiko Odaki
@ 2025-02-20 15:46 ` Michael S. Tsirkin
2025-02-27 7:08 ` Akihiko Odaki
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2025-02-20 15:46 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Jason Wang, Dmitry Fleytman, Sriram Yagnaraman, Luigi Rizzo,
Giuseppe Lettieri, Vincenzo Maffione, Andrew Melnychenko,
Yuri Benditovich, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster, Michael Roth,
Marcel Apfelbaum, Philippe Mathieu-Daudé, Yanan Wang,
Zhao Liu, Lei Yang, BALATON Zoltan, qemu-devel
On Sat, Feb 08, 2025 at 04:51:10PM +0900, Akihiko Odaki 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 | 1 +
> hw/virtio/virtio-bus.c | 14 ++++++++++++--
> hw/virtio/virtio.c | 4 +++-
> 4 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 638691028050d2599592d8c7e95c75ac3913fbdd..b854c2cb1d04da0a35165289c28f87e8cb869df6 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 c23b39949649054ac59d2a9b497f34e1b7bd8d6c..0de04baa61735ff02f797f778c626ef690625ce3 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -38,6 +38,7 @@
>
> GlobalProperty hw_compat_9_2[] = {
> {"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"},
> + { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
> };
> const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
>
Confused why this is on.
If set, this breaks existing setups if they run a configuration vhost
does not support, does it not?
And in particular, aren't some of these features exposed through
libvirt?
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 896feb37a1caa805543e971c150d3673675b9a6b..75d433b252d5337d91616a2847b3dc12e811c2da 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 85110bce37443bb46c4159761af112d0dba466b4..83f803fc703da6257608e21476305c8e9c6a8b07 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.48.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v5 4/4] virtio: Convert feature properties to OnOffAuto
2025-02-20 15:46 ` Michael S. Tsirkin
@ 2025-02-27 7:08 ` Akihiko Odaki
0 siblings, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2025-02-27 7:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, Dmitry Fleytman, Sriram Yagnaraman, Luigi Rizzo,
Giuseppe Lettieri, Vincenzo Maffione, Andrew Melnychenko,
Yuri Benditovich, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster, Michael Roth,
Marcel Apfelbaum, Philippe Mathieu-Daudé, Yanan Wang,
Zhao Liu, Lei Yang, BALATON Zoltan, qemu-devel
On 2025/02/21 0:46, Michael S. Tsirkin wrote:
> On Sat, Feb 08, 2025 at 04:51:10PM +0900, Akihiko Odaki 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 | 1 +
>> hw/virtio/virtio-bus.c | 14 ++++++++++++--
>> hw/virtio/virtio.c | 4 +++-
>> 4 files changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 638691028050d2599592d8c7e95c75ac3913fbdd..b854c2cb1d04da0a35165289c28f87e8cb869df6 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 c23b39949649054ac59d2a9b497f34e1b7bd8d6c..0de04baa61735ff02f797f778c626ef690625ce3 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -38,6 +38,7 @@
>>
>> GlobalProperty hw_compat_9_2[] = {
>> {"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"},
>> + { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
>> };
>> const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
>>
>
> Confused why this is on.
> If set, this breaks existing setups if they run a configuration vhost
> does not support, does it not?
> And in particular, aren't some of these features exposed through
> libvirt?
This property enforces the existing behavior, which is to disable
unsupported features even if they are explicitly set to "on". It
introduces no breaking change.
>
>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>> index 896feb37a1caa805543e971c150d3673675b9a6b..75d433b252d5337d91616a2847b3dc12e811c2da 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 85110bce37443bb46c4159761af112d0dba466b4..83f803fc703da6257608e21476305c8e9c6a8b07 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.48.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread