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