[PATCH] target/arm: Restructure has_vfp_d32 test

[PATCH] target/arm: Restructure has_vfp_d32 test

[Richard Henderson]

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

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Philippe Mathieu-Daudé]

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>

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Richard Henderson]

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~

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Cédric Le Goater]

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);
>               }

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Mads Ynddal]

> 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>

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Philippe Mathieu-Daudé]

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);
          }
      }
---

?

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Richard Henderson]

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~

Re: [PATCH] target/arm: Restructure has_vfp_d32 test

[Peter Maydell]

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