[PATCH v2 0/3] virtio: Convert feature properties to OnOffAuto

[PATCH v2 0/3] 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 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 (3):
      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 ++-
 6 files changed, 138 insertions(+), 23 deletions(-)
---
base-commit: 7e3b6d8063f245d27eecce5aabe624b5785f2a77
change-id: 20241013-virtio-164ea3f295c3

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

[PATCH v2 1/3] 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 86a583574dd0..f0a270bb4f61 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.0

[PATCH v2 2/3] 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 09aa04ca1e27..afec53a48470 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 f0a270bb4f61..cc76ff0dfae6 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(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.0

[PATCH v2 3/3] 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 f526ecc8fcc0..b4cb5642ddd0 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;
@@ -374,22 +377,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 adaba17ebac1..3021d42fca12 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,7 +34,9 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "audio/audio.h"
 
-GlobalProperty hw_compat_9_1[] = {};
+GlobalProperty hw_compat_9_1[] = {
+    { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
+};
 const size_t hw_compat_9_1_len = G_N_ELEMENTS(hw_compat_9_1);
 
 GlobalProperty hw_compat_9_0[] = {
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 a26f18908ea5..44ad58de33c8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4006,11 +4006,13 @@ static void virtio_device_instance_finalize(Object *obj)
 }
 
 static 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),
     DEFINE_PROP_END_OF_LIST(),
 };
 

-- 
2.47.0

Re: [PATCH v2 1/3] qdev-properties: Accept bool for OnOffAuto

[Daniel P. Berrangé]

The parent msg was sent off-list orignially, so below is a copy
of my feedback to that off-list posting.

On Tue, Oct 22, 2024 at 01:50:38PM +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.
> 
> 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 86a583574dd0..f0a270bb4f61 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);
> +}

My feedback is the same as last time this was posted.

This is adding redundant new input-only & secret syntax for every
usage of OnOffAuto across QEMU.

"consistency with bool" isn't a expressing a compelling advantage.

The new permitted values are invisible to applications, beacuse
introspecting QAPI schema for the "OnOffAuto" type will never
report them, and querying the value of a property will also never
report them.

I'm not seeing an advantage, or clear problem solved, by adding
this.

> +
>  /* --- 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.0
> 

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 v2 2/3] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()

[Daniel P. Berrangé]

On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote:
> 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.

Again, same feedback as last time.

With this design, existing users of DEFINE_PROP_BIT64 that
get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition
to gaining the desired "auto" value, also gain redundant
'on' and 'off' values as side-effects.

In the next patch, the stated problem is that virtio code
needs to distinguish between bits that are user set, and
bits that are set based on available host backend features.

That doesn't seem to require us to accept any new values
from the user. It should be sufficient to track user
specified features, separately from user specified values.
ie when parsing user input for bitfields, we need to parse
into a pair of fields

  uint64_t has_user_features;  /* which bits were specified */
  uint64_t user_features;      /* values of specified bits*/

> 
> 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 09aa04ca1e27..afec53a48470 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 f0a270bb4f61..cc76ff0dfae6 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(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.0
> 
> 

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 v2 1/3] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2024/10/29 1:49, Daniel P. Berrangé wrote:
> The parent msg was sent off-list orignially, so below is a copy
> of my feedback to that off-list posting.
> 
> On Tue, Oct 22, 2024 at 01:50:38PM +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.
>>
>> 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 86a583574dd0..f0a270bb4f61 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);
>> +}
> 
> My feedback is the same as last time this was posted.
> 
> This is adding redundant new input-only & secret syntax for every
> usage of OnOffAuto across QEMU.
> 
> "consistency with bool" isn't a expressing a compelling advantage.
> 
> The new permitted values are invisible to applications, beacuse
> introspecting QAPI schema for the "OnOffAuto" type will never
> report them, and querying the value of a property will also never
> report them.
> 
> I'm not seeing an advantage, or clear problem solved, by adding
> this.

The intent of this patch is to ease migration from bool to OnOffAuto; a 
user should be able to set the "on" or "off" value without knowing the 
"auto" value is accepted.

The redundancy of syntax is already present with bool. If it is 
problematic, the redundant syntax should be deprecated altogether, 
whether the type is OnOffAuto or bool.

We can add a alternate type of OnOffAuto and bool to the QAPI schema, 
but this type is not used in QAPI and is unnecessary.

Regards,
Akihiko Odaki

> 
>> +
>>   /* --- 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.0
>>
> 
> With regards,
> Daniel

Re: [PATCH v2 2/3] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()

[Akihiko Odaki]

On 2024/10/29 1:50, Daniel P. Berrangé wrote:
> On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote:
>> 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.
> 
> Again, same feedback as last time.
> 
> With this design, existing users of DEFINE_PROP_BIT64 that
> get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition
> to gaining the desired "auto" value, also gain redundant
> 'on' and 'off' values as side-effects.
> 
> In the next patch, the stated problem is that virtio code
> needs to distinguish between bits that are user set, and
> bits that are set based on available host backend features.
> 
> That doesn't seem to require us to accept any new values
> from the user. It should be sufficient to track user
> specified features, separately from user specified values.
> ie when parsing user input for bitfields, we need to parse
> into a pair of fields
> 
>    uint64_t has_user_features;  /* which bits were specified */
>    uint64_t user_features;      /* values of specified bits*/

Properties also have getters. We don't know what to return with them 
without the new value.

Regards,
Akihiko Odaki

> 
>>
>> 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 09aa04ca1e27..afec53a48470 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 f0a270bb4f61..cc76ff0dfae6 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(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.0
>>
>>
> 
> With regards,
> Daniel

Re: [PATCH v2 1/3] qdev-properties: Accept bool for OnOffAuto

[Daniel P. Berrangé]

On Thu, Oct 31, 2024 at 04:21:08PM +0900, Akihiko Odaki wrote:
> On 2024/10/29 1:49, Daniel P. Berrangé wrote:
> > The parent msg was sent off-list orignially, so below is a copy
> > of my feedback to that off-list posting.
> > 
> > On Tue, Oct 22, 2024 at 01:50:38PM +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.
> > > 
> > > 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 86a583574dd0..f0a270bb4f61 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);
> > > +}
> > 
> > My feedback is the same as last time this was posted.
> > 
> > This is adding redundant new input-only & secret syntax for every
> > usage of OnOffAuto across QEMU.
> > 
> > "consistency with bool" isn't a expressing a compelling advantage.
> > 
> > The new permitted values are invisible to applications, beacuse
> > introspecting QAPI schema for the "OnOffAuto" type will never
> > report them, and querying the value of a property will also never
> > report them.
> > 
> > I'm not seeing an advantage, or clear problem solved, by adding
> > this.
> 
> The intent of this patch is to ease migration from bool to OnOffAuto; a user
> should be able to set the "on" or "off" value without knowing the "auto"
> value is accepted.
> 
> The redundancy of syntax is already present with bool. If it is problematic,
> the redundant syntax should be deprecated altogether, whether the type is
> OnOffAuto or bool.

The redundant syntax for bool is only present in the legacy QemuOpts
based CLI syntax. If using modern JSON syntax, or QMP, it is required
to use the navtive JSON bool type.

This proposed change to OnOffAuto is affecting both legacy and modern
syntax, adding redundancy to both, here none currently exists for the
latter. So this is qualatively different from the status quo, and
taking us in a direction we don't want to go.

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 v2 2/3] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()

[Daniel P. Berrangé]

On Thu, Oct 31, 2024 at 04:21:53PM +0900, Akihiko Odaki wrote:
> On 2024/10/29 1:50, Daniel P. Berrangé wrote:
> > On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote:
> > > 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.
> > 
> > Again, same feedback as last time.
> > 
> > With this design, existing users of DEFINE_PROP_BIT64 that
> > get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition
> > to gaining the desired "auto" value, also gain redundant
> > 'on' and 'off' values as side-effects.
> > 
> > In the next patch, the stated problem is that virtio code
> > needs to distinguish between bits that are user set, and
> > bits that are set based on available host backend features.
> > 
> > That doesn't seem to require us to accept any new values
> > from the user. It should be sufficient to track user
> > specified features, separately from user specified values.
> > ie when parsing user input for bitfields, we need to parse
> > into a pair of fields
> > 
> >    uint64_t has_user_features;  /* which bits were specified */
> >    uint64_t user_features;      /* values of specified bits*/
> 
> Properties also have getters. We don't know what to return with them without
> the new value.

The virtio changes in the next patch are just accessing the bitsets
directly. A getter could just be made to return false for unset
values, on the assumption that any caller should be checking the
has_user_features bits beforehand.

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 v2 1/3] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki]

On 2024/11/01 20:41, Daniel P. Berrangé wrote:
> On Thu, Oct 31, 2024 at 04:21:08PM +0900, Akihiko Odaki wrote:
>> On 2024/10/29 1:49, Daniel P. Berrangé wrote:
>>> The parent msg was sent off-list orignially, so below is a copy
>>> of my feedback to that off-list posting.
>>>
>>> On Tue, Oct 22, 2024 at 01:50:38PM +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.
>>>>
>>>> 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 86a583574dd0..f0a270bb4f61 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);
>>>> +}
>>>
>>> My feedback is the same as last time this was posted.
>>>
>>> This is adding redundant new input-only & secret syntax for every
>>> usage of OnOffAuto across QEMU.
>>>
>>> "consistency with bool" isn't a expressing a compelling advantage.
>>>
>>> The new permitted values are invisible to applications, beacuse
>>> introspecting QAPI schema for the "OnOffAuto" type will never
>>> report them, and querying the value of a property will also never
>>> report them.
>>>
>>> I'm not seeing an advantage, or clear problem solved, by adding
>>> this.
>>
>> The intent of this patch is to ease migration from bool to OnOffAuto; a user
>> should be able to set the "on" or "off" value without knowing the "auto"
>> value is accepted.
>>
>> The redundancy of syntax is already present with bool. If it is problematic,
>> the redundant syntax should be deprecated altogether, whether the type is
>> OnOffAuto or bool.
> 
> The redundant syntax for bool is only present in the legacy QemuOpts
> based CLI syntax. If using modern JSON syntax, or QMP, it is required
> to use the navtive JSON bool type.


My understanding of the present situation is different. OnOffAuto always 
requires to specify string "on" or "off", not a JSON bool type.

I see the core problem is that QOM bool and OnOffAuto disagrees how "on" 
and "off" should be specified, especially in the JSON syntax. In JSON 
syntax, QOM bool requires JSON bool, which is inconsistent with OnOffAuto.

If the added redundancy is not OK, I think we need to decide how "on" 
and "off" should be decided, and use the syntax for both of QOM bool and 
OnOffAuto while deprecating the old syntax.

I don't have a strong opinion regarding the JSON syntax that should be 
used for "on" or "off". While I feel the JSON type system fits poor for 
QEMU, QEMU already have code that handles JSON numbers and null so using 
JSON true/false should be OK.

Regards,
Akihiko Odaki

> 
> This proposed change to OnOffAuto is affecting both legacy and modern
> syntax, adding redundancy to both, here none currently exists for the
> latter. So this is qualatively different from the status quo, and
> taking us in a direction we don't want to go.
 > > With regards,
> Daniel

Re: [PATCH v2 2/3] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()

[Akihiko Odaki]

On 2024/11/01 20:44, Daniel P. Berrangé wrote:
> On Thu, Oct 31, 2024 at 04:21:53PM +0900, Akihiko Odaki wrote:
>> On 2024/10/29 1:50, Daniel P. Berrangé wrote:
>>> On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote:
>>>> 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.
>>>
>>> Again, same feedback as last time.
>>>
>>> With this design, existing users of DEFINE_PROP_BIT64 that
>>> get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition
>>> to gaining the desired "auto" value, also gain redundant
>>> 'on' and 'off' values as side-effects.
>>>
>>> In the next patch, the stated problem is that virtio code
>>> needs to distinguish between bits that are user set, and
>>> bits that are set based on available host backend features.
>>>
>>> That doesn't seem to require us to accept any new values
>>> from the user. It should be sufficient to track user
>>> specified features, separately from user specified values.
>>> ie when parsing user input for bitfields, we need to parse
>>> into a pair of fields
>>>
>>>     uint64_t has_user_features;  /* which bits were specified */
>>>     uint64_t user_features;      /* values of specified bits*/
>>
>> Properties also have getters. We don't know what to return with them without
>> the new value.
> 
> The virtio changes in the next patch are just accessing the bitsets
> directly. A getter could just be made to return false for unset
> values, on the assumption that any caller should be checking the
> has_user_features bits beforehand.

It means this construct is specialized for the case when the getter will 
never be called for the default value. I think it is difficult to 
convince others to introduce such a new mechanism when we already have 
OnOffAuto as a generic solution. There is even DEFINE_PROP_ON_OFF_AUTO() 
for qdev properties.

The question is: when we have BOOL, BIT64, and ON_OFF_AUTO, shouldn't we 
have ON_OFF_AUTO_BIT64? Whether OnOffAuto is good or not is not a 
problem here unless we are going to deprecate DEFINE_PROP_ON_OFF_AUTO().

Regards,
Akihiko Odaki