[PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[Akihiko Odaki]

This series was spun off from:
"[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
(https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)

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>
---
Changes in v6:
- Rebased.
- Link to v5: https://lore.kernel.org/qemu-devel/20250208-virtio-v5-0-4376cb218c0f@daynix.com

Changes in v5:
- Covered QAPI more than just qdev.
- Expanded the description of patch
  "qapi: Accept bool for OnOffAuto and OnOffSplit".
- Rebased.
- Link to v4: https://lore.kernel.org/r/20250108-virtio-v4-0-cbf0aa04c9f9@daynix.com

Changes in v4:
- Added patch "qapi: Do not consume a value if failed".
- Link to v3: https://lore.kernel.org/r/20250104-virtio-v3-0-63ef70e9ddf3@daynix.com

Changes in v3:
- Rebased.
- Link to v2: https://lore.kernel.org/r/20241022-virtio-v2-0-b2394236e053@daynix.com

Changes in v2:
- Expanded the message of patch "qdev-properties: Accept bool for
  OnOffAuto".
- Link to v1: https://lore.kernel.org/r/20241014-virtio-v1-0-e9ddf7a81891@daynix.com

---
Akihiko Odaki (4):
      qapi: Do not consume a value if failed
      qapi: Accept bool for OnOffAuto and OnOffSplit
      qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
      virtio: Convert feature properties to OnOffAuto

 include/hw/qdev-properties.h |  18 ++++++++
 include/hw/virtio/virtio.h   |  38 +++++++++-------
 hw/core/machine.c            |   1 +
 hw/core/qdev-properties.c    |  83 +++++++++++++++++++++++++++++++++-
 hw/virtio/virtio-bus.c       |  14 +++++-
 hw/virtio/virtio.c           |   4 +-
 qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
 scripts/qapi/visit.py        |  24 ++++++++++
 8 files changed, 229 insertions(+), 56 deletions(-)
---
base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
change-id: 20241013-virtio-164ea3f295c3

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH v6 1/4] qapi: Do not consume a value if failed

[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 c52d36997df8d99bc575c553a413df8ebcb60c32..0cc75b3c74e40a24cb1ce764330ae2703e80d7cf 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

[PATCH v6 2/4] qapi: Accept bool for OnOffAuto and OnOffSplit

[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 0b52aad55516099917a4e9b71a920bc3da8a92ec..3462921841db74456284b91374955b6101f03bbc 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 "qobject/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 36e240967b687db10115ba375da3371898b7d95c..5844b64ad6b49a7b88e286dd35ab28b2755282d0 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -205,6 +205,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,
@@ -355,6 +378,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"
 #include "%(prefix)sqapi-features.h"
 ''',

-- 
2.48.1

[PATCH v6 3/4] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()

[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 3462921841db74456284b91374955b6101f03bbc..54fbcc9fa8d4b66003d9a66b1e6af61684c59ae4 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

[PATCH v6 4/4] virtio: Convert feature properties to OnOffAuto

[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 02cff735b3fb8cf0ce8a85ec7f1fb36a98005a24..b25b366bdc715acd5c90af7ac38cfb3ac88f4c5b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -43,6 +43,7 @@ GlobalProperty hw_compat_9_2[] = {
     { "virtio-balloon-pci-non-transitional", "vectors", "0" },
     { "virtio-mem-pci", "vectors", "0" },
     { "migration", "multifd-clean-tls-termination", "false" },
+    { 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

Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[Michael S. Tsirkin]

On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
> This series was spun off from:
> "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
> (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
> 
> 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>


Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?

> ---
> Changes in v6:
> - Rebased.
> - Link to v5: https://lore.kernel.org/qemu-devel/20250208-virtio-v5-0-4376cb218c0f@daynix.com
> 
> Changes in v5:
> - Covered QAPI more than just qdev.
> - Expanded the description of patch
>   "qapi: Accept bool for OnOffAuto and OnOffSplit".
> - Rebased.
> - Link to v4: https://lore.kernel.org/r/20250108-virtio-v4-0-cbf0aa04c9f9@daynix.com
> 
> Changes in v4:
> - Added patch "qapi: Do not consume a value if failed".
> - Link to v3: https://lore.kernel.org/r/20250104-virtio-v3-0-63ef70e9ddf3@daynix.com
> 
> Changes in v3:
> - Rebased.
> - Link to v2: https://lore.kernel.org/r/20241022-virtio-v2-0-b2394236e053@daynix.com
> 
> Changes in v2:
> - Expanded the message of patch "qdev-properties: Accept bool for
>   OnOffAuto".
> - Link to v1: https://lore.kernel.org/r/20241014-virtio-v1-0-e9ddf7a81891@daynix.com
> 
> ---
> Akihiko Odaki (4):
>       qapi: Do not consume a value if failed
>       qapi: Accept bool for OnOffAuto and OnOffSplit
>       qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
>       virtio: Convert feature properties to OnOffAuto
> 
>  include/hw/qdev-properties.h |  18 ++++++++
>  include/hw/virtio/virtio.h   |  38 +++++++++-------
>  hw/core/machine.c            |   1 +
>  hw/core/qdev-properties.c    |  83 +++++++++++++++++++++++++++++++++-
>  hw/virtio/virtio-bus.c       |  14 +++++-
>  hw/virtio/virtio.c           |   4 +-
>  qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
>  scripts/qapi/visit.py        |  24 ++++++++++
>  8 files changed, 229 insertions(+), 56 deletions(-)
> ---
> base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
> change-id: 20241013-virtio-164ea3f295c3
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[Daniel P. Berrangé]

On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
> > This series was spun off from:
> > "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
> > (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
> > 
> > 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>
> 
> 
> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?

I've said on every previous version that I don't think we should be
changing OnOffAuto to secretly accept bool values. That is bypassing
QAPI schema definitions with a special code hack.

With 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 :|

Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
>> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
>> > This series was spun off from:
>> > "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
>> > (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
>> > 
>> > 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>
>> 
>> 
>> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?
>
> I've said on every previous version that I don't think we should be
> changing OnOffAuto to secretly accept bool values. That is bypassing
> QAPI schema definitions with a special code hack.

I also objected to v4.  Thread starts at

    Message-ID: <87cyfwxveo.fsf@pond.sub.org>
    https://lore.kernel.org/qemu-devel/87cyfwxveo.fsf@pond.sub.org/

I could be persuaded to accept a patch that changes exactly the
properties that need to be changed to tri-state, with suitable
rationale.  This patch changes a bunch of unrelated properties, too.

Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[Akihiko Odaki]

On 2025/04/26 0:08, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
>>> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
>>>> This series was spun off from:
>>>> "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
>>>> (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
>>>>
>>>> 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>
>>>
>>>
>>> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?
>>
>> I've said on every previous version that I don't think we should be
>> changing OnOffAuto to secretly accept bool values. That is bypassing
>> QAPI schema definitions with a special code hack.
> 
> I also objected to v4.  Thread starts at
> 
>      Message-ID: <87cyfwxveo.fsf@pond.sub.org>
>      https://lore.kernel.org/qemu-devel/87cyfwxveo.fsf@pond.sub.org/
> 
> I could be persuaded to accept a patch that changes exactly the
> properties that need to be changed to tri-state, with suitable
> rationale.  This patch changes a bunch of unrelated properties, too.
> 

I replied to the thread as I found that I haven't replied to the last 
message in the thread and the newer versions of the series do not 
address its discussion points either.

Regards,
Akihiko Odaki

Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

[Markus Armbruster]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/04/26 0:08, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>>> On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
>>>> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
>>>>> This series was spun off from:
>>>>> "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
>>>>> (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
>>>>>
>>>>> 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>
>>>>
>>>>
>>>> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?
>>>
>>> I've said on every previous version that I don't think we should be
>>> changing OnOffAuto to secretly accept bool values. That is bypassing
>>> QAPI schema definitions with a special code hack.
>> 
>> I also objected to v4.  Thread starts at
>>      Message-ID: <87cyfwxveo.fsf@pond.sub.org>
>>      https://lore.kernel.org/qemu-devel/87cyfwxveo.fsf@pond.sub.org/
>> I could be persuaded to accept a patch that changes exactly the
>> properties that need to be changed to tri-state, with suitable
>> rationale.  This patch changes a bunch of unrelated properties, too.
>
> I replied to the thread as I found that I haven't replied to the last message in the thread and the newer versions of the series do not address its discussion points either.

I just reiterated and clarified my objection in said thread, and tried
to point towards possible solutions I could accept.