| Message ID | 20231214171447.44025-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] hw/arm: Prefer arm_feature() over object_property_find() | expand |
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? thanks -- PMM
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.
On Mon, 18 Dec 2023 at 07:26, Markus Armbruster <armbru@redhat.com> 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(). But we don't in most of these cases really want to know "is this a CPU with feature foo?". What we're asking is "does this QOM property exist so it won't blow up if I set/get it?". > 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. I agree, and the Arm CPU is a bit of an outlier in what it's doing, for reasons that are largely I think historical. I'd be happy to review patches that change these to class properties where applicable, but I suspect that might be tricky... thanks -- PMM
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" > --- > hw/arm/armv7m.c | 21 ++++++++++++--------- > 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 | 27 +++++++++++++++------------ > hw/arm/xilinx_zynq.c | 2 +- > hw/cpu/a15mpcore.c | 17 +++++++++++------ > hw/cpu/a9mpcore.c | 6 +++--- > 12 files changed, 57 insertions(+), 44 deletions(-) > > diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c > index 3a6d72b0f3..932061c11a 100644 > --- a/hw/arm/armv7m.c > +++ b/hw/arm/armv7m.c > @@ -302,28 +302,29 @@ static void armv7m_realize(DeviceState *dev, Error **errp) > > object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container), > &error_abort); > - if (object_property_find(OBJECT(s->cpu), "idau")) { > + if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) { > 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), "init-nsvtor")) { > + if (arm_feature(&s->cpu->env, ARM_FEATURE_M)) { This doesn't make sense as a check -- we shouldn't be able to get here if the CPU isn't M-profile. > 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; > + if (arm_feature(&s->cpu->env, ARM_FEATURE_AARCH64)) { Similarly this can't possibly be an AArch64 CPU, so this is not the correct condition to check. > + if (cpu_isar_feature(aa64_fp_simd, s->cpu)) { > + if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) { > + return; > + } > } > } > - if (object_property_find(OBJECT(s->cpu), "dsp")) { > + if (arm_feature(&s->cpu->env, ARM_FEATURE_M) && > + arm_feature(&s->cpu->env, ARM_FEATURE_THUMB_DSP)) { > if (!object_property_set_bool(OBJECT(s->cpu), "dsp", s->dsp, errp)) { Another unnecessary "is this M-profile?" check. This also is introducing a point of potential future failure because now we have the condition for "do we have a dsp property" in two places: in the CPU object where we add it, and then again here when we set it. Now they can get out of sync. Most of the others are similar. There might be places where we're using the "does property X check" to do something more than just guard "now set property X"; those are probably worth looking at to see if they should be checking something else. thanks -- PMM
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.
Hi, On 18/12/23 10:48, Peter Maydell wrote: > On Mon, 18 Dec 2023 at 07:26, Markus Armbruster <armbru@redhat.com> 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(). > > But we don't in most of these cases really want to know "is this > a CPU with feature foo?". What we're asking is "does this > QOM property exist so it won't blow up if I set/get it?". [More analysis on this topic.] ARMV7M (hw/arm/armv7m.c) is an interesting QOM use case. ARMV7M is a ARMCPU container, with few more things. (We have more complex cases with containers of array of vCPUs, so this single-vCPU case is a good start). We'd like to apply properties on ARMV7M which get forwarded to the embedded ARMCPU. Usually we create the ARMCPU in armv7m_instance_init(), call object_property_add_alias() to alias some ARMCPU to ARMV7M, so these properties can be set externally before ARMV7M is realized, being directly forwarded to the embedded ARMCPU [*]. The problem with ARMV7M is it the ARMCPU QOM type is variable, so we don't know it in armv7m_instance_init() but only later in armv7m_realize(), thus we can not call QOM _add_alias() to alias them. One way to resolve this is to duplicate all possible ARMCPU properties we want to set on ARMV7M, and set them in armv7m_realize() after the ARMCPU is created and before it is realized (the current implementation): static void armv7m_realize(DeviceState *dev, Error **errp) { ... s->cpu = ARM_CPU(object_new_with_props(s->cpu_type, OBJECT(s), "cpu", &err, NULL)); ... if (object_property_find(OBJECT(s->cpu), "vfp")) { if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) { return; } } ... if (!qdev_realize(cpudev, NULL, errp)) { return; } ... } static Property armv7m_properties[] = { DEFINE_PROP_STRING("cpu-type", ARMv7MState, cpu_type), ... DEFINE_PROP_BOOL("vfp", ARMv7MState, vfp, true), ... }; Note ARMV7M "vfp" is a /static/ QOM property, so can not be unregistered if the ARMCPU doesn't expose it. * If ARMCPU doesn't provide "vfp", ARMV7M properties introspection still shows 'vfp=true'. * If ARMCPU doesn't provide "vfp", requesting 'vfp=true' on ARMV7M is silently ignored. * If ARMCPU doesn't provide "vfp", even if we unregister ARMV7M "vfp" property in realize() for cleaner introspection, we can not check whether user requested an explicit value before realize(). Possibly we could use a tri-state {unset/false/true} dynamic property to check that. [*] object_property_add_alias() is a 1-1 static aliasing. In the case of cluster of objects we don't have API to do a 1-N static aliasing; the current way to do that is similar to dynamic properties setters iterating on the array (getter usually return the container property, ignoring the cluster values). Regards, Phil.
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c index 3a6d72b0f3..932061c11a 100644 --- a/hw/arm/armv7m.c +++ b/hw/arm/armv7m.c @@ -302,28 +302,29 @@ static void armv7m_realize(DeviceState *dev, Error **errp) object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container), &error_abort); - if (object_property_find(OBJECT(s->cpu), "idau")) { + if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) { 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), "init-nsvtor")) { + if (arm_feature(&s->cpu->env, ARM_FEATURE_M)) { 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; + if (arm_feature(&s->cpu->env, ARM_FEATURE_AARCH64)) { + if (cpu_isar_feature(aa64_fp_simd, s->cpu)) { + if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) { + return; + } } } - if (object_property_find(OBJECT(s->cpu), "dsp")) { + if (arm_feature(&s->cpu->env, ARM_FEATURE_M) && + arm_feature(&s->cpu->env, ARM_FEATURE_THUMB_DSP)) { if (!object_property_set_bool(OBJECT(s->cpu), "dsp", s->dsp, errp)) { return; } @@ -342,12 +343,14 @@ static void armv7m_realize(DeviceState *dev, Error **errp) return; } if (s->mpu_ns_regions != UINT_MAX && - object_property_find(OBJECT(s->cpu), "pmsav7-dregion")) { + arm_feature(&s->cpu->env, ARM_FEATURE_PMSA)) { + if (arm_feature(&s->cpu->env, ARM_FEATURE_V7)) { if (!object_property_set_uint(OBJECT(s->cpu), "pmsav7-dregion", s->mpu_ns_regions, errp)) { return; } } + } /* * Tell the CPU where the NVIC is; it will fail realize if it doesn't 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/highbank.c b/hw/arm/highbank.c index f12aacea6b..0b5def8015 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/integratorcp.c b/hw/arm/integratorcp.c index d176e9af7e..5ec840ce89 100644 --- a/hw/arm/integratorcp.c +++ b/hw/arm/integratorcp.c @@ -599,19 +599,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/sbsa-ref.c b/hw/arm/sbsa-ref.c index f3c9704693..ecb05f6007 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -796,7 +796,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/versatilepb.c b/hw/arm/versatilepb.c index 2f22dc890f..addd7a6988 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/vexpress.c b/hw/arm/vexpress.c index fd981f4c33..ea3c76f3e1 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -218,17 +218,19 @@ 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); } } - 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 be2856c018..e5d883c4c3 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2182,21 +2182,22 @@ 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); } - if (vmc->kvm_no_adjvtime && - object_property_find(cpuobj, "kvm-no-adjvtime")) { - object_property_set_bool(cpuobj, "kvm-no-adjvtime", true, NULL); + if (kvm_enabled()) { + if (arm_feature(cpu_env(cs), ARM_FEATURE_GENERIC_TIMER)) { + object_property_set_bool(cpuobj, "kvm-no-adjvtime", + vmc->kvm_no_adjvtime, NULL); + } + + if (vmc->no_kvm_steal_time) { + object_property_set_bool(cpuobj, "kvm-steal-time", false, 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 (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); } @@ -2204,7 +2205,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); @@ -2224,7 +2226,8 @@ 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 (!(arm_feature(cpu_env(cs), ARM_FEATURE_AARCH64) + && cpu_isar_feature(aa64_mte, ARM_CPU(cs)))) { error_report("MTE requested, but not supported " "by the guest CPU"); exit(1); 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..1fa079b3b8 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,17 +61,23 @@ 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") && + cpu = qemu_get_cpu(0); + cpuobj = OBJECT(cpu); + if (arm_feature(cpu_env(cpu), ARM_FEATURE_EL3)) { object_property_get_bool(cpuobj, "has_el3", &error_abort); - qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3); + qdev_prop_set_bit(gicdev, "has-security-extensions", true); + } /* Similarly for virtualization support */ - has_el2 = object_property_find(cpuobj, "has_el2") && + has_el2 = arm_feature(cpu_env(cpu), ARM_FEATURE_EL2); + if (has_el2) { object_property_get_bool(cpuobj, "has_el2", &error_abort); - qdev_prop_set_bit(gicdev, "has-virtualization-extensions", has_el2); + qdev_prop_set_bit(gicdev, "has-virtualization-extensions", true); + } } if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) { diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c index d03f57e579..9355e8443b 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,10 @@ 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") && + if (arm_feature(cpu_env(cpu0), ARM_FEATURE_EL3)) { object_property_get_bool(cpuobj, "has_el3", &error_abort); - qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3); + qdev_prop_set_bit(gicdev, "has-security-extensions", true); + } if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) { return;
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" --- hw/arm/armv7m.c | 21 ++++++++++++--------- 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 | 27 +++++++++++++++------------ hw/arm/xilinx_zynq.c | 2 +- hw/cpu/a15mpcore.c | 17 +++++++++++------ hw/cpu/a9mpcore.c | 6 +++--- 12 files changed, 57 insertions(+), 44 deletions(-)