* Re: [PATCH] target/arm: Restructure has_vfp_d32 test
2023-06-19 14:02 [PATCH] target/arm: Restructure has_vfp_d32 test Richard Henderson
@ 2023-06-19 16:07 ` Philippe Mathieu-Daudé
2023-06-19 16:59 ` Richard Henderson
2023-06-19 17:36 ` Cédric Le Goater
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-19 16:07 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm, Mads Ynddal
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>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/arm: Restructure has_vfp_d32 test
2023-06-19 14:02 [PATCH] target/arm: Restructure has_vfp_d32 test Richard Henderson
2023-06-19 16:07 ` Philippe Mathieu-Daudé
@ 2023-06-19 17:36 ` Cédric Le Goater
2023-06-19 19:58 ` Mads Ynddal
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2023-06-19 17:36 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm, Mads Ynddal
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);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/arm: Restructure has_vfp_d32 test
2023-06-19 14:02 [PATCH] target/arm: Restructure has_vfp_d32 test Richard Henderson
2023-06-19 16:07 ` Philippe Mathieu-Daudé
2023-06-19 17:36 ` Cédric Le Goater
@ 2023-06-19 19:58 ` Mads Ynddal
2023-06-20 5:29 ` Philippe Mathieu-Daudé
2023-06-23 10:27 ` Peter Maydell
4 siblings, 0 replies; 8+ messages in thread
From: Mads Ynddal @ 2023-06-19 19:58 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm
> 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>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/arm: Restructure has_vfp_d32 test
2023-06-19 14:02 [PATCH] target/arm: Restructure has_vfp_d32 test Richard Henderson
` (2 preceding siblings ...)
2023-06-19 19:58 ` Mads Ynddal
@ 2023-06-20 5:29 ` Philippe Mathieu-Daudé
2023-06-20 7:19 ` Richard Henderson
2023-06-23 10:27 ` Peter Maydell
4 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-20 5:29 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm, Mads Ynddal
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);
}
}
---
?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/arm: Restructure has_vfp_d32 test
2023-06-20 5:29 ` Philippe Mathieu-Daudé
@ 2023-06-20 7:19 ` Richard Henderson
0 siblings, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2023-06-20 7:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: qemu-arm, Mads Ynddal
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~
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] target/arm: Restructure has_vfp_d32 test
2023-06-19 14:02 [PATCH] target/arm: Restructure has_vfp_d32 test Richard Henderson
` (3 preceding siblings ...)
2023-06-20 5:29 ` Philippe Mathieu-Daudé
@ 2023-06-23 10:27 ` Peter Maydell
4 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2023-06-23 10:27 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm, Mads Ynddal
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
^ permalink raw reply [flat|nested] 8+ messages in thread