Re: [RFC PATCH] hw/arm: Prefer arm_feature() over object_property_find()

[Philippe Mathieu-Daudé]

Hi Markus, Kevin,

On 18/12/23 08:26, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Thu, 14 Dec 2023 at 17:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > QOM properties are added on the ARM vCPU object when a
> > > feature is present. Rather than checking the property
> > > is present, check the feature.
> > >
> > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > > RFC: If there is no objection on this patch, I can split
> > >       as a per-feature series if necessary.
> > >
> > > Based-on: <20231123143813.42632-1-philmd@linaro.org>
> > >    "hw: Simplify accesses to CPUState::'start-powered-off' property"
> >
> > I'm not a super-fan of board-level code looking inside
> > the QOM object with direct use of arm_feature() when
> > it doesn't have to. What's wrong with asking whether
> > the property exists before trying to set it?
>
> I'm not a fan of using QOM instead of the native C interface.
>
> The native C interface is CPUArmState method arm_feature().
>
> Attempts to use it on anything but a CPUArmState * will be caught by the
> compiler.  object_property_find() will happily take any Object.
>
> Likewise, typos in its second argument will be caught by the compiler.
> object_property_find() will happily return NULL then.
>
>
> I also don't like adding QOM properties to instances.
> arm_cpu_post_init() seems to do that.  I feel it's best to stick to
> class properties whenever practical.
>
> More so when management applications are expected to use them, because
> introspection is much easier to use correctly when existence of
> properties doesn't depend on run-time state.
>
> Kevin's "[RFC PATCH 00/12] QOM/QAPI integration part 1" explores
> QAPIfication of object configuration, loosely based on
> <https://wiki.qemu.org/Features/QOM-QAPI_integration>.  A QOM class's
> instance configuration interface becomes compile-time static.

What is your take on this pattern from commit f50cd31413
("arm: Don't clear ARM_FEATURE_PMSA for no-mpu configs"):

commit f50cd31413d8bc9d1eef8edd1f878324543bf65d

    arm: Don't clear ARM_FEATURE_PMSA for no-mpu configs

    Fix the handling of QOM properties for PMSA CPUs with no MPU:

    Allow no-MPU to be specified by either:
     * has-mpu = false
     * pmsav7_dregion = 0
    and make setting one imply the other. Don't clear the PMSA
    feature bit in this situation.

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index f844af5673..76a5e20111 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -763,8 +763,14 @@ static void arm_cpu_realizefn(DeviceState *dev, 
Error **errp)
         cpu->id_pfr1 &= ~0xf000;
     }

+    /* MPU can be configured out of a PMSA CPU either by setting has-mpu
+     * to false or by setting pmsav7-dregion to 0.
+     */
     if (!cpu->has_mpu) {
-        unset_feature(env, ARM_FEATURE_PMSA);
+        cpu->pmsav7_dregion = 0;
+    }
+    if (cpu->pmsav7_dregion == 0) {
+        cpu->has_mpu = false;
     }

     if (arm_feature(env, ARM_FEATURE_PMSA) &&

---

Both "has-mpu" and "pmsav7-dregion" are static properties, used as QOM
configuration provided by /before/ the realize() handler; but then one
is (en)forced /after/ realize().

Logically this looks like a AND configuration gate; is this acceptable?

I tend to see QOM config properties as writable *before* realize() and
then to be used as read-only within/after realize(). So here the KISS
approach would be to report an error the 2 properties differ, as an XNOR
gate:

  if (cpu->has_mpu ^ !cpu->pmsav7_dregion) {
      error_setg(errp,
                 "%u pmsav7-dregions requested but no MPU available",
                 cpu->pmsav7_dregion);
      return false;
  }

Thanks,

Phil.