| Message ID | 20240110195329.3995-14-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: Prefer arm_feature() over object_property_find() | expand |
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.
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.
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.
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
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);
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(-)