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

[PATCH v4 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 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
      qdev-properties: Accept bool for OnOffAuto
      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            |   4 +-
 hw/core/qdev-properties.c    |  83 +++++++++++++++++++++++++++++++++-
 hw/virtio/virtio-bus.c       |  14 +++++-
 hw/virtio/virtio.c           |   4 +-
 qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
 7 files changed, 207 insertions(+), 57 deletions(-)
---
base-commit: 38d0939b86e2eef6f6a622c6f1f7befda0146595
change-id: 20241013-virtio-164ea3f295c3

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

[PATCH v4 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 f110a804b2ae..799c1c9bd6bd 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.47.1

[PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

Accept bool literals for OnOffAuto properties for consistency with bool
properties. This enables users to set the "on" or "off" value in a
uniform syntax without knowing whether the "auto" value is accepted.
This behavior is especially useful when converting an existing bool
property to OnOffAuto or vice versa.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/core/qdev-properties.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 434a76f5036e..0081d79f9b7b 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
     .set   = set_string,
 };
 
+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);
+    bool value;
+
+    if (visit_type_bool(v, name, &value, NULL)) {
+        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+        return;
+    }
+
+    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
+}
+
 /* --- on/off/auto --- */
 
 const PropertyInfo qdev_prop_on_off_auto = {
@@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
     .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,
 };
 

-- 
2.47.1

[PATCH v4 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 bf27375a3ccd..0d161325e8dc 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 0081d79f9b7b..9be85cc6b7d6 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -187,7 +187,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;
 }
 
@@ -232,6 +233,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);
+    int 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_enum(v, name, &value, &OnOffAuto_lookup, 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);
+    bool bool_value;
+    int value;
+    uint64_t mask = qdev_get_prop_mask64(prop);
+
+    if (visit_type_bool(v, name, &bool_value, NULL)) {
+        value = bool_value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+    } else if (!visit_type_enum(v, name, &value, &OnOffAuto_lookup, 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;
+    }
+}
+
+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.47.1

[PATCH v4 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          |  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

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

[Lei Yang]

I tested this series of patches v4 with virtio-net regression tests,
everything works fine. And the qemu core dump issues that hit it on v3
have gone.

Tested-by: Lei Yang <leiyang@redhat.com>

On Wed, Jan 8, 2025 at 2:18 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
>

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

[Philippe Mathieu-Daudé]

On 8/1/25 07:17, 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          |  4 +++-
>   hw/virtio/virtio-bus.c     | 14 ++++++++++++--
>   hw/virtio/virtio.c         |  4 +++-
>   4 files changed, 39 insertions(+), 21 deletions(-)


> 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);


> 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),

This doesn't seem an experiment, so can we not use the 'x-' prefix?

>   };

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

[Akihiko Odaki]

On 2025/01/09 19:56, Philippe Mathieu-Daudé wrote:
> On 8/1/25 07:17, 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          |  4 +++-
>>   hw/virtio/virtio-bus.c     | 14 ++++++++++++--
>>   hw/virtio/virtio.c         |  4 +++-
>>   4 files changed, 39 insertions(+), 21 deletions(-)
> 
> 
>> 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);
> 
> 
>> 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),
> 
> This doesn't seem an experiment, so can we not use the 'x-' prefix?

I put x- prefix because it is only intended to be used for compatibility 
purpose and users will not be interested in this.

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

[Philippe Mathieu-Daudé]

On 9/1/25 12:08, Akihiko Odaki wrote:
> On 2025/01/09 19:56, Philippe Mathieu-Daudé wrote:
>> On 8/1/25 07:17, 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          |  4 +++-
>>>   hw/virtio/virtio-bus.c     | 14 ++++++++++++--
>>>   hw/virtio/virtio.c         |  4 +++-
>>>   4 files changed, 39 insertions(+), 21 deletions(-)
>>
>>
>>> 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);
>>
>>
>>> 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),
>>
>> This doesn't seem an experiment, so can we not use the 'x-' prefix?
> 
> I put x- prefix because it is only intended to be used for compatibility 
> purpose and users will not be interested in this.

I guess that's OK then.

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

[Markus Armbruster]

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

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

How is this related to "[PATCH v2 0/4] hw/pci: Convert rom_bar into
OnOffAuto"?
https://lore.kernel.org/all/20240714-rombar-v2-0-af1504ef55de@daynix.com/

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

[Akihiko Odaki]

On 2025/01/09 21:53, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> 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>
> 
> How is this related to "[PATCH v2 0/4] hw/pci: Convert rom_bar into
> OnOffAuto"?
> https://lore.kernel.org/all/20240714-rombar-v2-0-af1504ef55de@daynix.com/
 >

They are orthogonal. "[PATCH v2 0/4] hw/pci: Convert rom_bar into
OnOffAuto" was also abandoned in favor of "[PATCH v19 13/14] hw/pci: Use 
-1 as the default value for rombar".
https://lore.kernel.org/r/20250109-reuse-v19-13-f541e82ca5f7@daynix.com

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Daniel P. Berrangé]

On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
> Accept bool literals for OnOffAuto properties for consistency with bool
> properties. This enables users to set the "on" or "off" value in a
> uniform syntax without knowing whether the "auto" value is accepted.
> This behavior is especially useful when converting an existing bool
> property to OnOffAuto or vice versa.

Again, to repeat my previous feedback, OnOffAuto is a well defined
QAPI type - making it secretly accept other values/types behind
the scenes which are not visible in QAPI scheme is not acceptable.

Effectively this is a backdoor impl of a QAPI alternate

  { 'alternate': 'OnOffAutoOrBool',
    'data': {
      'o': 'OnOffAuto',
      'b': 'bool'
    }
  }

except this isn't permitted as the QAPI generator explicitly blocks
use of alternate when the two branches are 'bool' and 'enum'.

I'm assuming this is because in the QemuOpts scenario, it cannot
guess upfront whether the input is a bool or enum. This is unfortunate
though, because at the JSON visitor level it is unambiguous. 

I wonder if the QAPI generator could be relaxed in any viable way ?

> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/core/qdev-properties.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 434a76f5036e..0081d79f9b7b 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
>      .set   = set_string,
>  };
>  
> +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);
> +    bool value;
> +
> +    if (visit_type_bool(v, name, &value, NULL)) {
> +        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> +        return;
> +    }
> +
> +    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
> +}
> +
>  /* --- on/off/auto --- */
>  
>  const PropertyInfo qdev_prop_on_off_auto = {
> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
>      .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,
>  };
>  
> 
> -- 
> 2.47.1
> 

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 v4 4/4] virtio: Convert feature properties to OnOffAuto

[Daniel P. Berrangé]

On Wed, Jan 08, 2025 at 03:17:53PM +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.

This change is addressing two distinct problems at the same time.

The first (big) problem is that permitted host features, and user
requested features are both stored in the same field. This is
logically solved by separating the fields to 'host_features' and
'requested_features'.

To maintain the ability to have automagic disablement of unavailable
features out of the box, however, requires more than just a single
bitmask for the requested features.

There are two approaches to this

 * Separate the tracking of "has value" from "value", with
   "not set" implicitly representing "auto". ie two bitmask fields

 * Switch from bool to a tri-state, with the new default
   value being "auto". ie an araray of enums

The second approach there requires user visible input parsing
changes as proposed in the previous patch.

The ability to explicitly set "auto" on input is only required
if the built-in default value could ever NOT be "auto".

If we can assume the built-in default is *always* auto, then
the user can request 'auto' by simply not setting the parameter.

I'm not especially convinced that we need an explicit "auto"
here, and a variant of DEFINE_PROP_BIT64 could work that let
us track 'has value" from "value" would be sufficient.

If we do want an explicit value, then IMHO we need a better
solution than hacking OnOffAuto to secretly parse bools
without this being represented in the QAPI schema.

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

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 v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/01/10 20:09, Daniel P. Berrangé wrote:
> On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
>> Accept bool literals for OnOffAuto properties for consistency with bool
>> properties. This enables users to set the "on" or "off" value in a
>> uniform syntax without knowing whether the "auto" value is accepted.
>> This behavior is especially useful when converting an existing bool
>> property to OnOffAuto or vice versa.
> 
> Again, to repeat my previous feedback, OnOffAuto is a well defined
> QAPI type - making it secretly accept other values/types behind
> the scenes which are not visible in QAPI scheme is not acceptable.
> 
> Effectively this is a backdoor impl of a QAPI alternate
> 
>    { 'alternate': 'OnOffAutoOrBool',
>      'data': {
>        'o': 'OnOffAuto',
>        'b': 'bool'
>      }
>    }
> 
> except this isn't permitted as the QAPI generator explicitly blocks
> use of alternate when the two branches are 'bool' and 'enum'.

The QAPI generator specifically blocks the case where the enum contains 
'on' or 'off'.

> 
> I'm assuming this is because in the QemuOpts scenario, it cannot
> guess upfront whether the input is a bool or enum. This is unfortunate
> though, because at the JSON visitor level it is unambiguous.

It's probably for the command line and possibly HMP.

> 
> I wonder if the QAPI generator could be relaxed in any viable way ?
It will make the interpretation of 'on' and 'off' on the command line 
ambigious; it can be either of OnOffAuto or bool.

Making some sort of backdoor is necessary to support the proposed 
semantics. We will need a new built-in type to represent the tristate 
value; I haven't added one though as it's not necessary for virtio 
properties I'm concerned of.

> 
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/core/qdev-properties.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 434a76f5036e..0081d79f9b7b 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
>>       .set   = set_string,
>>   };
>>   
>> +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);
>> +    bool value;
>> +
>> +    if (visit_type_bool(v, name, &value, NULL)) {
>> +        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>> +        return;
>> +    }
>> +
>> +    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
>> +}
>> +
>>   /* --- on/off/auto --- */
>>   
>>   const PropertyInfo qdev_prop_on_off_auto = {
>> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
>>       .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,
>>   };
>>   
>>
>> -- 
>> 2.47.1
>>
> 
> With regards,
> Daniel

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

[Akihiko Odaki]

On 2025/01/10 20:23, Daniel P. Berrangé wrote:
> On Wed, Jan 08, 2025 at 03:17:53PM +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.
> 
> This change is addressing two distinct problems at the same time.
> 
> The first (big) problem is that permitted host features, and user
> requested features are both stored in the same field. This is
> logically solved by separating the fields to 'host_features' and
> 'requested_features'.
> 
> To maintain the ability to have automagic disablement of unavailable
> features out of the box, however, requires more than just a single
> bitmask for the requested features.
> 
> There are two approaches to this
> 
>   * Separate the tracking of "has value" from "value", with
>     "not set" implicitly representing "auto". ie two bitmask fields

This approach has its own problem described in the following thread:
https://lore.kernel.org/all/d4f53c8b-4267-4386-bfa6-40ee2b1bbb49@daynix.com/

> 
>   * Switch from bool to a tri-state, with the new default
>     value being "auto". ie an araray of enums
> 
> The second approach there requires user visible input parsing
> changes as proposed in the previous patch.
> 
> The ability to explicitly set "auto" on input is only required
> if the built-in default value could ever NOT be "auto".

Specifically I want the "rss" property of virtio-net be "false" by 
default as it is useful specifically for Windows.

> 
> If we can assume the built-in default is *always* auto, then
> the user can request 'auto' by simply not setting the parameter.
> 
> I'm not especially convinced that we need an explicit "auto"
> here, and a variant of DEFINE_PROP_BIT64 could work that let
> us track 'has value" from "value" would be sufficient.
> 
> If we do want an explicit value, then IMHO we need a better
> solution than hacking OnOffAuto to secretly parse bools
> without this being represented in the QAPI schema.
> 
>> 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
>>
> 
> With regards,
> Daniel

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Daniel P. Berrangé]

On Fri, Jan 10, 2025 at 08:31:57PM +0900, Akihiko Odaki wrote:
> On 2025/01/10 20:09, Daniel P. Berrangé wrote:
> > On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
> > > Accept bool literals for OnOffAuto properties for consistency with bool
> > > properties. This enables users to set the "on" or "off" value in a
> > > uniform syntax without knowing whether the "auto" value is accepted.
> > > This behavior is especially useful when converting an existing bool
> > > property to OnOffAuto or vice versa.
> > 
> > Again, to repeat my previous feedback, OnOffAuto is a well defined
> > QAPI type - making it secretly accept other values/types behind
> > the scenes which are not visible in QAPI scheme is not acceptable.
> > 
> > Effectively this is a backdoor impl of a QAPI alternate
> > 
> >    { 'alternate': 'OnOffAutoOrBool',
> >      'data': {
> >        'o': 'OnOffAuto',
> >        'b': 'bool'
> >      }
> >    }
> > 
> > except this isn't permitted as the QAPI generator explicitly blocks
> > use of alternate when the two branches are 'bool' and 'enum'.
> 
> The QAPI generator specifically blocks the case where the enum contains 'on'
> or 'off'.
> 
> > 
> > I'm assuming this is because in the QemuOpts scenario, it cannot
> > guess upfront whether the input is a bool or enum. This is unfortunate
> > though, because at the JSON visitor level it is unambiguous.
> 
> It's probably for the command line and possibly HMP.
> 
> > 
> > I wonder if the QAPI generator could be relaxed in any viable way ?
> It will make the interpretation of 'on' and 'off' on the command line
> ambigious; it can be either of OnOffAuto or bool.

The ambiguity would not neccessarily matter from a functional POV
though.

The consumer of an "OnOffAutoOrBool" field, would likely want to apply
logic to collapse it into just "OnOffAuto". As such, whether "on" is
parsed as a enum value or a bool value would have no functional
difference in the end. The OnOffAutoOrBool is essentially there to
just make sure we clearly express our input schema.

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 v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/01/10 21:16, Daniel P. Berrangé wrote:
> On Fri, Jan 10, 2025 at 08:31:57PM +0900, Akihiko Odaki wrote:
>> On 2025/01/10 20:09, Daniel P. Berrangé wrote:
>>> On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
>>>> Accept bool literals for OnOffAuto properties for consistency with bool
>>>> properties. This enables users to set the "on" or "off" value in a
>>>> uniform syntax without knowing whether the "auto" value is accepted.
>>>> This behavior is especially useful when converting an existing bool
>>>> property to OnOffAuto or vice versa.
>>>
>>> Again, to repeat my previous feedback, OnOffAuto is a well defined
>>> QAPI type - making it secretly accept other values/types behind
>>> the scenes which are not visible in QAPI scheme is not acceptable.
>>>
>>> Effectively this is a backdoor impl of a QAPI alternate
>>>
>>>     { 'alternate': 'OnOffAutoOrBool',
>>>       'data': {
>>>         'o': 'OnOffAuto',
>>>         'b': 'bool'
>>>       }
>>>     }
>>>
>>> except this isn't permitted as the QAPI generator explicitly blocks
>>> use of alternate when the two branches are 'bool' and 'enum'.
>>
>> The QAPI generator specifically blocks the case where the enum contains 'on'
>> or 'off'.
>>
>>>
>>> I'm assuming this is because in the QemuOpts scenario, it cannot
>>> guess upfront whether the input is a bool or enum. This is unfortunate
>>> though, because at the JSON visitor level it is unambiguous.
>>
>> It's probably for the command line and possibly HMP.
>>
>>>
>>> I wonder if the QAPI generator could be relaxed in any viable way ?
>> It will make the interpretation of 'on' and 'off' on the command line
>> ambigious; it can be either of OnOffAuto or bool.
> 
> The ambiguity would not neccessarily matter from a functional POV
> though.
> 
> The consumer of an "OnOffAutoOrBool" field, would likely want to apply
> logic to collapse it into just "OnOffAuto". As such, whether "on" is
> parsed as a enum value or a bool value would have no functional
> difference in the end. The OnOffAutoOrBool is essentially there to
> just make sure we clearly express our input schema.

I think it is worth creating a special type that requires the logic to 
collapse into OnOffAuto.

If we don't have a special type, the QAPI generator will write C struct 
like as follows:

struct OnOffAutoOrBool {
     QType type;
     union { /* union tag is @type */
         OnOffAuto o;
         bool b;
     } u;
};

It's easier if we have a special type that directly maps into a 
tristate. The story shouldn't be different for other possible consumers; 
a special type will make it easy to map QAPI types into 
consumer-specific type systems.

Regards,
Akihiko Odaki

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Markus Armbruster]

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

> Accept bool literals for OnOffAuto properties for consistency with bool
> properties. This enables users to set the "on" or "off" value in a
> uniform syntax without knowing whether the "auto" value is accepted.
> This behavior is especially useful when converting an existing bool
> property to OnOffAuto or vice versa.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/core/qdev-properties.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 434a76f5036e..0081d79f9b7b 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
>      .set   = set_string,
>  };
>  
> +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);
> +    bool value;
> +
> +    if (visit_type_bool(v, name, &value, NULL)) {
> +        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> +        return;
> +    }
> +
> +    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
> +}
> +
>  /* --- on/off/auto --- */
>  
>  const PropertyInfo qdev_prop_on_off_auto = {
> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
>      .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,
>  };

The qdev properties defined with DEFINE_PROP_ON_OFF_AUTO() now
additionally accept bool.

The commit message tries to explain why this change is useful, but it
leaves me confused.

Does this solve a problem with existing properties?  If yes, what
exactly is the problem?

Or does this enable new uses of DEFINE_PROP_ON_OFF_AUTO()?

I'm trying to understand, but my gut feeling is "bad idea".

Having multiple ways to express the same thing is generally undesirable.
In this case, "foo": "on" and "foo": true, as well as "foo": "off" and
"foo": false.

Moreover, OnOffAuto then has two meanings: straightfoward enum as
defined in the QAPI schema, and the funny qdev property.  This is
definitely a bad idea.  DEFINE_PROP_T(), where T is some QAPI type,
should accept *exactly* the values of T.  If these properties need to
accept something else, use another name to not invite confusion.

If I understand the cover letter correctly, you want to make certain
bool properties tri-state for some reason.  I haven't looked closely
enough to judge whether that makes sense.  But do you really have to
change a whole bunch of unrelated properties to solve your problem?
This is going to be a very hard sell.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/02/06 0:29, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> Accept bool literals for OnOffAuto properties for consistency with bool
>> properties. This enables users to set the "on" or "off" value in a
>> uniform syntax without knowing whether the "auto" value is accepted.
>> This behavior is especially useful when converting an existing bool
>> property to OnOffAuto or vice versa.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/core/qdev-properties.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 434a76f5036e..0081d79f9b7b 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
>>       .set   = set_string,
>>   };
>>   
>> +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);
>> +    bool value;
>> +
>> +    if (visit_type_bool(v, name, &value, NULL)) {
>> +        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>> +        return;
>> +    }
>> +
>> +    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
>> +}
>> +
>>   /* --- on/off/auto --- */
>>   
>>   const PropertyInfo qdev_prop_on_off_auto = {
>> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
>>       .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,
>>   };
> 
> The qdev properties defined with DEFINE_PROP_ON_OFF_AUTO() now
> additionally accept bool.
> 
> The commit message tries to explain why this change is useful, but it
> leaves me confused.
> 
> Does this solve a problem with existing properties?  If yes, what
> exactly is the problem?
> 
> Or does this enable new uses of DEFINE_PROP_ON_OFF_AUTO()?
> 
> I'm trying to understand, but my gut feeling is "bad idea".
> 
> Having multiple ways to express the same thing is generally undesirable.
> In this case, "foo": "on" and "foo": true, as well as "foo": "off" and
> "foo": false.
> 
> Moreover, OnOffAuto then has two meanings: straightfoward enum as
> defined in the QAPI schema, and the funny qdev property.  This is
> definitely a bad idea.  DEFINE_PROP_T(), where T is some QAPI type,
> should accept *exactly* the values of T.  If these properties need to
> accept something else, use another name to not invite confusion.
> 
> If I understand the cover letter correctly, you want to make certain
> bool properties tri-state for some reason.  I haven't looked closely
> enough to judge whether that makes sense.  But do you really have to
> change a whole bunch of unrelated properties to solve your problem?
> This is going to be a very hard sell.
> 

I change various virtio properties because they all have a common 
problem. The problem is, when the host does not support a virtio 
capability, virtio devices automatically set capability properties false 
even if the user explicitly sets them true. This problem can be solved 
using an existing mechanism, OnOffAuto, which differentiates the "auto" 
state and explicit the "on" state.

However, converting bool to OnOffAuto surfaces another problem: they 
disagree how "on" and "off" should be written. Please note that the 
disagreement already exists and so it is nice to solve anyway.

This patch tries to solve it by tolerating bool values for OnOffAuto. As 
you pointed out, this approach has a downside: it makes OnOffAuto more 
complicated by having multiple ways to express the same thing.

Another approach is to have one unified way to express "on"/"off" for 
bool and OnOffAuto. This will give three options in total:

1. Let OnOffAuto accept JSON bool and "on"/"off" (what this patch does)
2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool

I'm fine with either of these approaches; they are at least better than 
the current situation where users need to care if the value is OnOffAuto 
or bool when they just want to express on/off. Please tell me what you 
prefer.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Markus Armbruster]

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

> On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
>> Accept bool literals for OnOffAuto properties for consistency with bool
>> properties. This enables users to set the "on" or "off" value in a
>> uniform syntax without knowing whether the "auto" value is accepted.
>> This behavior is especially useful when converting an existing bool
>> property to OnOffAuto or vice versa.
>
> Again, to repeat my previous feedback, OnOffAuto is a well defined
> QAPI type - making it secretly accept other values/types behind
> the scenes which are not visible in QAPI scheme is not acceptable.
>
> Effectively this is a backdoor impl of a QAPI alternate
>
>   { 'alternate': 'OnOffAutoOrBool',
>     'data': {
>       'o': 'OnOffAuto',
>       'b': 'bool'
>     }
>   }
>
> except this isn't permitted as the QAPI generator explicitly blocks
> use of alternate when the two branches are 'bool' and 'enum'.
>
> I'm assuming this is because in the QemuOpts scenario, it cannot
> guess upfront whether the input is a bool or enum. This is unfortunate
> though, because at the JSON visitor level it is unambiguous. 
>
> I wonder if the QAPI generator could be relaxed in any viable way ?

Discussed in review of a related prior patch:
https://lore.kernel.org/qemu-devel/87h6c4fqz6.fsf@pond.sub.org/

Here's the relevant part for your convenience:

    >> parse the value as enum, and if that fails, as uint32_t.  QAPI already
    >> provides a way to express "either this type or that type": alternate.
    >> Like this:
    >> 
    >>      { 'alternate': 'OnOffAutoUint32',
    >>        'data': { 'sym': 'OnOffAuto',
    >>                  'uint': 'uint32' } }
    >> 
    >> Unfortunately, such alternates don't work on the command line due to
    >> keyval visitor restrictions.  These cannot be lifted entirely, but we
    >> might be able to lift them sufficiently to make this alternate work.
    >
    > The keyval visitor cannot implement alternates because the command line 
    > input does not have type information. For example, you cannot 
    > distinguish string "0" and integer 0.

    Correct.

    For alternate types, an input visitor picks the branch based on the
    QType.

    With JSON, we have scalar types null, number, string, and bool.

    With keyval, we only have string.  Alternates with more than one scalar
    branch don't work.

    They could be made to work to some degree, though.  Observe:

    * Any value can be a string.

    * "frob" can be nothing else.

    * "on" and "off" can also be bool.

    * "123" and "1e3" can also be number or enum.

    Instead of picking the branch based on the QType, we could pick based on
    QType and value, where the value part is delegated to a visitor method.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Markus Armbruster]

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

> On 2025/02/06 0:29, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>> 
>>> Accept bool literals for OnOffAuto properties for consistency with bool
>>> properties. This enables users to set the "on" or "off" value in a
>>> uniform syntax without knowing whether the "auto" value is accepted.
>>> This behavior is especially useful when converting an existing bool
>>> property to OnOffAuto or vice versa.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/core/qdev-properties.c | 17 ++++++++++++++++-
>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 434a76f5036e..0081d79f9b7b 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
>>>       .set   = set_string,
>>>   };
>>>   
>>> +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);
>>> +    bool value;
>>> +
>>> +    if (visit_type_bool(v, name, &value, NULL)) {
>>> +        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>>> +        return;
>>> +    }
>>> +
>>> +    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
>>> +}
>>> +
>>>   /* --- on/off/auto --- */
>>>   
>>>   const PropertyInfo qdev_prop_on_off_auto = {
>>> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
>>>       .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,
>>>   };
>> 
>> The qdev properties defined with DEFINE_PROP_ON_OFF_AUTO() now
>> additionally accept bool.
>> 
>> The commit message tries to explain why this change is useful, but it
>> leaves me confused.
>> 
>> Does this solve a problem with existing properties?  If yes, what
>> exactly is the problem?
>> 
>> Or does this enable new uses of DEFINE_PROP_ON_OFF_AUTO()?
>> 
>> I'm trying to understand, but my gut feeling is "bad idea".
>> 
>> Having multiple ways to express the same thing is generally undesirable.
>> In this case, "foo": "on" and "foo": true, as well as "foo": "off" and
>> "foo": false.
>> 
>> Moreover, OnOffAuto then has two meanings: straightfoward enum as
>> defined in the QAPI schema, and the funny qdev property.  This is
>> definitely a bad idea.  DEFINE_PROP_T(), where T is some QAPI type,
>> should accept *exactly* the values of T.  If these properties need to
>> accept something else, use another name to not invite confusion.
>> 
>> If I understand the cover letter correctly, you want to make certain
>> bool properties tri-state for some reason.  I haven't looked closely
>> enough to judge whether that makes sense.  But do you really have to
>> change a whole bunch of unrelated properties to solve your problem?
>> This is going to be a very hard sell.
>> 
>
> I change various virtio properties because they all have a common 
> problem. The problem is, when the host does not support a virtio 
> capability, virtio devices automatically set capability properties false 
> even if the user explicitly sets them true.

I understand we have something like this:

* true: on if possible, else off

* false: off (always possible)

Which one is the default?

There is no way to reliably configure "on", i.e. fail if it's not
possible.  I agree that's a problem.

>                                             This problem can be solved 
> using an existing mechanism, OnOffAuto, which differentiates the "auto" 
> state and explicit the "on" state.

I guess you're proposing something like this:

* auto: on if possible, else off

* on: on if possible, else error

* off: off (always possible)

Which one is the default?

> However, converting bool to OnOffAuto surfaces another problem: they 
> disagree how "on" and "off" should be written. Please note that the 
> disagreement already exists and so it is nice to solve anyway.

Yes, converting bool to OnOffAuto is an incompatible change.

> This patch tries to solve it by tolerating bool values for OnOffAuto. As 
> you pointed out, this approach has a downside: it makes OnOffAuto more 
> complicated by having multiple ways to express the same thing.

It also affects existing uses of OnOffAuto, where such a change is
unnecessary and undesirable.

> Another approach is to have one unified way to express "on"/"off" for 
> bool and OnOffAuto. This will give three options in total:
>
> 1. Let OnOffAuto accept JSON bool and "on"/"off" (what this patch does)

The parenthesis is inaccurate.  This patch only affects qdev properties.
It does not affect use of OnOffAuto elsewhere, e.g. QOM object
"sev-guest" property "legacy-vm-type", or QMP command blockdev-add
argument "locking" with driver "file".

> 2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
> 3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool

For each of these options:

(a) Change exactly the uses of OnOffAuto that need to become tri-state

(b) Change all qdev properties (currently a superset of (a); what this
   patch does)

(c) Change all uses of OnOffAuto

I dislike (c) and especially (b).

> I'm fine with either of these approaches; they are at least better than 
> the current situation where users need to care if the value is OnOffAuto 
> or bool when they just want to express on/off. Please tell me what you 
> prefer.

We managed to maneuver ourselves into a bit of a corner in just a few
simple steps:

* The obvious type for a flag is bool.

* The obvious type for a small set of values is enum.

* Thus, the obvious type for a tri-state is enum.

* But this prevents growing a flag into a tri-state compatibly.  Which
  is what you want to do.

However, we actually have a second way to do a tri-state: optional bool,
i.e. present and true, present and false, absent.

Permit me a digression...  I'm not a fan of assigning "absent" a meaning
different from any present value.  But it's a design choice QAPI made.

Using optional that way can occasionally lead to trouble.  Consider
migrate-set-parameters.  Its arguments are all optional.  For each
argument present, the respective migration parameter is set to the
argument value.  You cannot use this to reset a migration parameter from
present to absent.  Matters for parameters where "absent" has a meaning
different from any "present" value.

End of digression.

Start of next digression :)

Note that qdev properties are generally optional.  The only way to make
them mandatory is to reject their default value in .realize().  When
users set this default value explicitly, the error message will almost
certainly be confusing.

End of digression.

Optional bool may enable a fourth solution:

4. Make "absent" mean on if possible, else off, "present and true" mean
   on if possible, else error, and "present and false" mean off (always
   possible).

   This changes the meaning of "present and true", but it's precisely
   the change you want, isn't it?

Yet another solutions:

5. Alternate of bool and an enum with a single value "auto".

   Falls apart with the keyval visitor used for the command line.
   Fixable, I believe, but a good chunk of work and complexity.

My gut feeling: explore 4. first.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/02/06 18:48, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/02/06 0:29, Markus Armbruster wrote:
>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>
>>>> Accept bool literals for OnOffAuto properties for consistency with bool
>>>> properties. This enables users to set the "on" or "off" value in a
>>>> uniform syntax without knowing whether the "auto" value is accepted.
>>>> This behavior is especially useful when converting an existing bool
>>>> property to OnOffAuto or vice versa.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    hw/core/qdev-properties.c | 17 ++++++++++++++++-
>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>>> index 434a76f5036e..0081d79f9b7b 100644
>>>> --- a/hw/core/qdev-properties.c
>>>> +++ b/hw/core/qdev-properties.c
>>>> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
>>>>        .set   = set_string,
>>>>    };
>>>>    
>>>> +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);
>>>> +    bool value;
>>>> +
>>>> +    if (visit_type_bool(v, name, &value, NULL)) {
>>>> +        *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qdev_propinfo_set_enum(obj, v, name, opaque, errp);
>>>> +}
>>>> +
>>>>    /* --- on/off/auto --- */
>>>>    
>>>>    const PropertyInfo qdev_prop_on_off_auto = {
>>>> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
>>>>        .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,
>>>>    };
>>>
>>> The qdev properties defined with DEFINE_PROP_ON_OFF_AUTO() now
>>> additionally accept bool.
>>>
>>> The commit message tries to explain why this change is useful, but it
>>> leaves me confused.
>>>
>>> Does this solve a problem with existing properties?  If yes, what
>>> exactly is the problem?
>>>
>>> Or does this enable new uses of DEFINE_PROP_ON_OFF_AUTO()?
>>>
>>> I'm trying to understand, but my gut feeling is "bad idea".
>>>
>>> Having multiple ways to express the same thing is generally undesirable.
>>> In this case, "foo": "on" and "foo": true, as well as "foo": "off" and
>>> "foo": false.
>>>
>>> Moreover, OnOffAuto then has two meanings: straightfoward enum as
>>> defined in the QAPI schema, and the funny qdev property.  This is
>>> definitely a bad idea.  DEFINE_PROP_T(), where T is some QAPI type,
>>> should accept *exactly* the values of T.  If these properties need to
>>> accept something else, use another name to not invite confusion.
>>>
>>> If I understand the cover letter correctly, you want to make certain
>>> bool properties tri-state for some reason.  I haven't looked closely
>>> enough to judge whether that makes sense.  But do you really have to
>>> change a whole bunch of unrelated properties to solve your problem?
>>> This is going to be a very hard sell.
>>>
>>
>> I change various virtio properties because they all have a common
>> problem. The problem is, when the host does not support a virtio
>> capability, virtio devices automatically set capability properties false
>> even if the user explicitly sets them true.

First, I'd like to thank you for your detailed reply.

> 
> I understand we have something like this:
> 
> * true: on if possible, else off
> 
> * false: off (always possible)
> 
> Which one is the default?

It depends. Some properties have true by default. The others have false.

> 
> There is no way to reliably configure "on", i.e. fail if it's not
> possible.  I agree that's a problem.
> 
>>                                              This problem can be solved
>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>> state and explicit the "on" state.
> 
> I guess you're proposing something like this:
> 
> * auto: on if possible, else off
> 
> * on: on if possible, else error
> 
> * off: off (always possible)
> 
> Which one is the default?

I converted on to auto and off to false in a following patch.

> 
>> However, converting bool to OnOffAuto surfaces another problem: they
>> disagree how "on" and "off" should be written. Please note that the
>> disagreement already exists and so it is nice to solve anyway.
> 
> Yes, converting bool to OnOffAuto is an incompatible change.

Not just about conversion, but this inconsistency require users to know 
whether a property is bool or OnOffAuto and change how the values are 
written in JSON accordingly. This somewhat hurts usability.

> 
>> This patch tries to solve it by tolerating bool values for OnOffAuto. As
>> you pointed out, this approach has a downside: it makes OnOffAuto more
>> complicated by having multiple ways to express the same thing.
> 
> It also affects existing uses of OnOffAuto, where such a change is
> unnecessary and undesirable.
> 
>> Another approach is to have one unified way to express "on"/"off" for
>> bool and OnOffAuto. This will give three options in total:
>>
>> 1. Let OnOffAuto accept JSON bool and "on"/"off" (what this patch does)
> 
> The parenthesis is inaccurate.  This patch only affects qdev properties.
> It does not affect use of OnOffAuto elsewhere, e.g. QOM object
> "sev-guest" property "legacy-vm-type", or QMP command blockdev-add
> argument "locking" with driver "file".
> 
>> 2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
>> 3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool
> 
> For each of these options:
> 
> (a) Change exactly the uses of OnOffAuto that need to become tri-state
> 
> (b) Change all qdev properties (currently a superset of (a); what this
>     patch does)
> 
> (c) Change all uses of OnOffAuto
> 
> I dislike (c) and especially (b).
> 
>> I'm fine with either of these approaches; they are at least better than
>> the current situation where users need to care if the value is OnOffAuto
>> or bool when they just want to express on/off. Please tell me what you
>> prefer.
> 
> We managed to maneuver ourselves into a bit of a corner in just a few
> simple steps:
> 
> * The obvious type for a flag is bool.
> 
> * The obvious type for a small set of values is enum.
> 
> * Thus, the obvious type for a tri-state is enum.
> 
> * But this prevents growing a flag into a tri-state compatibly.  Which
>    is what you want to do.
> 
> However, we actually have a second way to do a tri-state: optional bool,
> i.e. present and true, present and false, absent.
> 
> Permit me a digression...  I'm not a fan of assigning "absent" a meaning
> different from any present value.  But it's a design choice QAPI made.

It's a new insight I didn't know. Properties in qdev have a default 
value instead of special "absent". But if QAPI does have special 
"absent", perhaps qdev may be modified to align with.

> 
> Using optional that way can occasionally lead to trouble.  Consider
> migrate-set-parameters.  Its arguments are all optional.  For each
> argument present, the respective migration parameter is set to the
> argument value.  You cannot use this to reset a migration parameter from
> present to absent.  Matters for parameters where "absent" has a meaning
> different from any "present" value.
> 
> End of digression.
> 
> Start of next digression :)
> 
> Note that qdev properties are generally optional.  The only way to make
> them mandatory is to reject their default value in .realize().  When
> users set this default value explicitly, the error message will almost
> certainly be confusing.
> 
> End of digression.
> 
> Optional bool may enable a fourth solution:
> 
> 4. Make "absent" mean on if possible, else off, "present and true" mean
>     on if possible, else error, and "present and false" mean off (always
>     possible).
> 
>     This changes the meaning of "present and true", but it's precisely
>     the change you want, isn't it?

We have "false by default" properties so it unfortunately does not work.

> 
> Yet another solutions:
> 
> 5. Alternate of bool and an enum with a single value "auto".
> 
>     Falls apart with the keyval visitor used for the command line.
>     Fixable, I believe, but a good chunk of work and complexity.

I may have missed something, but I think that will break JSON string 
literals "on" and "off".

Regards,
Akihiko Odaki

> 
> My gut feeling: explore 4. first.
> 

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[BALATON Zoltan]

On Thu, 6 Feb 2025, Akihiko Odaki wrote:
> On 2025/02/06 18:48, Markus Armbruster wrote:
>>>                                              This problem can be solved
>>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>>> state and explicit the "on" state.
>> 
>> I guess you're proposing something like this:
>> 
>> * auto: on if possible, else off
>> 
>> * on: on if possible, else error
>> 
>> * off: off (always possible)
>> 
>> Which one is the default?
>
> I converted on to auto and off to false in a following patch.
>
>> 
>>> However, converting bool to OnOffAuto surfaces another problem: they
>>> disagree how "on" and "off" should be written. Please note that the
>>> disagreement already exists and so it is nice to solve anyway.
>> 
>> Yes, converting bool to OnOffAuto is an incompatible change.
>
> Not just about conversion, but this inconsistency require users to know 
> whether a property is bool or OnOffAuto and change how the values are written 
> in JSON accordingly. This somewhat hurts usability.

Worse than that, the help text is also confusing.
Excerpt from -device virtio-gpu,help

   blob=<bool>            - on/off (default: false)
   busnr=<busnr>
   disable-legacy=<OnOffAuto> - on/off/auto (default: "auto")
   disable-modern=<bool>  -  (default: false)
   edid=<bool>            - on/off (default: true)
   event_idx=<bool>       - on/off (default: true)

For bools it says on/off yet expects true/false. Wasn't originally 
true/on/1 and false/off/0 accepted for bools? Where that got lost? I think 
getting back that behaviour would be easiest for users. Users don't care 
if OnOffAuto is an enum internally and can't (or don't want to) remember 
if bools are written on/off or true/false so accepting these as synonyms 
would help users.

Regads,
BALATON Zoltan

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/02/06 22:23, BALATON Zoltan wrote:
> On Thu, 6 Feb 2025, Akihiko Odaki wrote:
>> On 2025/02/06 18:48, Markus Armbruster wrote:
>>>>                                              This problem can be solved
>>>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>>>> state and explicit the "on" state.
>>>
>>> I guess you're proposing something like this:
>>>
>>> * auto: on if possible, else off
>>>
>>> * on: on if possible, else error
>>>
>>> * off: off (always possible)
>>>
>>> Which one is the default?
>>
>> I converted on to auto and off to false in a following patch.
>>
>>>
>>>> However, converting bool to OnOffAuto surfaces another problem: they
>>>> disagree how "on" and "off" should be written. Please note that the
>>>> disagreement already exists and so it is nice to solve anyway.
>>>
>>> Yes, converting bool to OnOffAuto is an incompatible change.
>>
>> Not just about conversion, but this inconsistency require users to 
>> know whether a property is bool or OnOffAuto and change how the values 
>> are written in JSON accordingly. This somewhat hurts usability.
> 
> Worse than that, the help text is also confusing.
> Excerpt from -device virtio-gpu,help
> 
>    blob=<bool>            - on/off (default: false)
>    busnr=<busnr>
>    disable-legacy=<OnOffAuto> - on/off/auto (default: "auto")
>    disable-modern=<bool>  -  (default: false)
>    edid=<bool>            - on/off (default: true)
>    event_idx=<bool>       - on/off (default: true)
> 
> For bools it says on/off yet expects true/false. Wasn't originally true/ 
> on/1 and false/off/0 accepted for bools? Where that got lost? I think 
> getting back that behaviour would be easiest for users. Users don't care 
> if OnOffAuto is an enum internally and can't (or don't want to) remember 
> if bools are written on/off or true/false so accepting these as synonyms 
> would help users.

The help shows another problem: it mixes two different syntaxes. I sent 
a patch to fix the inconsistency in the help. Please review it for more 
detailed explanation and actual fix:
https://lore.kernel.org/qemu-devel/20250207-bool-v1-1-5749d5d6df24@daynix.com

Let me go back to the discussion of the bool/OnOffAuto problem below:

The values the command line syntax accepts are on/yes/true/y and 
off/no/false/n.

For the command line syntax, you can always use on/off whether the type 
is bool or OnOffAuto. In my opinion, it is still not good to reject 
yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them 
when the property gets the "auto" value? As you pointed out, the usage 
of enum is our internal concern and should not bother users.

The situation is worse for JSON as there is no common literals that are 
compatible with both of bool and OnOffAuto, which forces users to 
remember the type.

So I think this patch makes sense in terms of usability. Accepting 
multiple representations for one value is ugly, but it is better than 
exposing the ugliness to users. We should deprecate the representations 
except one if we really hate the ugliness.

Regards,
Akihiko Odaki

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 6 Feb 2025, Akihiko Odaki wrote:
>> On 2025/02/06 18:48, Markus Armbruster wrote:
>>>>                                              This problem can be solved
>>>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>>>> state and explicit the "on" state.
>>>
>>> I guess you're proposing something like this:
>>>
>>> * auto: on if possible, else off
>>>
>>> * on: on if possible, else error
>>>
>>> * off: off (always possible)
>>>
>>> Which one is the default?
>>
>> I converted on to auto and off to false in a following patch.
>>
>>>> However, converting bool to OnOffAuto surfaces another problem: they
>>>> disagree how "on" and "off" should be written. Please note that the
>>>> disagreement already exists and so it is nice to solve anyway.
>>> 
>>> Yes, converting bool to OnOffAuto is an incompatible change.
>>
>> Not just about conversion, but this inconsistency require users to know whether a property is bool or OnOffAuto and change how the values are written in JSON accordingly. This somewhat hurts usability.
>
> Worse than that, the help text is also confusing.
> Excerpt from -device virtio-gpu,help
>
>   blob=<bool>            - on/off (default: false)
>   busnr=<busnr>
>   disable-legacy=<OnOffAuto> - on/off/auto (default: "auto")
>   disable-modern=<bool>  -  (default: false)
>   edid=<bool>            - on/off (default: true)
>   event_idx=<bool>       - on/off (default: true)
>
> For bools it says on/off yet expects true/false. Wasn't originally true/on/1 and false/off/0 accepted for bools? Where that got lost? I think getting back that behaviour would be easiest for users. Users don't care if OnOffAuto is an enum internally and can't (or don't want to) remember if bools are written on/off or true/false so accepting these as synonyms would help users.

The help text printed by -device is about usage of -device, not about
QMP.

There, the preferred syntax for bool values is on/off, but
yes/true/y/no/false/n are also accepted.  I think that's a disgusting
mess, but it's here to stay, so let's not argue this any further.

Instead of "(default: true)" we should have "(default: on)".

All of this is a digression from the topic at hand, which is QMP.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Markus Armbruster]

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

[...]

> Let me go back to the discussion of the bool/OnOffAuto problem below:
>
> The values the command line syntax accepts are on/yes/true/y and off/no/false/n.
>
> For the command line syntax, you can always use on/off whether the type is bool or OnOffAuto. In my opinion, it is still not good to reject yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them when the property gets the "auto" value? As you pointed out, the usage of enum is our internal concern and should not bother users.

The command line is a different mess.

For better or worse (worse if you ask me), we added code to accept
additional syntax for bool values.

Doing the same for enums that happen to have some values that look
boolean at a glance is in my opinion a terrible idea.  We have at least
two: OnOffAuto and OnOffSplit.

But let's get back to QMP.

> The situation is worse for JSON as there is no common literals that are compatible with both of bool and OnOffAuto, which forces users to remember the type.

JSON is primarily for machines, and machines are very good at
remembering the type.

An argument can be made that OnOffAuto is problematic interface design.
In fact, I made it; see "managed to maneuver ourselves into a bit of a
corner" upthread.

> So I think this patch makes sense in terms of usability. Accepting multiple representations for one value is ugly, but it is better than exposing the ugliness to users. We should deprecate the representations except one if we really hate the ugliness.

I believe churn & complexity outweigh the benefits.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Daniel P. Berrangé]

On Fri, Feb 07, 2025 at 01:31:47PM +0100, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
> [...]
> 
> > Let me go back to the discussion of the bool/OnOffAuto problem below:
> >
> > The values the command line syntax accepts are on/yes/true/y and off/no/false/n.
> >
> > For the command line syntax, you can always use on/off whether the type is bool or OnOffAuto. In my opinion, it is still not good to reject yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them when the property gets the "auto" value? As you pointed out, the usage of enum is our internal concern and should not bother users.
> 
> The command line is a different mess.
> 
> For better or worse (worse if you ask me), we added code to accept
> additional syntax for bool values.
> 
> Doing the same for enums that happen to have some values that look
> boolean at a glance is in my opinion a terrible idea.  We have at least
> two: OnOffAuto and OnOffSplit.
> 
> But let's get back to QMP.

Before we get back to QMP I should point out that our current HMP bool /
OnOffAuto properties are a significant developer foot-gun in terms of
back compat.

Though I'm struggling to find the examples, I'm pretty sure I've seen
patches where we converted a property from bool to OnOffAuto, with the
developer (likely) thinking it was back-compatible.

It does have the illusion of being compatible given that the HMP bool
syntax is accepting 'on/off' (especially when our usage examples often
on/off rather than yes/no/true/false) values ... very much not the
case for QMP though.

This does make me really want the idea of an "alternate" in QMP that
can be made to work for scalars.


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 v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/02/07 21:46, Daniel P. Berrangé wrote:
> On Fri, Feb 07, 2025 at 01:31:47PM +0100, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>
>> [...]
>>
>>> Let me go back to the discussion of the bool/OnOffAuto problem below:
>>>
>>> The values the command line syntax accepts are on/yes/true/y and off/no/false/n.
>>>
>>> For the command line syntax, you can always use on/off whether the type is bool or OnOffAuto. In my opinion, it is still not good to reject yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them when the property gets the "auto" value? As you pointed out, the usage of enum is our internal concern and should not bother users.
>>
>> The command line is a different mess.
>>
>> For better or worse (worse if you ask me), we added code to accept
>> additional syntax for bool values.
>>
>> Doing the same for enums that happen to have some values that look
>> boolean at a glance is in my opinion a terrible idea.  We have at least
>> two: OnOffAuto and OnOffSplit.

I agree that we shouldn't have additional syntax for on/off in the first 
place.

However, we do have the additional syntax when the property is typed as 
bool, and treating it differently for OnOffAuto is even more confusing. 
The argument made by BALATON Zoltan still stands whether the additional 
syntax is ugly or not:

 > For bools it says on/off yet expects true/false. Wasn't originally
 > true/ on/1 and false/off/0 accepted for bools? Where that got lost?
 > I think getting back that behaviour would be easiest for users. Users
 > don't care if OnOffAuto is an enum internally and can't (or don't want
 > to) remember if bools are written on/off or true/false so accepting
 > these as synonyms would help users.

This also applies whether it's in the JSON syntax, whether the client is 
a machine. Users will be freed from the mess by us dealing with it with 
40 additional lines (the number of lines are of v6).

>>
>> But let's get back to QMP.
> 
> Before we get back to QMP I should point out that our current HMP bool /
> OnOffAuto properties are a significant developer foot-gun in terms of
> back compat.
> 
> Though I'm struggling to find the examples, I'm pretty sure I've seen
> patches where we converted a property from bool to OnOffAuto, with the
> developer (likely) thinking it was back-compatible.
> 
> It does have the illusion of being compatible given that the HMP bool
> syntax is accepting 'on/off' (especially when our usage examples often
> on/off rather than yes/no/true/false) values ... very much not the
> case for QMP though.
> 
> This does make me really want the idea of an "alternate" in QMP that
> can be made to work for scalars.

I argued that "alternate" will not work well to solve problems with 
existing types, OnOffAuto and OnOffSplit at:
https://lore.kernel.org/qemu-devel/2299cb79-f03e-45e6-b536-66dab16830d1@daynix.com/

Regards,
Akihiko Odaki

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Markus Armbruster]

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

> On 2025/02/06 18:48, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:

[...]

>> I understand we have something like this:
>> 
>> * true: on if possible, else off
>> 
>> * false: off (always possible)
>> 
>> Which one is the default?
>
> It depends. Some properties have true by default. The others have false.
>
>> 
>> There is no way to reliably configure "on", i.e. fail if it's not
>> possible.  I agree that's a problem.
>> 
>>>                                              This problem can be solved
>>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>>> state and explicit the "on" state.
>> 
>> I guess you're proposing something like this:
>> 
>> * auto: on if possible, else off
>> 
>> * on: on if possible, else error
>> 
>> * off: off (always possible)
>> 
>> Which one is the default?
>
> I converted on to auto and off to false in a following patch.
>
>> 
>>> However, converting bool to OnOffAuto surfaces another problem: they
>>> disagree how "on" and "off" should be written. Please note that the
>>> disagreement already exists and so it is nice to solve anyway.
>> 
>> Yes, converting bool to OnOffAuto is an incompatible change.
>
> Not just about conversion, but this inconsistency require users to know 
> whether a property is bool or OnOffAuto and change how the values are 
> written in JSON accordingly. This somewhat hurts usability.
>
>> 
>>> This patch tries to solve it by tolerating bool values for OnOffAuto. As
>>> you pointed out, this approach has a downside: it makes OnOffAuto more
>>> complicated by having multiple ways to express the same thing.
>> 
>> It also affects existing uses of OnOffAuto, where such a change is
>> unnecessary and undesirable.

To be clear: this is pretty much a deal-breaker for me.

We established above that you need certain boolean properties to take a
third state.  I'm willing to discuss patches that change exactly these
properties.  I'm going to reject patches that affect properties that do
not need such a change.

>>> Another approach is to have one unified way to express "on"/"off" for
>>> bool and OnOffAuto. This will give three options in total:
>>>
>>> 1. Let OnOffAuto accept JSON bool and "on"/"off" (what this patch does)
>> 
>> The parenthesis is inaccurate.  This patch only affects qdev properties.
>> It does not affect use of OnOffAuto elsewhere, e.g. QOM object
>> "sev-guest" property "legacy-vm-type", or QMP command blockdev-add
>> argument "locking" with driver "file".
>> 
>>> 2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
>>> 3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool
>> 
>> For each of these options:
>> 
>> (a) Change exactly the uses of OnOffAuto that need to become tri-state
>> 
>> (b) Change all qdev properties (currently a superset of (a); what this
>>     patch does)
>> 
>> (c) Change all uses of OnOffAuto
>> 
>> I dislike (c) and especially (b).
>> 
>>> I'm fine with either of these approaches; they are at least better than
>>> the current situation where users need to care if the value is OnOffAuto
>>> or bool when they just want to express on/off. Please tell me what you
>>> prefer.
>> 
>> We managed to maneuver ourselves into a bit of a corner in just a few
>> simple steps:
>> 
>> * The obvious type for a flag is bool.
>> 
>> * The obvious type for a small set of values is enum.
>> 
>> * Thus, the obvious type for a tri-state is enum.
>> 
>> * But this prevents growing a flag into a tri-state compatibly.  Which
>>   is what you want to do.
>> 
>> However, we actually have a second way to do a tri-state: optional bool,
>> i.e. present and true, present and false, absent.
>> 
>> Permit me a digression...  I'm not a fan of assigning "absent" a meaning
>> different from any present value.  But it's a design choice QAPI made.
>
> It's a new insight I didn't know. Properties in qdev have a default 
> value instead of special "absent". But if QAPI does have special 
> "absent", perhaps qdev may be modified to align with.

Nothing stops you from creating qdev properties with a special "absent"
value.  All you need is a special value that cannot be set.

In fact, the humble "str" property already works that way: it's a char *
where null means "absent".

Code can recognize "absent" and do whatever needs doing then.  For
instance, consider device "ide-cd".  It has three such properties:
"ver", "serial", and "model".  "ver" defaults to "2.5+", "serial" to
some unique string, but "model" defaults to NULL.  Since you cannot set
such a value, it effectively means "absent".  The code responsible for
this is in ide_dev_initfn():

    if (!dev->version) {
        dev->version = g_strdup(s->version);
    }
    if (!dev->serial) {
        dev->serial = g_strdup(s->drive_serial_str);
    }

Note it leaves a null dev->model null.

>> Using optional that way can occasionally lead to trouble.  Consider
>> migrate-set-parameters.  Its arguments are all optional.  For each
>> argument present, the respective migration parameter is set to the
>> argument value.  You cannot use this to reset a migration parameter from
>> present to absent.  Matters for parameters where "absent" has a meaning
>> different from any "present" value.
>> 
>> End of digression.
>> 
>> Start of next digression :)
>> 
>> Note that qdev properties are generally optional.  The only way to make
>> them mandatory is to reject their default value in .realize().  When
>> users set this default value explicitly, the error message will almost
>> certainly be confusing.
>> 
>> End of digression.
>> 
>> Optional bool may enable a fourth solution:
>> 
>> 4. Make "absent" mean on if possible, else off, "present and true" mean
>>    on if possible, else error, and "present and false" mean off (always
>>    possible).
>> 
>>    This changes the meaning of "present and true", but it's precisely
>>    the change you want, isn't it?
>
> We have "false by default" properties so it unfortunately does not work.

Then make the code make "absent" mean what you need it to mean.  Just
like the code from ide_dev_initfn() I quoted above.

>> Yet another solutions:
>> 
>> 5. Alternate of bool and an enum with a single value "auto".
>> 
>>     Falls apart with the keyval visitor used for the command line.
>>     Fixable, I believe, but a good chunk of work and complexity.
>
> I may have missed something, but I think that will break JSON string 
> literals "on" and "off".

Unbreaking it will be a good chunk of work and complexity, I believe.

>> My gut feeling: explore 4. first.

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[BALATON Zoltan]

On Tue, 6 May 2025, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
>> On 2025/02/06 18:48, Markus Armbruster wrote:
>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
> [...]
>
>>> I understand we have something like this:
>>>
>>> * true: on if possible, else off
>>>
>>> * false: off (always possible)
>>>
>>> Which one is the default?
>>
>> It depends. Some properties have true by default. The others have false.
>>
>>>
>>> There is no way to reliably configure "on", i.e. fail if it's not
>>> possible.  I agree that's a problem.
>>>
>>>>                                              This problem can be solved
>>>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>>>> state and explicit the "on" state.
>>>
>>> I guess you're proposing something like this:
>>>
>>> * auto: on if possible, else off
>>>
>>> * on: on if possible, else error
>>>
>>> * off: off (always possible)
>>>
>>> Which one is the default?
>>
>> I converted on to auto and off to false in a following patch.
>>
>>>
>>>> However, converting bool to OnOffAuto surfaces another problem: they
>>>> disagree how "on" and "off" should be written. Please note that the
>>>> disagreement already exists and so it is nice to solve anyway.
>>>
>>> Yes, converting bool to OnOffAuto is an incompatible change.
>>
>> Not just about conversion, but this inconsistency require users to know
>> whether a property is bool or OnOffAuto and change how the values are
>> written in JSON accordingly. This somewhat hurts usability.
>>
>>>
>>>> This patch tries to solve it by tolerating bool values for OnOffAuto. As
>>>> you pointed out, this approach has a downside: it makes OnOffAuto more
>>>> complicated by having multiple ways to express the same thing.
>>>
>>> It also affects existing uses of OnOffAuto, where such a change is
>>> unnecessary and undesirable.
>
> To be clear: this is pretty much a deal-breaker for me.
>
> We established above that you need certain boolean properties to take a
> third state.  I'm willing to discuss patches that change exactly these
> properties.  I'm going to reject patches that affect properties that do
> not need such a change.

Even if the change is backwards compatible not breaking existing usage 
just adding additional ways to specify same value?

I may not understand this problem completely and may have forgotten what I 
understood but I think the proposed solution only added new alternative 
names for existing values so the old values still work. Then the only way 
this could break if somebody using new names want them to work on older 
QEMU versions but that's not reasonable. Also JSON is not primarily used 
by humans but by management apps and scripts which won't change the names 
they use when we add new names that will likely only be used on the 
command line so this should not break it even if those apps or script are 
used with older QEMU.

>>>> Another approach is to have one unified way to express "on"/"off" for
>>>> bool and OnOffAuto. This will give three options in total:
>>>>
>>>> 1. Let OnOffAuto accept JSON bool and "on"/"off" (what this patch does)
>>>
>>> The parenthesis is inaccurate.  This patch only affects qdev properties.
>>> It does not affect use of OnOffAuto elsewhere, e.g. QOM object
>>> "sev-guest" property "legacy-vm-type", or QMP command blockdev-add
>>> argument "locking" with driver "file".
>>>
>>>> 2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
>>>> 3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool
>>>
>>> For each of these options:
>>>
>>> (a) Change exactly the uses of OnOffAuto that need to become tri-state
>>>
>>> (b) Change all qdev properties (currently a superset of (a); what this
>>>     patch does)
>>>
>>> (c) Change all uses of OnOffAuto
>>>
>>> I dislike (c) and especially (b).
>>>
>>>> I'm fine with either of these approaches; they are at least better than
>>>> the current situation where users need to care if the value is OnOffAuto
>>>> or bool when they just want to express on/off. Please tell me what you
>>>> prefer.
>>>
>>> We managed to maneuver ourselves into a bit of a corner in just a few
>>> simple steps:
>>>
>>> * The obvious type for a flag is bool.
>>>
>>> * The obvious type for a small set of values is enum.
>>>
>>> * Thus, the obvious type for a tri-state is enum.
>>>
>>> * But this prevents growing a flag into a tri-state compatibly.  Which
>>>   is what you want to do.
>>>
>>> However, we actually have a second way to do a tri-state: optional bool,
>>> i.e. present and true, present and false, absent.
>>>
>>> Permit me a digression...  I'm not a fan of assigning "absent" a meaning
>>> different from any present value.  But it's a design choice QAPI made.
>>
>> It's a new insight I didn't know. Properties in qdev have a default
>> value instead of special "absent". But if QAPI does have special
>> "absent", perhaps qdev may be modified to align with.
>
> Nothing stops you from creating qdev properties with a special "absent"
> value.  All you need is a special value that cannot be set.

The problem is that the default value is 0 and that means false. You can't 
change it without either changing the default or requiring to set the 
default explicitly which may need a lot of changes and error prone so we 
should keep the current default to avoid this.

> In fact, the humble "str" property already works that way: it's a char *
> where null means "absent".

This case works as the default value matches the uninitialised value but 
would not work for bool.

> Code can recognize "absent" and do whatever needs doing then.  For
> instance, consider device "ide-cd".  It has three such properties:
> "ver", "serial", and "model".  "ver" defaults to "2.5+", "serial" to
> some unique string, but "model" defaults to NULL.  Since you cannot set
> such a value, it effectively means "absent".  The code responsible for
> this is in ide_dev_initfn():
>
>    if (!dev->version) {
>        dev->version = g_strdup(s->version);
>    }
>    if (!dev->serial) {
>        dev->serial = g_strdup(s->drive_serial_str);
>    }
>
> Note it leaves a null dev->model null.
>
>>> Using optional that way can occasionally lead to trouble.  Consider
>>> migrate-set-parameters.  Its arguments are all optional.  For each
>>> argument present, the respective migration parameter is set to the
>>> argument value.  You cannot use this to reset a migration parameter from
>>> present to absent.  Matters for parameters where "absent" has a meaning
>>> different from any "present" value.
>>>
>>> End of digression.
>>>
>>> Start of next digression :)
>>>
>>> Note that qdev properties are generally optional.  The only way to make
>>> them mandatory is to reject their default value in .realize().  When
>>> users set this default value explicitly, the error message will almost
>>> certainly be confusing.
>>>
>>> End of digression.
>>>
>>> Optional bool may enable a fourth solution:
>>>
>>> 4. Make "absent" mean on if possible, else off, "present and true" mean
>>>    on if possible, else error, and "present and false" mean off (always
>>>    possible).
>>>
>>>    This changes the meaning of "present and true", but it's precisely
>>>    the change you want, isn't it?
>>
>> We have "false by default" properties so it unfortunately does not work.
>
> Then make the code make "absent" mean what you need it to mean.  Just
> like the code from ide_dev_initfn() I quoted above.

This would change the value for false as 0 is now taken to mean auto so 
either all places that assume false need to be updated or if we use 
different value for auto then all places need to explicitly set that and 
you can't easily change a bool into OnOffAuto.

What is the core of the issue that you don't like about allowing on/off 
for bool? If it's just some cases are not covered and still would not 
accept alternative names would it be easier to fix those?

Regards,
BALATON Zoltan

>>> Yet another solutions:
>>>
>>> 5. Alternate of bool and an enum with a single value "auto".
>>>
>>>     Falls apart with the keyval visitor used for the command line.
>>>     Fixable, I believe, but a good chunk of work and complexity.
>>
>> I may have missed something, but I think that will break JSON string
>> literals "on" and "off".
>
> Unbreaking it will be a good chunk of work and complexity, I believe.
>
>>> My gut feeling: explore 4. first.
>
>
>

Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2025/05/07 0:37, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/02/06 18:48, Markus Armbruster wrote:
>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
> [...]
> 
>>> I understand we have something like this:
>>>
>>> * true: on if possible, else off
>>>
>>> * false: off (always possible)
>>>
>>> Which one is the default?
>>
>> It depends. Some properties have true by default. The others have false.
>>
>>>
>>> There is no way to reliably configure "on", i.e. fail if it's not
>>> possible.  I agree that's a problem.
>>>
>>>>                                               This problem can be solved
>>>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>>>> state and explicit the "on" state.
>>>
>>> I guess you're proposing something like this:
>>>
>>> * auto: on if possible, else off
>>>
>>> * on: on if possible, else error
>>>
>>> * off: off (always possible)
>>>
>>> Which one is the default?
>>
>> I converted on to auto and off to false in a following patch.
>>
>>>
>>>> However, converting bool to OnOffAuto surfaces another problem: they
>>>> disagree how "on" and "off" should be written. Please note that the
>>>> disagreement already exists and so it is nice to solve anyway.
>>>
>>> Yes, converting bool to OnOffAuto is an incompatible change.
>>
>> Not just about conversion, but this inconsistency require users to know
>> whether a property is bool or OnOffAuto and change how the values are
>> written in JSON accordingly. This somewhat hurts usability.
>>
>>>
>>>> This patch tries to solve it by tolerating bool values for OnOffAuto. As
>>>> you pointed out, this approach has a downside: it makes OnOffAuto more
>>>> complicated by having multiple ways to express the same thing.
>>>
>>> It also affects existing uses of OnOffAuto, where such a change is
>>> unnecessary and undesirable.
> 
> To be clear: this is pretty much a deal-breaker for me.
> 
> We established above that you need certain boolean properties to take a
> third state.  I'm willing to discuss patches that change exactly these
> properties.  I'm going to reject patches that affect properties that do
> not need such a change.

I also want to change the existing OnOffAuto properties while I do want 
to change certain boolean properties as you read.

For my reasoning to change the existing OnOffAuto properties, please 
refer to:
https://lore.kernel.org/qemu-devel/d166d6c2-2b52-4c70-8fcf-a12f34a2347e@daynix.com/