[RFC PATCH 0/5] qdev-properties: Try to improve use of dynamic property introspection

[RFC PATCH 0/5] qdev-properties: Try to improve use of dynamic property introspection

[Philippe Mathieu-Daudé]

Hi,

This RFC series tries to work over some limitations exposed in
https://lore.kernel.org/qemu-devel/9293e63b-8032-4ea0-b516-9db6949fb607@linaro.org/

Eventually all QDev objects would use static QOM properties,
but in some cases we can not. ARMv7MState is a such example
adding properties that might end up irrelevant.

This is just an example, but thinking long term (in particular
in the context of dynamic machines) I'm looking at how this
could be improved. Thus this series. I don't like much this
current approach (because more boiler place and complexity)
however this seems to DTRT for the user.

Philippe Mathieu-Daudé (5):
  qdev-properties: Add qdev_property_del_static()
  qdev-properties: Add OptionalBool QAPI type
  hw/arm/armv7m: Convert ARMv7MState::vfp from bool to OptionalBool
  hw/arm/armv7m: Error if trying to set unavailable ARMCPU::vfp property
  hw/arm/armv7m: Do not expose 'vfp' property if ARM CPU doesn't have it

 qapi/common.json             | 16 ++++++++++++++++
 include/hw/arm/armv7m.h      |  2 +-
 include/hw/qdev-properties.h |  7 +++++++
 hw/arm/armsse.c              |  2 +-
 hw/arm/armv7m.c              | 12 +++++++++++-
 hw/core/qdev-properties.c    | 17 +++++++++++++++++
 6 files changed, 53 insertions(+), 3 deletions(-)

-- 
2.41.0

[RFC PATCH 1/5] qdev-properties: Add qdev_property_del_static()

[Philippe Mathieu-Daudé]

We can add properties with qdev_property_add_static().
Add qdev_property_del_static() to delete them.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/qdev-properties.h | 2 ++
 hw/core/qdev-properties.c    | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 09aa04ca1e..0e1930177e 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -225,6 +225,8 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
  */
 void qdev_property_add_static(DeviceState *dev, Property *prop);
 
+void qdev_property_del_static(DeviceState *dev, Property *prop);
+
 /**
  * qdev_alias_all_properties: Create aliases on source for all target properties
  * @target: Device which has properties to be aliased
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 7d6fa726fd..0c17a5de82 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -994,6 +994,13 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
     }
 }
 
+void qdev_property_del_static(DeviceState *dev, Property *prop)
+{
+    Object *obj = OBJECT(dev);
+
+    object_property_del(obj, prop->name);
+}
+
 static void qdev_class_add_property(DeviceClass *klass, const char *name,
                                     Property *prop)
 {
-- 
2.41.0

[RFC PATCH 2/5] qdev-properties: Add OptionalBool QAPI type

[Philippe Mathieu-Daudé]

To be able to distinct whether a boolean qdev property
has been set or not, add the DEFINE_PROP_BOOL_NODEFAULT()
qdev macro based on the tri-state OptionalBool QAPI type.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/common.json             | 16 ++++++++++++++++
 include/hw/qdev-properties.h |  5 +++++
 hw/core/qdev-properties.c    | 10 ++++++++++
 3 files changed, 31 insertions(+)

diff --git a/qapi/common.json b/qapi/common.json
index 6fed9cde1a..884c143e2a 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -207,3 +207,19 @@
 ##
 { 'struct': 'HumanReadableText',
   'data': { 'human-readable-text': 'str' } }
+
+##
+# @OptionalBool:
+#
+# An enumeration of three options: true, false, and unset
+#
+# @unset: Unset (default)
+#
+# @false: False
+#
+# @true: True
+#
+# Since: 9.0
+##
+{ 'enum': 'OptionalBool',
+  'data': [ 'false', 'true', 'unset' ] }
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0e1930177e..8cf95da2c3 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -49,6 +49,7 @@ struct PropertyInfo {
 extern const PropertyInfo qdev_prop_bit;
 extern const PropertyInfo qdev_prop_bit64;
 extern const PropertyInfo qdev_prop_bool;
+extern const PropertyInfo qdev_prop_bool_unset;
 extern const PropertyInfo qdev_prop_enum;
 extern const PropertyInfo qdev_prop_uint8;
 extern const PropertyInfo qdev_prop_uint16;
@@ -105,6 +106,10 @@ extern const PropertyInfo qdev_prop_link;
                 .set_default = true,                         \
                 .defval.u    = (bool)_defval)
 
+#define DEFINE_PROP_BOOL_NODEFAULT(_name, _state, _field) \
+    DEFINE_PROP_SIGNED(_name, _state, _field, OPTIONAL_BOOL_UNSET, \
+                        qdev_prop_bool_unset, OptionalBool)
+
 /**
  * The DEFINE_PROP_UINT64_CHECKMASK macro checks a user-supplied value
  * against corresponding bitmask, rejects the value if it violates.
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 0c17a5de82..1bec8ee679 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -260,6 +260,16 @@ const PropertyInfo qdev_prop_bool = {
     .set_default_value = set_default_value_bool,
 };
 
+/* --- optional bool --- */
+
+const PropertyInfo qdev_prop_bool_unset = {
+    .name  = "OptionalBool",
+    .enum_table  = &OptionalBool_lookup,
+    .get   = qdev_propinfo_get_enum,
+    .set   = qdev_propinfo_set_enum,
+    .set_default_value = qdev_propinfo_set_default_value_enum,
+};
+
 /* --- 8bit integer --- */
 
 static void get_uint8(Object *obj, Visitor *v, const char *name, void *opaque,
-- 
2.41.0

Re: [RFC PATCH 2/5] qdev-properties: Add OptionalBool QAPI type

[Richard Henderson]

On 1/3/24 03:04, Philippe Mathieu-Daudé wrote:
> To be able to distinct whether a boolean qdev property
> has been set or not, add the DEFINE_PROP_BOOL_NODEFAULT()
> qdev macro based on the tri-state OptionalBool QAPI type.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   qapi/common.json             | 16 ++++++++++++++++
>   include/hw/qdev-properties.h |  5 +++++
>   hw/core/qdev-properties.c    | 10 ++++++++++
>   3 files changed, 31 insertions(+)

How is this different from OnOffAuto?


r~

Re: [RFC PATCH 2/5] qdev-properties: Add OptionalBool QAPI type

[Philippe Mathieu-Daudé]

On 2/1/24 23:44, Richard Henderson wrote:
> On 1/3/24 03:04, Philippe Mathieu-Daudé wrote:
>> To be able to distinct whether a boolean qdev property
>> has been set or not, add the DEFINE_PROP_BOOL_NODEFAULT()
>> qdev macro based on the tri-state OptionalBool QAPI type.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   qapi/common.json             | 16 ++++++++++++++++
>>   include/hw/qdev-properties.h |  5 +++++
>>   hw/core/qdev-properties.c    | 10 ++++++++++
>>   3 files changed, 31 insertions(+)
> 
> How is this different from OnOffAuto?

I am trying to not break CLI which expects true/false and not on/off.

We could extend OnOffAuto to parse true/false if preferred.

(The particular device used as example in this series is not - yet -
user-creatable, so this doesn't matter there).

[RFC PATCH 3/5] hw/arm/armv7m: Convert ARMv7MState::vfp from bool to OptionalBool

[Philippe Mathieu-Daudé]

We want to know if the 'vfp' property has been initialized.
Convert it from boolean to OptionalBool (which contain the
'unset' enum).

Note the monitor output is changed as:

  (qemu) info qtree
    ...
    dev: armv7m, id ""
      gpio-in "NMI" 1
      gpio-out "SYSRESETREQ" 1
      gpio-in "" 96
      clock-in "cpuclk" freq_hz=40 MHz
      clock-in "refclk" freq_hz=0 Hz
      cpu-type = "cortex-m33-arm-cpu"
      init-svtor = 270532608 (0x10200000)
      init-nsvtor = 0 (0x0)
      enable-bitband = false
      start-powered-off = false
-     vfp = false
+     vfp = "false"
      dsp = false
      mpu-ns-regions = 8 (0x8)
      mpu-s-regions = 8 (0x8)

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/arm/armv7m.h | 2 +-
 hw/arm/armsse.c         | 2 +-
 hw/arm/armv7m.c         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index e2cebbd15c..6c9e65b644 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -105,7 +105,7 @@ struct ARMv7MState {
     uint32_t mpu_s_regions;
     bool enable_bitband;
     bool start_powered_off;
-    bool vfp;
+    OptionalBool vfp;
     bool dsp;
 };
 
diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 31acbf7347..472b16fc30 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -1028,7 +1028,7 @@ static void armsse_realize(DeviceState *dev, Error **errp)
             }
         }
         if (!s->cpu_fpu[i]) {
-            if (!object_property_set_bool(cpuobj, "vfp", false, errp)) {
+            if (!object_property_set_str(cpuobj, "vfp", "false", errp)) {
                 return;
             }
         }
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index d10abb36a8..3610f6f4a1 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -548,7 +548,7 @@ static Property armv7m_properties[] = {
     DEFINE_PROP_BOOL("enable-bitband", ARMv7MState, enable_bitband, false),
     DEFINE_PROP_BOOL("start-powered-off", ARMv7MState, start_powered_off,
                      false),
-    DEFINE_PROP_BOOL("vfp", ARMv7MState, vfp, true),
+    DEFINE_PROP_BOOL_NODEFAULT("vfp", ARMv7MState, vfp),
     DEFINE_PROP_BOOL("dsp", ARMv7MState, dsp, true),
     DEFINE_PROP_UINT32("mpu-ns-regions", ARMv7MState, mpu_ns_regions, UINT_MAX),
     DEFINE_PROP_UINT32("mpu-s-regions", ARMv7MState, mpu_s_regions, UINT_MAX),
-- 
2.41.0

[RFC PATCH 4/5] hw/arm/armv7m: Error if trying to set unavailable ARMCPU::vfp property

[Philippe Mathieu-Daudé]

Do not ignore impossible configuration requested by the user.
For example, when trying to enable VFP on a Cortex-M33 we now get:

  qemu-system-arm: 'cortex-m33-arm-cpu' does not support VFP

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armv7m.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 3610f6f4a1..12cdad09f9 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -328,6 +328,9 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
         if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) {
             return;
         }
+    } else if (s->vfp == OPTIONAL_BOOL_TRUE) {
+        error_setg(errp, "'%s' does not support VFP", s->cpu_type);
+        return;
     }
     if (object_property_find(OBJECT(s->cpu), "dsp")) {
         if (!object_property_set_bool(OBJECT(s->cpu), "dsp", s->dsp, errp)) {
-- 
2.41.0

Re: [RFC PATCH 4/5] hw/arm/armv7m: Error if trying to set unavailable ARMCPU::vfp property

[Peter Maydell]

On Tue, 2 Jan 2024 at 16:05, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Do not ignore impossible configuration requested by the user.
> For example, when trying to enable VFP on a Cortex-M33 we now get:
>
>   qemu-system-arm: 'cortex-m33-arm-cpu' does not support VFP
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/armv7m.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 3610f6f4a1..12cdad09f9 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -328,6 +328,9 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
>          if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) {
>              return;
>          }
> +    } else if (s->vfp == OPTIONAL_BOOL_TRUE) {
> +        error_setg(errp, "'%s' does not support VFP", s->cpu_type);
> +        return;
>      }

I'm not sure exactly what this series is trying to do, but
this isn't the right error message, at least at the moment.
Our Cortex-M33 model does support VFP -- in fact, there's
currently no way to turn it off, since we only expose the vfp
property for AArch64 CPUs.

I think we broke this in commit 4315f7c61474 last year,
accidentally restricting the definition of the "vfp"
property to ARM_FEATURE_AARCH64 CPUs only.
(filed https://gitlab.com/qemu-project/qemu/-/issues/2098
to track that)

I suppose if we fixed that regression then the error message
would become correct again, since we'd be back to exposing
the 'vfp' property only if the CPU did support VFP.

-- PMM

[RFC PATCH 5/5] hw/arm/armv7m: Do not expose 'vfp' property if ARM CPU doesn't have it

[Philippe Mathieu-Daudé]

Since the TYPE_ARMV7M object doesn't know its CPU type at the
time armv7m_instance_init() is called, we need to prepare to
forward any CPU properties there, then we can forward them in
armv7m_realize().

But then when introspecting at runtime, in the case the requested
CPU doesn't expose such properties, we can still see them exposed
in the container.

It is possible to remove an unmeaningful property with
qdev_property_del_static(). As an example, remove the 'vfp'
property when not relevant.

When running the musca-a board, the monitor output changes as:

  (qemu) info qtree
    ...
    dev: armv7m, id ""
      gpio-in "NMI" 1
      gpio-out "SYSRESETREQ" 1
      gpio-in "" 96
      clock-in "cpuclk" freq_hz=40 MHz
      clock-in "refclk" freq_hz=0 Hz
      cpu-type = "cortex-m33-arm-cpu"
      init-svtor = 270532608 (0x10200000)
      init-nsvtor = 0 (0x0)
      enable-bitband = false
      start-powered-off = false
-     vfp = "false"
      dsp = false
      mpu-ns-regions = 8 (0x8)
      mpu-s-regions = 8 (0x8)

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armv7m.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 12cdad09f9..f1f40353cb 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -244,6 +244,9 @@ static const MemoryRegionOps ppb_default_ops = {
     .valid.max_access_size = 8,
 };
 
+static Property optional_arm_cpu_vfp_property =
+    DEFINE_PROP_BOOL_NODEFAULT("vfp", ARMv7MState, vfp);
+
 static void armv7m_instance_init(Object *obj)
 {
     ARMv7MState *s = ARMV7M(obj);
@@ -271,6 +274,9 @@ static void armv7m_instance_init(Object *obj)
 
     s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk", NULL, NULL, 0);
     s->cpuclk = qdev_init_clock_in(DEVICE(obj), "cpuclk", NULL, NULL, 0);
+
+    qdev_property_add_static(DEVICE(obj),
+                             &optional_arm_cpu_vfp_property);
 }
 
 static void armv7m_realize(DeviceState *dev, Error **errp)
@@ -331,6 +337,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     } else if (s->vfp == OPTIONAL_BOOL_TRUE) {
         error_setg(errp, "'%s' does not support VFP", s->cpu_type);
         return;
+    } else {
+        qdev_property_del_static(dev, &optional_arm_cpu_vfp_property);
     }
     if (object_property_find(OBJECT(s->cpu), "dsp")) {
         if (!object_property_set_bool(OBJECT(s->cpu), "dsp", s->dsp, errp)) {
@@ -551,7 +559,6 @@ static Property armv7m_properties[] = {
     DEFINE_PROP_BOOL("enable-bitband", ARMv7MState, enable_bitband, false),
     DEFINE_PROP_BOOL("start-powered-off", ARMv7MState, start_powered_off,
                      false),
-    DEFINE_PROP_BOOL_NODEFAULT("vfp", ARMv7MState, vfp),
     DEFINE_PROP_BOOL("dsp", ARMv7MState, dsp, true),
     DEFINE_PROP_UINT32("mpu-ns-regions", ARMv7MState, mpu_ns_regions, UINT_MAX),
     DEFINE_PROP_UINT32("mpu-s-regions", ARMv7MState, mpu_s_regions, UINT_MAX),
-- 
2.41.0

Re: [RFC PATCH 0/5] qdev-properties: Try to improve use of dynamic property introspection

[Kevin Wolf]

Am 02.01.2024 um 17:04 hat Philippe Mathieu-Daudé geschrieben:
> Hi,
> 
> This RFC series tries to work over some limitations exposed in
> https://lore.kernel.org/qemu-devel/9293e63b-8032-4ea0-b516-9db6949fb607@linaro.org/
> 
> Eventually all QDev objects would use static QOM properties,
> but in some cases we can not. ARMv7MState is a such example
> adding properties that might end up irrelevant.
> 
> This is just an example, but thinking long term (in particular
> in the context of dynamic machines) I'm looking at how this
> could be improved. Thus this series. I don't like much this
> current approach (because more boiler place and complexity)
> however this seems to DTRT for the user.

This doesn't feel like the right approach to me. If you were to describe
the properties of armv7m in the QAPI schema language without looking at
the implementation, you would never add something like OptionalBool.

I'm not entirely sure if I understand the details of the problem, so if
I make wrong assumptions below, please correct me.

I think what you would get instead is a union with the different CPU
types as union variants. You would have few fields in the base type
(cpu-type, memory, etc.), and everything specific to the variant, you
wouldn't even want to look at in armv7m, but just forward it to the CPU.

Now of course actually getting this QAPIfied is not something to expect
in the next few days, so let's get back to the QOM level, but keep the
general idea in mind.

Based on the above, I'd argue that armv7m shouldn't even have the
properties that it only uses to forward them. Instead, we should let the
CPU grab its properties directly from the configuration. The way we
create objects currently is not really designed for this. But let's get
back to the QOM/QAPI integration patches [1] I sent two years ago and
suddently it becomes quite easy: armv7m's .instance_config simply calls
the CPU's .instance_config and passes its visitor. Then the CPU takes
the options it needs for its properties and that's it.

I discussed this series with Markus recently, and I actually assume that
he at least mentioned it to you when you discussed things with him. I
believe the part that you would need here (only up to patch 4 really) is
pretty much uncontroversial.

Of course, I ignored several "details" here, but I think the idea should
still be workable. Some of those details:

* This approach requires that the CPUs are first converted to have
  static properties, but given that making everything static is your
  goal, I assume that's not a problem.

* The CPU must actually exist when you want to set properties for it.
  This probably means moving creating the CPU from realize to the new
  instance_config.

* My patches are for object-add, but we're coming from board code here.
  So the callers would have to be changed to call .instance_config
  (probalby indirectly through object_configure()) instead of setting
  individual properties.

All of this is additional work, but it doesn't look like exceedingly
hard work.

Kevin

[1] https://patchew.org/QEMU/20211103173002.209906-1-kwolf@redhat.com/