[PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[Philippe Mathieu-Daudé]

Since v2 [2]:
- Dropped "Simplify checking A64_MTE bit in FEATURE_ID register"
- Correct object_property_get_bool() uses
- Update ARM_FEATURE_AARCH64 && aa64_mte

Since RFC [1]:
- Split one patch per feature
- Addressed Peter's review comments

[1] https://lore.kernel.org/qemu-devel/20231214171447.44025-1-philmd@linaro.org/
[2] https://lore.kernel.org/qemu-devel/20240109180930.90793-1-philmd@linaro.org/

Based-on: <20231123143813.42632-1-philmd@linaro.org>
  "hw: Simplify accesses to CPUState::'start-powered-off' property"

Philippe Mathieu-Daudé (14):
  hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()
  hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M
  hw/arm/armv7m: Move code setting 'start-powered-off' property around
  hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs
  hw/arm: Prefer arm_feature(M_SECURITY) over object_property_find()
  hw/arm: Prefer arm_feature(THUMB_DSP) over object_property_find(dsp)
  hw/arm: Prefer arm_feature(V7) over
    object_property_find(pmsav7-dregion)
  hw/arm: Prefer arm_feature(EL3) over object_property_find(has_el3)
  hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2)
  hw/arm: Prefer arm_feature(CBAR*) over
    object_property_find(reset-cbar)
  hw/arm: Prefer arm_feature(PMU) over object_property_find(pmu)
  hw/arm: Prefer arm_feature(GENERIC_TMR) over 'kvm-no-adjvtime'
    property
  hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)
  hw/arm: Prefer cpu_isar_feature(aa64_mte) over
    property_find(tag-memory)

 hw/arm/armv7m.c       | 36 ++++++++++++++++--------------------
 hw/arm/exynos4210.c   |  4 ++--
 hw/arm/highbank.c     |  3 ++-
 hw/arm/integratorcp.c |  5 ++---
 hw/arm/realview.c     |  2 +-
 hw/arm/sbsa-ref.c     |  3 ++-
 hw/arm/versatilepb.c  |  5 ++---
 hw/arm/vexpress.c     |  6 ++++--
 hw/arm/virt.c         | 26 +++++++++++++-------------
 hw/arm/xilinx_zynq.c  |  2 +-
 hw/cpu/a15mpcore.c    | 23 +++++++++++++++--------
 hw/cpu/a9mpcore.c     |  9 +++++----
 12 files changed, 65 insertions(+), 59 deletions(-)

-- 
2.41.0

[PATCH v3 01/14] hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()

[Philippe Mathieu-Daudé]

We are going to cast s->cpu as DeviceState multiple times.
Add a local 'cpudev' variable to simplify code review, having
a single DEVICE(s->cpu) conversion.

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

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 50c6c6b1f5..d239468558 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -277,6 +277,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
 {
     ARMv7MState *s = ARMV7M(dev);
     SysBusDevice *sbd;
+    DeviceState *cpudev;
     Error *err = NULL;
     int i;
 
@@ -299,6 +300,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
+    cpudev = DEVICE(s->cpu);
 
     object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container),
                              &error_abort);
@@ -356,7 +358,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     s->cpu->env.nvic = &s->nvic;
     s->nvic.cpu = s->cpu;
 
-    if (!qdev_realize(DEVICE(s->cpu), NULL, errp)) {
+    if (!qdev_realize(cpudev, NULL, errp)) {
         return;
     }
 
@@ -426,8 +428,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
 
     /* Wire the NVIC up to the CPU */
     sbd = SYS_BUS_DEVICE(&s->nvic);
-    sysbus_connect_irq(sbd, 0,
-                       qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));
+    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
 
     memory_region_add_subregion(&s->container, 0xe000e000,
                                 sysbus_mmio_get_region(sbd, 0));
-- 
2.41.0

[PATCH v3 02/14] hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M

[Philippe Mathieu-Daudé]

ARMV7M container can only accept M-profile CPU types.
Check requested type is valid once to allow further simplifications.

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

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index d239468558..8900730e53 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -300,6 +300,10 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
+    if (!arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
+        error_setg(errp, "armv7m: CPU must be of Cortex-M family");
+        return;
+    }
     cpudev = DEVICE(s->cpu);
 
     object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container),
-- 
2.41.0

[PATCH v3 03/14] hw/arm/armv7m: Move code setting 'start-powered-off' property around

[Philippe Mathieu-Daudé]

Reorganize a bit by first setting properties which are not
dependent of CPU features (and can not fail).

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

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 8900730e53..b752049add 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -308,6 +308,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
 
     object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container),
                              &error_abort);
+    qdev_prop_set_bit(cpudev, "start-powered-off", s->start_powered_off);
     if (object_property_find(OBJECT(s->cpu), "idau")) {
         object_property_set_link(OBJECT(s->cpu), "idau", s->idau,
                                  &error_abort);
@@ -334,7 +335,6 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
-    qdev_prop_set_bit(DEVICE(s->cpu), "start-powered-off", s->start_powered_off);
 
     /*
      * Real M-profile hardware can be configured with a different number of
-- 
2.41.0

[PATCH v3 04/14] hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs

[Philippe Mathieu-Daudé]

All CPUs implementing ARM_FEATURE_M have the 'init-nsvtor' property.
Since setting the property can not fail, replace

   object_property_set_uint(..., "init-nsvtor", ..., &error_abort);

by:
   qdev_prop_set_uint32(..., "init-nsvtor", ...).

which is a one-to-one replacement.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armv7m.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index b752049add..530729f42e 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -309,6 +309,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container),
                              &error_abort);
     qdev_prop_set_bit(cpudev, "start-powered-off", s->start_powered_off);
+    qdev_prop_set_uint32(cpudev, "init-nsvtor", s->init_nsvtor);
+
     if (object_property_find(OBJECT(s->cpu), "idau")) {
         object_property_set_link(OBJECT(s->cpu), "idau", s->idau,
                                  &error_abort);
@@ -319,12 +321,6 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
-    if (object_property_find(OBJECT(s->cpu), "init-nsvtor")) {
-        if (!object_property_set_uint(OBJECT(s->cpu), "init-nsvtor",
-                                      s->init_nsvtor, errp)) {
-            return;
-        }
-    }
     if (object_property_find(OBJECT(s->cpu), "vfp")) {
         if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) {
             return;
-- 
2.41.0

[PATCH v3 05/14] hw/arm: Prefer arm_feature(M_SECURITY) over object_property_find()

[Philippe Mathieu-Daudé]

Both "idau" and "init-svtor" properties are added to ARMCPU
when the ARM_FEATURE_M_SECURITY feature is available. Rather
than checking whether the QOM properties are present, directly
check the feature.

Since we are sure the "init-svtor" is present, the
object_property_set_uint() can't fail.
Instead of using &error_abort, replace:

  object_property_set_uint(OBJECT(s->cpu), "init-svtor",
                           s->init_svtor, &error_abort);
by:

  qdev_prop_set_uint32(cpudev, "init-svtor", s->init_svtor);

which is a one-to-one replacement.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armv7m.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 530729f42e..8350267d96 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -311,16 +311,11 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     qdev_prop_set_bit(cpudev, "start-powered-off", s->start_powered_off);
     qdev_prop_set_uint32(cpudev, "init-nsvtor", s->init_nsvtor);
 
-    if (object_property_find(OBJECT(s->cpu), "idau")) {
+    if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) {
+        qdev_prop_set_uint32(cpudev, "init-svtor", s->init_svtor);
         object_property_set_link(OBJECT(s->cpu), "idau", s->idau,
                                  &error_abort);
     }
-    if (object_property_find(OBJECT(s->cpu), "init-svtor")) {
-        if (!object_property_set_uint(OBJECT(s->cpu), "init-svtor",
-                                      s->init_svtor, errp)) {
-            return;
-        }
-    }
     if (object_property_find(OBJECT(s->cpu), "vfp")) {
         if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) {
             return;
-- 
2.41.0

[PATCH v3 06/14] hw/arm: Prefer arm_feature(THUMB_DSP) over object_property_find(dsp)

[Philippe Mathieu-Daudé]

The "dsp" property is added to ARMCPU when the ARM_FEATURE_THUMB_DSP
feature is available. Rather than checking whether the QOM property
is present, directly check the feature.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armv7m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 8350267d96..0a7ad2b762 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -321,7 +321,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
-    if (object_property_find(OBJECT(s->cpu), "dsp")) {
+    if (arm_feature(&s->cpu->env, ARM_FEATURE_THUMB_DSP)) {
         if (!object_property_set_bool(OBJECT(s->cpu), "dsp", s->dsp, errp)) {
             return;
         }
-- 
2.41.0

[PATCH v3 07/14] hw/arm: Prefer arm_feature(V7) over object_property_find(pmsav7-dregion)

[Philippe Mathieu-Daudé]

The "pmsav7-dregion" property is added to ARMCPU when the
ARM_FEATURE_V7 feature is available. Rather than checking
whether the QOM property is present, directly check the
feature.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armv7m.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 0a7ad2b762..7f15318ae3 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -338,8 +338,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
                    "mpu-ns-regions and mpu-s-regions properties must have the same value");
         return;
     }
-    if (s->mpu_ns_regions != UINT_MAX &&
-        object_property_find(OBJECT(s->cpu), "pmsav7-dregion")) {
+    if (s->mpu_ns_regions != UINT_MAX && arm_feature(&s->cpu->env,
+                                                     ARM_FEATURE_V7)) {
         if (!object_property_set_uint(OBJECT(s->cpu), "pmsav7-dregion",
                                       s->mpu_ns_regions, errp)) {
             return;
-- 
2.41.0

[PATCH v3 08/14] hw/arm: Prefer arm_feature(EL3) over object_property_find(has_el3)

[Philippe Mathieu-Daudé]

The "has_el3" property is added to ARMCPU when the
ARM_FEATURE_EL3 feature is available. Rather than
checking whether the QOM property is present, directly
check the feature.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/exynos4210.c   |  4 ++--
 hw/arm/integratorcp.c |  5 ++---
 hw/arm/realview.c     |  2 +-
 hw/arm/versatilepb.c  |  5 ++---
 hw/arm/xilinx_zynq.c  |  2 +-
 hw/cpu/a15mpcore.c    | 14 +++++++++-----
 hw/cpu/a9mpcore.c     |  9 +++++----
 7 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index de39fb0ece..5efaa538cd 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -554,14 +554,14 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
     for (n = 0; n < EXYNOS4210_NCPUS; n++) {
         Object *cpuobj = object_new(ARM_CPU_TYPE_NAME("cortex-a9"));
 
+        s->cpu[n] = ARM_CPU(cpuobj);
         /* By default A9 CPUs have EL3 enabled.  This board does not currently
          * support EL3 so the CPU EL3 property is disabled before realization.
          */
-        if (object_property_find(cpuobj, "has_el3")) {
+        if (arm_feature(&s->cpu[n]->env, ARM_FEATURE_EL3)) {
             object_property_set_bool(cpuobj, "has_el3", false, &error_fatal);
         }
 
-        s->cpu[n] = ARM_CPU(cpuobj);
         object_property_set_int(cpuobj, "mp-affinity",
                                 exynos4210_calc_affinity(n), &error_abort);
         object_property_set_int(cpuobj, "reset-cbar",
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 1830e1d785..7685527eb2 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -596,19 +596,18 @@ static void integratorcp_init(MachineState *machine)
     int i;
 
     cpuobj = object_new(machine->cpu_type);
+    cpu = ARM_CPU(cpuobj);
 
     /* By default ARM1176 CPUs have EL3 enabled.  This board does not
      * currently support EL3 so the CPU EL3 property is disabled before
      * realization.
      */
-    if (object_property_find(cpuobj, "has_el3")) {
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
         object_property_set_bool(cpuobj, "has_el3", false, &error_fatal);
     }
 
     qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
 
-    cpu = ARM_CPU(cpuobj);
-
     /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash.  */
     /* ??? RAM should repeat to fill physical memory space.  */
     /* SDRAM at address zero*/
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 132217b2ed..433fe72ced 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -123,7 +123,7 @@ static void realview_init(MachineState *machine,
          * does not currently support EL3 so the CPU EL3 property is disabled
          * before realization.
          */
-        if (object_property_find(cpuobj, "has_el3")) {
+        if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
             object_property_set_bool(cpuobj, "has_el3", false, &error_fatal);
         }
 
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 4b2257787b..1969bb4608 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -208,19 +208,18 @@ static void versatile_init(MachineState *machine, int board_id)
     }
 
     cpuobj = object_new(machine->cpu_type);
+    cpu = ARM_CPU(cpuobj);
 
     /* By default ARM1176 CPUs have EL3 enabled.  This board does not
      * currently support EL3 so the CPU EL3 property is disabled before
      * realization.
      */
-    if (object_property_find(cpuobj, "has_el3")) {
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
         object_property_set_bool(cpuobj, "has_el3", false, &error_fatal);
     }
 
     qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
 
-    cpu = ARM_CPU(cpuobj);
-
     /* ??? RAM should repeat to fill physical memory space.  */
     /* SDRAM at address zero.  */
     memory_region_add_subregion(sysmem, 0, machine->ram);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index dbb9793aa1..33e57dceef 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -198,7 +198,7 @@ static void zynq_init(MachineState *machine)
      * currently support EL3 so the CPU EL3 property is disabled before
      * realization.
      */
-    if (object_property_find(OBJECT(cpu), "has_el3")) {
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
         object_property_set_bool(OBJECT(cpu), "has_el3", false, &error_fatal);
     }
 
diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index bfd8aa5644..afe3897901 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -53,7 +53,6 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
     DeviceState *gicdev;
     SysBusDevice *busdev;
     int i;
-    bool has_el3;
     bool has_el2 = false;
     Object *cpuobj;
 
@@ -62,13 +61,18 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
 
     if (!kvm_irqchip_in_kernel()) {
+        CPUState *cpu;
+
         /* Make the GIC's TZ support match the CPUs. We assume that
          * either all the CPUs have TZ, or none do.
          */
-        cpuobj = OBJECT(qemu_get_cpu(0));
-        has_el3 = object_property_find(cpuobj, "has_el3") &&
-            object_property_get_bool(cpuobj, "has_el3", &error_abort);
-        qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
+        cpu = qemu_get_cpu(0);
+        cpuobj = OBJECT(cpu);
+        if (arm_feature(cpu_env(cpu), ARM_FEATURE_EL3)) {
+            qdev_prop_set_bit(gicdev, "has-security-extensions",
+                              object_property_get_bool(cpuobj, "has_el3",
+                                                       &error_abort));
+        }
         /* Similarly for virtualization support */
         has_el2 = object_property_find(cpuobj, "has_el2") &&
             object_property_get_bool(cpuobj, "has_el2", &error_abort);
diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index d03f57e579..3e1fef149f 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -52,7 +52,6 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     SysBusDevice *scubusdev, *gicbusdev, *gtimerbusdev, *mptimerbusdev,
                  *wdtbusdev;
     int i;
-    bool has_el3;
     CPUState *cpu0;
     Object *cpuobj;
 
@@ -81,9 +80,11 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     /* Make the GIC's TZ support match the CPUs. We assume that
      * either all the CPUs have TZ, or none do.
      */
-    has_el3 = object_property_find(cpuobj, "has_el3") &&
-        object_property_get_bool(cpuobj, "has_el3", &error_abort);
-    qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
+    if (arm_feature(cpu_env(cpu0), ARM_FEATURE_EL3)) {
+        qdev_prop_set_bit(gicdev, "has-security-extensions",
+                          object_property_get_bool(cpuobj, "has_el3",
+                                                   &error_abort));
+    }
 
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) {
         return;
-- 
2.41.0

[PATCH v3 09/14] hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2)

[Philippe Mathieu-Daudé]

The "has_el2" property is added to ARMCPU when the
ARM_FEATURE_EL2 feature is available. Rather than
checking whether the QOM property is present, directly
check the feature.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/vexpress.c  | 3 ++-
 hw/arm/virt.c      | 2 +-
 hw/cpu/a15mpcore.c | 9 ++++++---
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index fd981f4c33..753a645c05 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -218,12 +218,13 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
     /* Create the actual CPUs */
     for (n = 0; n < smp_cpus; n++) {
         Object *cpuobj = object_new(cpu_type);
+        ARMCPU *cpu = ARM_CPU(cpuobj);
 
         if (!secure) {
             object_property_set_bool(cpuobj, "has_el3", false, NULL);
         }
         if (!virt) {
-            if (object_property_find(cpuobj, "has_el2")) {
+            if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) {
                 object_property_set_bool(cpuobj, "has_el2", false, NULL);
             }
         }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2793121cb4..35eb01a3dc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2146,7 +2146,7 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "has_el3", false, NULL);
         }
 
-        if (!vms->virt && object_property_find(cpuobj, "has_el2")) {
+        if (!vms->virt &&  arm_feature(cpu_env(cs), ARM_FEATURE_EL2)) {
             object_property_set_bool(cpuobj, "has_el2", false, NULL);
         }
 
diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index afe3897901..7e456375e3 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -74,9 +74,12 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
                                                        &error_abort));
         }
         /* Similarly for virtualization support */
-        has_el2 = object_property_find(cpuobj, "has_el2") &&
-            object_property_get_bool(cpuobj, "has_el2", &error_abort);
-        qdev_prop_set_bit(gicdev, "has-virtualization-extensions", has_el2);
+        has_el2 = arm_feature(cpu_env(cpu), ARM_FEATURE_EL2);
+        if (has_el2) {
+            qdev_prop_set_bit(gicdev, "has-virtualization-extensions",
+                              object_property_get_bool(cpuobj, "has_el2",
+                                                       &error_abort));
+        }
     }
 
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) {
-- 
2.41.0

[PATCH v3 10/14] hw/arm: Prefer arm_feature(CBAR*) over object_property_find(reset-cbar)

[Philippe Mathieu-Daudé]

The "reset-cbar" property is added to ARMCPU when the
ARM_FEATURE_CBAR[_RO] features are available. Rather than
checking whether the QOM property is present, directly
check the features.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/highbank.c | 3 ++-
 hw/arm/sbsa-ref.c | 3 ++-
 hw/arm/vexpress.c | 3 ++-
 hw/arm/virt.c     | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index c21e18d08f..b06a727c06 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -211,7 +211,8 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         object_property_set_int(cpuobj, "psci-conduit", QEMU_PSCI_CONDUIT_SMC,
                                 &error_abort);
 
-        if (object_property_find(cpuobj, "reset-cbar")) {
+        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
+            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
             object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE,
                                     &error_abort);
         }
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 477dca0637..c073c462c7 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -770,7 +770,8 @@ static void sbsa_ref_init(MachineState *machine)
         numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
                           &error_fatal);
 
-        if (object_property_find(cpuobj, "reset-cbar")) {
+        if (arm_feature(cpu_env(cs), ARM_FEATURE_CBAR) ||
+            arm_feature(cpu_env(cs), ARM_FEATURE_CBAR_RO)) {
             object_property_set_int(cpuobj, "reset-cbar",
                                     sbsa_ref_memmap[SBSA_CPUPERIPHS].base,
                                     &error_abort);
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 753a645c05..ea3c76f3e1 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -229,7 +229,8 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
             }
         }
 
-        if (object_property_find(cpuobj, "reset-cbar")) {
+        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
+            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
             object_property_set_int(cpuobj, "reset-cbar", periphbase,
                                     &error_abort);
         }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 35eb01a3dc..7e7350fec2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2168,7 +2168,8 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "lpa2", false, NULL);
         }
 
-        if (object_property_find(cpuobj, "reset-cbar")) {
+        if (arm_feature(cpu_env(cs), ARM_FEATURE_CBAR) ||
+            arm_feature(cpu_env(cs), ARM_FEATURE_CBAR_RO)) {
             object_property_set_int(cpuobj, "reset-cbar",
                                     vms->memmap[VIRT_CPUPERIPHS].base,
                                     &error_abort);
-- 
2.41.0

[PATCH v3 11/14] hw/arm: Prefer arm_feature(PMU) over object_property_find(pmu)

[Philippe Mathieu-Daudé]

The "pmu" property is added to ARMCPU when the
ARM_FEATURE_PMU feature is available. Rather than
checking whether the QOM property is present, directly
check the feature.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7e7350fec2..6d1cb24a6e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2160,7 +2160,7 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "kvm-steal-time", false, NULL);
         }
 
-        if (vmc->no_pmu && object_property_find(cpuobj, "pmu")) {
+        if (arm_feature(cpu_env(cs), ARM_FEATURE_PMU) && vmc->no_pmu) {
             object_property_set_bool(cpuobj, "pmu", false, NULL);
         }
 
-- 
2.41.0

[PATCH v3 12/14] hw/arm: Prefer arm_feature(GENERIC_TMR) over 'kvm-no-adjvtime' property

[Philippe Mathieu-Daudé]

First, the "kvm-no-adjvtime" and "kvm-steal-time" are only
available when KVM is available, so guard this block within
a 'kvm_enabled()' check. Since the "kvm-steal-time" property
is always available under KVM, directly set it.

Then, the "kvm-no-adjvtime" property is added to ARMCPU when
the ARM_FEATURE_GENERIC_TIMER feature is available. Rather than
checking whether the QOM property is present, directly check
the feature.

Finally, since we are sure the properties are available, we can
use &error_abort instead of NULL error. Replace:

  object_property_set_bool(..., PROPERTY, ..., &error_abort);

by:

  qdev_prop_set_bit(..., PROPERTY, ...);

which is a one-to-one replacement.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6d1cb24a6e..49ed5309ff 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2150,14 +2150,13 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "has_el2", false, NULL);
         }
 
-        if (vmc->kvm_no_adjvtime &&
-            object_property_find(cpuobj, "kvm-no-adjvtime")) {
-            object_property_set_bool(cpuobj, "kvm-no-adjvtime", true, NULL);
-        }
-
-        if (vmc->no_kvm_steal_time &&
-            object_property_find(cpuobj, "kvm-steal-time")) {
-            object_property_set_bool(cpuobj, "kvm-steal-time", false, NULL);
+        if (kvm_enabled()) {
+            if (arm_feature(cpu_env(cs), ARM_FEATURE_GENERIC_TIMER)) {
+                qdev_prop_set_bit(DEVICE(cs), "kvm-no-adjvtime",
+                                  vmc->kvm_no_adjvtime);
+            }
+            qdev_prop_set_bit(DEVICE(cs), "kvm-steal-time",
+                              !vmc->no_kvm_steal_time);
         }
 
         if (arm_feature(cpu_env(cs), ARM_FEATURE_PMU) && vmc->no_pmu) {
-- 
2.41.0

[PATCH v3 13/14] hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)

[Philippe Mathieu-Daudé]

The "aarch64" property is added to ARMCPU when the
ARM_FEATURE_AARCH64 feature is available. Rather than
checking whether the QOM property is present, directly
check the feature.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 49ed5309ff..a43e87874c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2140,7 +2140,7 @@ static void machvirt_init(MachineState *machine)
         numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
                           &error_fatal);
 
-        aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
+        aarch64 &= arm_feature(cpu_env(cs), ARM_FEATURE_AARCH64);
 
         if (!vms->secure) {
             object_property_set_bool(cpuobj, "has_el3", false, NULL);
-- 
2.41.0

[PATCH v3 14/14] hw/arm: Prefer cpu_isar_feature(aa64_mte) over property_find(tag-memory)

[Philippe Mathieu-Daudé]

The "tag-memory" property is added to ARMCPU when the
A64_MTE bit is set in the feature ID register. Rather
than checking whether the QOM property is present, directly
check the feature bit.

Since when ARM_FEATURE_AARCH64 is disabled the isar_aa64_mte
register is invalid, also check for it (see the 'aarch64'
variable set in the previous commit).

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a43e87874c..7fd7173b5b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2188,7 +2188,7 @@ static void machvirt_init(MachineState *machine)
                  * The property exists only if MemTag is supported.
                  * If it is, we must allocate the ram to back that up.
                  */
-                if (!object_property_find(cpuobj, "tag-memory")) {
+                if (!aarch64 || !cpu_isar_feature(aa64_mte, ARM_CPU(cs))) {
                     error_report("MTE requested, but not supported "
                                  "by the guest CPU");
                     exit(1);
-- 
2.41.0

Re: [PATCH v3 13/14] hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)

[Philippe Mathieu-Daudé]

On 10/1/24 20:53, Philippe Mathieu-Daudé wrote:
> The "aarch64" property is added to ARMCPU when the
> ARM_FEATURE_AARCH64 feature is available. Rather than
> checking whether the QOM property is present, directly
> check the feature.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/virt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 49ed5309ff..a43e87874c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2140,7 +2140,7 @@ static void machvirt_init(MachineState *machine)
>           numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
>                             &error_fatal);
>   
> -        aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
> +        aarch64 &= arm_feature(cpu_env(cs), ARM_FEATURE_AARCH64);

So after this patch there are no more use of the ARMCPU "aarch64"
property from code. Still it is exposed via the qom-tree. Thus it
can be set (see aarch64_cpu_set_aarch64). I could understand one
flip this feature to create a custom CPU (as a big-LITTLE setup
as Marc mentioned on IRC), but I don't understand what is the
expected behavior when this is flipped at runtime. Can that
happen in real hardware (how could the guest react to that...)?

Thanks,

Phil.

Re: [PATCH v3 13/14] hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)

[Marc Zyngier]

On Thu, 11 Jan 2024 09:39:18 +0000,
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> On 10/1/24 20:53, Philippe Mathieu-Daudé wrote:
> > The "aarch64" property is added to ARMCPU when the
> > ARM_FEATURE_AARCH64 feature is available. Rather than
> > checking whether the QOM property is present, directly
> > check the feature.
> > 
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/arm/virt.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 49ed5309ff..a43e87874c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2140,7 +2140,7 @@ static void machvirt_init(MachineState *machine)
> >           numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> >                             &error_fatal);
> >   -        aarch64 &= object_property_get_bool(cpuobj, "aarch64",
> > NULL);
> > +        aarch64 &= arm_feature(cpu_env(cs), ARM_FEATURE_AARCH64);
> 
> So after this patch there are no more use of the ARMCPU "aarch64"
> property from code. Still it is exposed via the qom-tree. Thus it
> can be set (see aarch64_cpu_set_aarch64). I could understand one
> flip this feature to create a custom CPU (as a big-LITTLE setup
> as Marc mentioned on IRC), but I don't understand what is the
> expected behavior when this is flipped at runtime. Can that
> happen in real hardware (how could the guest react to that...)?

I don't think it makes any sense to do that while a guest is running
(and no HW I'm aware of would do this). However, it all depends what
you consider "run time". You could imagine creating a skeletal VM with
all features, and then apply a bunch of changes before the guest
actually runs.

I don't know enough about the qom-tree and dynamic manipulation of
these properties though, and I'm likely to be wrong about the expected
usage model.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 13/14] hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)

[Philippe Mathieu-Daudé]

On 11/1/24 10:47, Marc Zyngier wrote:
> On Thu, 11 Jan 2024 09:39:18 +0000,
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 10/1/24 20:53, Philippe Mathieu-Daudé wrote:
>>> The "aarch64" property is added to ARMCPU when the
>>> ARM_FEATURE_AARCH64 feature is available. Rather than
>>> checking whether the QOM property is present, directly
>>> check the feature.
>>>
>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/arm/virt.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 49ed5309ff..a43e87874c 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -2140,7 +2140,7 @@ static void machvirt_init(MachineState *machine)
>>>            numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
>>>                              &error_fatal);
>>>    -        aarch64 &= object_property_get_bool(cpuobj, "aarch64",
>>> NULL);
>>> +        aarch64 &= arm_feature(cpu_env(cs), ARM_FEATURE_AARCH64);
>>
>> So after this patch there are no more use of the ARMCPU "aarch64"
>> property from code. Still it is exposed via the qom-tree. Thus it
>> can be set (see aarch64_cpu_set_aarch64). I could understand one
>> flip this feature to create a custom CPU (as a big-LITTLE setup
>> as Marc mentioned on IRC), but I don't understand what is the
>> expected behavior when this is flipped at runtime. Can that
>> happen in real hardware (how could the guest react to that...)?
> 
> I don't think it makes any sense to do that while a guest is running
> (and no HW I'm aware of would do this). However, it all depends what
> you consider "run time". You could imagine creating a skeletal VM with
> all features, and then apply a bunch of changes before the guest
> actually runs.

Thanks, this makes sense and confirms my guess.

> I don't know enough about the qom-tree and dynamic manipulation of
> these properties though, and I'm likely to be wrong about the expected
> usage model.

Kevin, Markus, this seems a good example of QOM "config" property that
is RW *before* Realize and should become RO *after* it.

QDev properties has PropertyInfo::realized_set_allowed set to false by
default, but here this property is added at the QOM (lower) layer, so
there is no such check IIUC.

Should "aarch64" become a static QDev property instead (registered via
device_class_set_props -> qdev_class_add_property)?

This just an analyzed example, unfortunately there are many more...

Thanks,

Phil.

Re: [PATCH v3 01/14] hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()

[Peter Maydell]

On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We are going to cast s->cpu as DeviceState multiple times.
> Add a local 'cpudev' variable to simplify code review, having
> a single DEVICE(s->cpu) conversion.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/armv7m.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 02/14] hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M

[Peter Maydell]

On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> ARMV7M container can only accept M-profile CPU types.
> Check requested type is valid once to allow further simplifications.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/armv7m.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 03/14] hw/arm/armv7m: Move code setting 'start-powered-off' property around

[Peter Maydell]

On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Reorganize a bit by first setting properties which are not
> dependent of CPU features (and can not fail).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 04/14] hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs

[Peter Maydell]

On Wed, 10 Jan 2024 at 19:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> All CPUs implementing ARM_FEATURE_M have the 'init-nsvtor' property.
> Since setting the property can not fail, replace
>
>    object_property_set_uint(..., "init-nsvtor", ..., &error_abort);
>
> by:
>    qdev_prop_set_uint32(..., "init-nsvtor", ...).
>
> which is a one-to-one replacement.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[Peter Maydell]

On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Since v2 [2]:
> - Dropped "Simplify checking A64_MTE bit in FEATURE_ID register"
> - Correct object_property_get_bool() uses
> - Update ARM_FEATURE_AARCH64 && aa64_mte
>
> Since RFC [1]:
> - Split one patch per feature
> - Addressed Peter's review comments
>
> [1] https://lore.kernel.org/qemu-devel/20231214171447.44025-1-philmd@linaro.org/
> [2] https://lore.kernel.org/qemu-devel/20240109180930.90793-1-philmd@linaro.org/
>
> Based-on: <20231123143813.42632-1-philmd@linaro.org>
>   "hw: Simplify accesses to CPUState::'start-powered-off' property"
>
> Philippe Mathieu-Daudé (14):
>   hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()
>   hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M
>   hw/arm/armv7m: Move code setting 'start-powered-off' property around
>   hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs
>   hw/arm: Prefer arm_feature(M_SECURITY) over object_property_find()
>   hw/arm: Prefer arm_feature(THUMB_DSP) over object_property_find(dsp)
>   hw/arm: Prefer arm_feature(V7) over
>     object_property_find(pmsav7-dregion)
>   hw/arm: Prefer arm_feature(EL3) over object_property_find(has_el3)
>   hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2)
>   hw/arm: Prefer arm_feature(CBAR*) over
>     object_property_find(reset-cbar)
>   hw/arm: Prefer arm_feature(PMU) over object_property_find(pmu)
>   hw/arm: Prefer arm_feature(GENERIC_TMR) over 'kvm-no-adjvtime'
>     property
>   hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)
>   hw/arm: Prefer cpu_isar_feature(aa64_mte) over
>     property_find(tag-memory)

The first part of this is fine and reasonable cleanup, but I
continue to disagree about the later parts. What we want to do is
"if this property is present, then set it", and that's what we do.
Conversion to "if <some condition we know that the CPU is using to
decide whether to define the property> then set it" is duplicating
the condition logic in two places and opening the door for bugs
where we change the condition in one place and not in the other.
Where the <some condition> is a simple "feature X is set" it doesn't
look too bad, but where it gets more complex it makes it IMHO more
obvious that this is a bad idea, for example with:

-        if (object_property_find(cpuobj, "reset-cbar")) {
+        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
+            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {

thanks
-- PMM

Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[Philippe Mathieu-Daudé]

On 13/1/24 14:36, Peter Maydell wrote:
> On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Since v2 [2]:
>> - Dropped "Simplify checking A64_MTE bit in FEATURE_ID register"
>> - Correct object_property_get_bool() uses
>> - Update ARM_FEATURE_AARCH64 && aa64_mte
>>
>> Since RFC [1]:
>> - Split one patch per feature
>> - Addressed Peter's review comments
>>
>> [1] https://lore.kernel.org/qemu-devel/20231214171447.44025-1-philmd@linaro.org/
>> [2] https://lore.kernel.org/qemu-devel/20240109180930.90793-1-philmd@linaro.org/
>>
>> Based-on: <20231123143813.42632-1-philmd@linaro.org>
>>    "hw: Simplify accesses to CPUState::'start-powered-off' property"
>>
>> Philippe Mathieu-Daudé (14):
>>    hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()
>>    hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M
>>    hw/arm/armv7m: Move code setting 'start-powered-off' property around
>>    hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs


> The first part of this is fine and reasonable cleanup, but I
> continue to disagree about the later parts. What we want to do is
> "if this property is present, then set it", and that's what we do.
> Conversion to "if <some condition we know that the CPU is using to
> decide whether to define the property> then set it" is duplicating
> the condition logic in two places and opening the door for bugs
> where we change the condition in one place and not in the other.
> Where the <some condition> is a simple "feature X is set" it doesn't
> look too bad, but where it gets more complex it makes it IMHO more
> obvious that this is a bad idea, for example with:
> 
> -        if (object_property_find(cpuobj, "reset-cbar")) {
> +        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
> +            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {

For that we could add helpers such

   static inline bool arm_feature_cbar(CPUState *cpu) {
     return arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
   }

and use it in the target/ code where we register the property
and in the hw/ code where we set it.


Anyway I'll wait to see how Kevin's effort is going before
insisting with this series.
Do you mind taking the cleanup patches (1-4) meanwhile?

Thanks,

Phil.

Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[Peter Maydell]

On Tue, 16 Jan 2024 at 16:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 13/1/24 14:36, Peter Maydell wrote:
> > On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Since v2 [2]:
> >> - Dropped "Simplify checking A64_MTE bit in FEATURE_ID register"
> >> - Correct object_property_get_bool() uses
> >> - Update ARM_FEATURE_AARCH64 && aa64_mte
> >>
> >> Since RFC [1]:
> >> - Split one patch per feature
> >> - Addressed Peter's review comments
> >>
> >> [1] https://lore.kernel.org/qemu-devel/20231214171447.44025-1-philmd@linaro.org/
> >> [2] https://lore.kernel.org/qemu-devel/20240109180930.90793-1-philmd@linaro.org/
> >>
> >> Based-on: <20231123143813.42632-1-philmd@linaro.org>
> >>    "hw: Simplify accesses to CPUState::'start-powered-off' property"
> >>
> >> Philippe Mathieu-Daudé (14):
> >>    hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()
> >>    hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M
> >>    hw/arm/armv7m: Move code setting 'start-powered-off' property around
> >>    hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs
>
>
> > The first part of this is fine and reasonable cleanup, but I
> > continue to disagree about the later parts. What we want to do is
> > "if this property is present, then set it", and that's what we do.
> > Conversion to "if <some condition we know that the CPU is using to
> > decide whether to define the property> then set it" is duplicating
> > the condition logic in two places and opening the door for bugs
> > where we change the condition in one place and not in the other.
> > Where the <some condition> is a simple "feature X is set" it doesn't
> > look too bad, but where it gets more complex it makes it IMHO more
> > obvious that this is a bad idea, for example with:
> >
> > -        if (object_property_find(cpuobj, "reset-cbar")) {
> > +        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
> > +            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
>
> For that we could add helpers such
>
>    static inline bool arm_feature_cbar(CPUState *cpu) {
>      return arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
>             arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>    }
>
> and use it in the target/ code where we register the property
> and in the hw/ code where we set it.

Well, we could, but why? The question we're trying to
answer is "can we set this property?" and the simplest
and most logical way to test that is "does the object
have the property?". I really don't understand why we
would want to change the code at all.

> Do you mind taking the cleanup patches (1-4) meanwhile?

Yes, I can take patches 1-4.

thanks
-- PMM

Re: [PATCH v3 13/14] hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64)

[Kevin Wolf]

Am 11.01.2024 um 11:08 hat Philippe Mathieu-Daudé geschrieben:
> On 11/1/24 10:47, Marc Zyngier wrote:
> > On Thu, 11 Jan 2024 09:39:18 +0000,
> > Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > 
> > > On 10/1/24 20:53, Philippe Mathieu-Daudé wrote:
> > > > The "aarch64" property is added to ARMCPU when the
> > > > ARM_FEATURE_AARCH64 feature is available. Rather than
> > > > checking whether the QOM property is present, directly
> > > > check the feature.
> > > > 
> > > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > ---
> > > >    hw/arm/virt.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index 49ed5309ff..a43e87874c 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -2140,7 +2140,7 @@ static void machvirt_init(MachineState *machine)
> > > >            numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> > > >                              &error_fatal);
> > > >    -        aarch64 &= object_property_get_bool(cpuobj, "aarch64",
> > > > NULL);
> > > > +        aarch64 &= arm_feature(cpu_env(cs), ARM_FEATURE_AARCH64);
> > > 
> > > So after this patch there are no more use of the ARMCPU "aarch64"
> > > property from code. Still it is exposed via the qom-tree. Thus it
> > > can be set (see aarch64_cpu_set_aarch64). I could understand one
> > > flip this feature to create a custom CPU (as a big-LITTLE setup
> > > as Marc mentioned on IRC), but I don't understand what is the
> > > expected behavior when this is flipped at runtime. Can that
> > > happen in real hardware (how could the guest react to that...)?
> > 
> > I don't think it makes any sense to do that while a guest is running
> > (and no HW I'm aware of would do this). However, it all depends what
> > you consider "run time". You could imagine creating a skeletal VM with
> > all features, and then apply a bunch of changes before the guest
> > actually runs.
> 
> Thanks, this makes sense and confirms my guess.
> 
> > I don't know enough about the qom-tree and dynamic manipulation of
> > these properties though, and I'm likely to be wrong about the expected
> > usage model.
> 
> Kevin, Markus, this seems a good example of QOM "config" property that
> is RW *before* Realize and should become RO *after* it.
> 
> QDev properties has PropertyInfo::realized_set_allowed set to false by
> default, but here this property is added at the QOM (lower) layer, so
> there is no such check IIUC.

You can take almost any other config property and it will show the same
pattern. This is the normal case (and one of the reasons why the current
way of doing QOM properties isn't great).

As you say, qdev tries to take care of this. In pure QOM properties, the
property setter must have manually implemented checks, and perhaps not
very surprisingly, people tend to forget to add them.

> Should "aarch64" become a static QDev property instead (registered via
> device_class_set_props -> qdev_class_add_property)?
> 
> This just an analyzed example, unfortunately there are many more...

target/arm/cpu64.c already seems to use a wild mix of ways to add
properties, so maybe it wouldn't make things worse...

It's good to look at such devices because it shows how hard QAPIfication
of all devices would become (fortunately the subset we're really
interested in most is user creatable devices, and I don't think users
can create CPUs with -device even though they look like it and are
mentioned in -device help?).

The basic requirement for QAPIfication is that each type has a fixed
list of properties. This is easy with devices that create their
properties with a single device_class_set_props(), but devices that
directly create properties, some of them conditional, and spread across
several different functions, it becomes hard to see what the real list
of properties is.

Even worse, there are properties whose creation depends on runtime
options like which accelerator is used ("pauth-impdef" and
"pauth-qarma3" exist only for TCG). There is no way to write a schema
for that. In QAPI, you can have it optional and return an error if it's
set when it shouldn't be, but the existence of the property itself can't
(currently?) depend on runtime options.

Kevin

Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[Peter Maydell]

On Tue, 16 Jan 2024 at 16:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 16 Jan 2024 at 16:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > On 13/1/24 14:36, Peter Maydell wrote:
> > > On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > >>
> > >> Since v2 [2]:
> > >> - Dropped "Simplify checking A64_MTE bit in FEATURE_ID register"
> > >> - Correct object_property_get_bool() uses
> > >> - Update ARM_FEATURE_AARCH64 && aa64_mte
> > >>
> > >> Since RFC [1]:
> > >> - Split one patch per feature
> > >> - Addressed Peter's review comments
> > >>
> > >> [1] https://lore.kernel.org/qemu-devel/20231214171447.44025-1-philmd@linaro.org/
> > >> [2] https://lore.kernel.org/qemu-devel/20240109180930.90793-1-philmd@linaro.org/
> > >>
> > >> Based-on: <20231123143813.42632-1-philmd@linaro.org>
> > >>    "hw: Simplify accesses to CPUState::'start-powered-off' property"
> > >>
> > >> Philippe Mathieu-Daudé (14):
> > >>    hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()
> > >>    hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M
> > >>    hw/arm/armv7m: Move code setting 'start-powered-off' property around
> > >>    hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs
> >
> >
> > > The first part of this is fine and reasonable cleanup, but I
> > > continue to disagree about the later parts. What we want to do is
> > > "if this property is present, then set it", and that's what we do.
> > > Conversion to "if <some condition we know that the CPU is using to
> > > decide whether to define the property> then set it" is duplicating
> > > the condition logic in two places and opening the door for bugs
> > > where we change the condition in one place and not in the other.
> > > Where the <some condition> is a simple "feature X is set" it doesn't
> > > look too bad, but where it gets more complex it makes it IMHO more
> > > obvious that this is a bad idea, for example with:
> > >
> > > -        if (object_property_find(cpuobj, "reset-cbar")) {
> > > +        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
> > > +            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
> >
> > For that we could add helpers such
> >
> >    static inline bool arm_feature_cbar(CPUState *cpu) {
> >      return arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
> >             arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> >    }
> >
> > and use it in the target/ code where we register the property
> > and in the hw/ code where we set it.
>
> Well, we could, but why? The question we're trying to
> answer is "can we set this property?" and the simplest
> and most logical way to test that is "does the object
> have the property?". I really don't understand why we
> would want to change the code at all.
>
> > Do you mind taking the cleanup patches (1-4) meanwhile?
>
> Yes, I can take patches 1-4.

Hmm, this doesn't apply cleanly (probably due to the dependency
noted in the Based-on tag). Can you resend the relevant patches
in a form where they'll apply, or ping me once the dependency has
gone upstream), please?

thanks
-- PMM