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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.