| Message ID | 20230619140216.402530-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Restructure has_vfp_d32 test | expand |
On 19/6/23 16:02, Richard Henderson wrote: > One cannot test for feature aa32_simd_r32 without first > testing if AArch32 mode is supported at all. This leads to > > qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither > > for Apple M1 cpus. > > We already have a check for ARMv8-A never setting vfp-d32 true, > so restructure the code so that AArch64 avoids the test entirely. > > Reported-by: Mads Ynddal <mads@ynddal.dk> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 6/19/23 18:07, Philippe Mathieu-Daudé wrote:
> We already have a check for ARMv8-A never setting vfp-d32 true,
... gah! "false".
r~
On 6/19/23 16:02, Richard Henderson wrote: > One cannot test for feature aa32_simd_r32 without first > testing if AArch32 mode is supported at all. This leads to > > qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither > > for Apple M1 cpus. The orangepi5 board I use didn't have that kind problem. > We already have a check for ARMv8-A never setting vfp-d32 true, > so restructure the code so that AArch64 avoids the test entirely. > > Reported-by: Mads Ynddal <mads@ynddal.dk> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks for the fix, C. > --- > target/arm/cpu.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 353fc48567..706dbd37b1 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1402,25 +1402,27 @@ void arm_cpu_post_init(Object *obj) > * KVM does not currently allow us to lie to the guest about its > * ID/feature registers, so the guest always sees what the host has. > */ > - if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) > - ? cpu_isar_feature(aa64_fp_simd, cpu) > - : cpu_isar_feature(aa32_vfp, cpu)) { > - cpu->has_vfp = true; > - if (!kvm_enabled()) { > - qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property); > + if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { > + if (cpu_isar_feature(aa64_fp_simd, cpu)) { > + cpu->has_vfp = true; > + cpu->has_vfp_d32 = true; > + if (tcg_enabled() || qtest_enabled()) { > + qdev_property_add_static(DEVICE(obj), > + &arm_cpu_has_vfp_property); > + } > } > - } > - > - if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) { > - cpu->has_vfp_d32 = true; > - if (!kvm_enabled()) { > + } else if (cpu_isar_feature(aa32_vfp, cpu)) { > + cpu->has_vfp = true; > + if (cpu_isar_feature(aa32_simd_r32, cpu)) { > + cpu->has_vfp_d32 = true; > /* > * The permitted values of the SIMDReg bits [3:0] on > * Armv8-A are either 0b0000 and 0b0010. On such CPUs, > * make sure that has_vfp_d32 can not be set to false. > */ > - if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) && > - !arm_feature(&cpu->env, ARM_FEATURE_M))) { > + if ((tcg_enabled() || qtest_enabled()) > + && !(arm_feature(&cpu->env, ARM_FEATURE_V8) > + && !arm_feature(&cpu->env, ARM_FEATURE_M))) { > qdev_property_add_static(DEVICE(obj), > &arm_cpu_has_vfp_d32_property); > }
> On 19 Jun 2023, at 16.02, Richard Henderson <richard.henderson@linaro.org> wrote: > > One cannot test for feature aa32_simd_r32 without first > testing if AArch32 mode is supported at all. This leads to > > qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither > > for Apple M1 cpus. > > We already have a check for ARMv8-A never setting vfp-d32 true, > so restructure the code so that AArch64 avoids the test entirely. > > Reported-by: Mads Ynddal <mads@ynddal.dk> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 353fc48567..706dbd37b1 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1402,25 +1402,27 @@ void arm_cpu_post_init(Object *obj) > * KVM does not currently allow us to lie to the guest about its > * ID/feature registers, so the guest always sees what the host has. > */ > - if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) > - ? cpu_isar_feature(aa64_fp_simd, cpu) > - : cpu_isar_feature(aa32_vfp, cpu)) { > - cpu->has_vfp = true; > - if (!kvm_enabled()) { > - qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property); > + if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { > + if (cpu_isar_feature(aa64_fp_simd, cpu)) { > + cpu->has_vfp = true; > + cpu->has_vfp_d32 = true; > + if (tcg_enabled() || qtest_enabled()) { > + qdev_property_add_static(DEVICE(obj), > + &arm_cpu_has_vfp_property); > + } > } > - } > - > - if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) { > - cpu->has_vfp_d32 = true; > - if (!kvm_enabled()) { > + } else if (cpu_isar_feature(aa32_vfp, cpu)) { > + cpu->has_vfp = true; > + if (cpu_isar_feature(aa32_simd_r32, cpu)) { > + cpu->has_vfp_d32 = true; > /* > * The permitted values of the SIMDReg bits [3:0] on > * Armv8-A are either 0b0000 and 0b0010. On such CPUs, > * make sure that has_vfp_d32 can not be set to false. > */ > - if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) && > - !arm_feature(&cpu->env, ARM_FEATURE_M))) { > + if ((tcg_enabled() || qtest_enabled()) > + && !(arm_feature(&cpu->env, ARM_FEATURE_V8) > + && !arm_feature(&cpu->env, ARM_FEATURE_M))) { > qdev_property_add_static(DEVICE(obj), > &arm_cpu_has_vfp_d32_property); > } > -- > 2.34.1 > Perfect! This seems to do it for Apple M1. Tested-by: Mads Ynddal <m.ynddal@samsung.com> Reviewed-by: Mads Ynddal <m.ynddal@samsung.com>
On 19/6/23 16:02, Richard Henderson wrote: > One cannot test for feature aa32_simd_r32 without first > testing if AArch32 mode is supported at all. This leads to > > qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither > > for Apple M1 cpus. > > We already have a check for ARMv8-A never setting vfp-d32 true, > so restructure the code so that AArch64 avoids the test entirely. > > Reported-by: Mads Ynddal <mads@ynddal.dk> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 353fc48567..706dbd37b1 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1402,25 +1402,27 @@ void arm_cpu_post_init(Object *obj) > * KVM does not currently allow us to lie to the guest about its > * ID/feature registers, so the guest always sees what the host has. > */ > - if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) > - ? cpu_isar_feature(aa64_fp_simd, cpu) > - : cpu_isar_feature(aa32_vfp, cpu)) { > - cpu->has_vfp = true; > - if (!kvm_enabled()) { > - qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property); > + if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { > + if (cpu_isar_feature(aa64_fp_simd, cpu)) { > + cpu->has_vfp = true; > + cpu->has_vfp_d32 = true; > + if (tcg_enabled() || qtest_enabled()) { > + qdev_property_add_static(DEVICE(obj), > + &arm_cpu_has_vfp_property); > + } > } > - } > - > - if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) { > - cpu->has_vfp_d32 = true; > - if (!kvm_enabled()) { > + } else if (cpu_isar_feature(aa32_vfp, cpu)) { > + cpu->has_vfp = true; > + if (cpu_isar_feature(aa32_simd_r32, cpu)) { > + cpu->has_vfp_d32 = true; > /* > * The permitted values of the SIMDReg bits [3:0] on > * Armv8-A are either 0b0000 and 0b0010. On such CPUs, > * make sure that has_vfp_d32 can not be set to false. > */ > - if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) && > - !arm_feature(&cpu->env, ARM_FEATURE_M))) { > + if ((tcg_enabled() || qtest_enabled()) > + && !(arm_feature(&cpu->env, ARM_FEATURE_V8) > + && !arm_feature(&cpu->env, ARM_FEATURE_M))) { > qdev_property_add_static(DEVICE(obj), > &arm_cpu_has_vfp_d32_property); > } Shouldn't we also change: -- >8 -- @@ -1431,7 +1431,7 @@ void arm_cpu_post_init(Object *obj) if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) { cpu->has_neon = true; - if (!kvm_enabled()) { + if (tcg_enabled() || qtest_enabled()) { qdev_property_add_static(DEVICE(obj), &arm_cpu_has_neon_property); } } --- ?
On 6/20/23 07:29, Philippe Mathieu-Daudé wrote: > On 19/6/23 16:02, Richard Henderson wrote: >> One cannot test for feature aa32_simd_r32 without first >> testing if AArch32 mode is supported at all. This leads to >> >> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither >> >> for Apple M1 cpus. >> >> We already have a check for ARMv8-A never setting vfp-d32 true, >> so restructure the code so that AArch64 avoids the test entirely. >> >> Reported-by: Mads Ynddal <mads@ynddal.dk> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/cpu.c | 28 +++++++++++++++------------- >> 1 file changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index 353fc48567..706dbd37b1 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -1402,25 +1402,27 @@ void arm_cpu_post_init(Object *obj) >> * KVM does not currently allow us to lie to the guest about its >> * ID/feature registers, so the guest always sees what the host has. >> */ >> - if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) >> - ? cpu_isar_feature(aa64_fp_simd, cpu) >> - : cpu_isar_feature(aa32_vfp, cpu)) { >> - cpu->has_vfp = true; >> - if (!kvm_enabled()) { >> - qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property); >> + if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { >> + if (cpu_isar_feature(aa64_fp_simd, cpu)) { >> + cpu->has_vfp = true; >> + cpu->has_vfp_d32 = true; >> + if (tcg_enabled() || qtest_enabled()) { >> + qdev_property_add_static(DEVICE(obj), >> + &arm_cpu_has_vfp_property); >> + } >> } >> - } >> - >> - if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) { >> - cpu->has_vfp_d32 = true; >> - if (!kvm_enabled()) { >> + } else if (cpu_isar_feature(aa32_vfp, cpu)) { >> + cpu->has_vfp = true; >> + if (cpu_isar_feature(aa32_simd_r32, cpu)) { >> + cpu->has_vfp_d32 = true; >> /* >> * The permitted values of the SIMDReg bits [3:0] on >> * Armv8-A are either 0b0000 and 0b0010. On such CPUs, >> * make sure that has_vfp_d32 can not be set to false. >> */ >> - if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) && >> - !arm_feature(&cpu->env, ARM_FEATURE_M))) { >> + if ((tcg_enabled() || qtest_enabled()) >> + && !(arm_feature(&cpu->env, ARM_FEATURE_V8) >> + && !arm_feature(&cpu->env, ARM_FEATURE_M))) { >> qdev_property_add_static(DEVICE(obj), >> &arm_cpu_has_vfp_d32_property); >> } > > Shouldn't we also change: > > -- >8 -- > @@ -1431,7 +1431,7 @@ void arm_cpu_post_init(Object *obj) > > if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) { > cpu->has_neon = true; > - if (!kvm_enabled()) { > + if (tcg_enabled() || qtest_enabled()) { > qdev_property_add_static(DEVICE(obj), &arm_cpu_has_neon_property); > } > } > --- > > ? Yes, I missed that, being just out of view. :-) r~
On Mon, 19 Jun 2023 at 15:02, Richard Henderson <richard.henderson@linaro.org> wrote: > > One cannot test for feature aa32_simd_r32 without first > testing if AArch32 mode is supported at all. This leads to > > qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither > > for Apple M1 cpus. > > We already have a check for ARMv8-A never setting vfp-d32 true, > so restructure the code so that AArch64 avoids the test entirely. > > Reported-by: Mads Ynddal <mads@ynddal.dk> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Applied to target-arm.next, thanks. (We can change the check in the "provide 'neon' property" code as a separate patch.) -- PMM
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 353fc48567..706dbd37b1 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1402,25 +1402,27 @@ void arm_cpu_post_init(Object *obj) * KVM does not currently allow us to lie to the guest about its * ID/feature registers, so the guest always sees what the host has. */ - if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) - ? cpu_isar_feature(aa64_fp_simd, cpu) - : cpu_isar_feature(aa32_vfp, cpu)) { - cpu->has_vfp = true; - if (!kvm_enabled()) { - qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property); + if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { + if (cpu_isar_feature(aa64_fp_simd, cpu)) { + cpu->has_vfp = true; + cpu->has_vfp_d32 = true; + if (tcg_enabled() || qtest_enabled()) { + qdev_property_add_static(DEVICE(obj), + &arm_cpu_has_vfp_property); + } } - } - - if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) { - cpu->has_vfp_d32 = true; - if (!kvm_enabled()) { + } else if (cpu_isar_feature(aa32_vfp, cpu)) { + cpu->has_vfp = true; + if (cpu_isar_feature(aa32_simd_r32, cpu)) { + cpu->has_vfp_d32 = true; /* * The permitted values of the SIMDReg bits [3:0] on * Armv8-A are either 0b0000 and 0b0010. On such CPUs, * make sure that has_vfp_d32 can not be set to false. */ - if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) && - !arm_feature(&cpu->env, ARM_FEATURE_M))) { + if ((tcg_enabled() || qtest_enabled()) + && !(arm_feature(&cpu->env, ARM_FEATURE_V8) + && !arm_feature(&cpu->env, ARM_FEATURE_M))) { qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_d32_property); }
One cannot test for feature aa32_simd_r32 without first testing if AArch32 mode is supported at all. This leads to qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither for Apple M1 cpus. We already have a check for ARMv8-A never setting vfp-d32 true, so restructure the code so that AArch64 avoids the test entirely. Reported-by: Mads Ynddal <mads@ynddal.dk> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)