Instead of checking these bits in fp_exception_el and
also in sve_exception_el, document that we must compare
the results.  The only place where we have not already
checked that FP EL is zero is in rebuild_hflags_a64.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 52 ++++++++++++++-------------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index edeab4e473..05baa73986 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6136,10 +6136,12 @@ static const ARMCPRegInfo minimal_ras_reginfo[] = {
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.vsesr_el2) },
 };
 
-/* Return the exception level to which exceptions should be taken
- * via SVEAccessTrap.  If an exception should be routed through
- * AArch64.AdvSIMDFPAccessTrap, return 0; fp_exception_el should
- * take care of raising that exception.
+/*
+ * Return the exception level to which exceptions should be taken
+ * via SVEAccessTrap.  This excludes the check for whether the exception
+ * should be routed through AArch64.AdvSIMDFPAccessTrap.  That can easily
+ * be found by testing 0 < fp_exception_el <= sve_exception_el.
+ *
  * C.f. the ARM pseudocode function CheckSVEEnabled.
  */
 int sve_exception_el(CPUARMState *env, int el)
@@ -6159,18 +6161,6 @@ int sve_exception_el(CPUARMState *env, int el)
             /* route_to_el2 */
             return hcr_el2 & HCR_TGE ? 2 : 1;
         }
-
-        /* Check CPACR.FPEN.  */
-        switch (FIELD_EX64(env->cp15.cpacr_el1, CPACR_EL1, FPEN)) {
-        case 1:
-            if (el != 0) {
-                break;
-            }
-            /* fall through */
-        case 0:
-        case 2:
-            return 0;
-        }
     }
 
     /*
@@ -6188,24 +6178,10 @@ int sve_exception_el(CPUARMState *env, int el)
             case 2:
                 return 2;
             }
-
-            switch (FIELD_EX32(env->cp15.cptr_el[2], CPTR_EL2, FPEN)) {
-            case 1:
-                if (el == 2 || !(hcr_el2 & HCR_TGE)) {
-                    break;
-                }
-                /* fall through */
-            case 0:
-            case 2:
-                return 0;
-            }
         } else if (arm_is_el2_enabled(env)) {
             if (FIELD_EX64(env->cp15.cptr_el[2], CPTR_EL2, TZ)) {
                 return 2;
             }
-            if (FIELD_EX64(env->cp15.cptr_el[2], CPTR_EL2, TFP)) {
-                return 0;
-            }
         }
     }
 
@@ -13541,15 +13517,19 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
 
     if (cpu_isar_feature(aa64_sve, env_archcpu(env))) {
         int sve_el = sve_exception_el(env, el);
-        uint32_t zcr_len;
+        uint32_t zcr_len = 0;
 
         /*
-         * If SVE is disabled, but FP is enabled,
-         * then the effective len is 0.
+         * If either FP or SVE are disabled, translator does not need len.
+         * If SVE EL >= FP EL, FP exception has precedence, and translator
+         * does not need SVE EL.  Save potential re-translations by forcing
+         * the unneeded data to zero.
          */
-        if (sve_el != 0 && fp_el == 0) {
-            zcr_len = 0;
-        } else {
+        if (fp_el != 0) {
+            if (sve_el >= fp_el) {
+                sve_el = 0;
+            }
+        } else if (sve_el == 0) {
             zcr_len = sve_zcr_len_for_el(env, el);
         }
         DP_TBFLAG_A64(flags, SVEEXC_EL, sve_el);
-- 
2.34.1

On Tue, 17 May 2022 at 07:00, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Instead of checking these bits in fp_exception_el and
> also in sve_exception_el, document that we must compare
> the results.  The only place where we have not already
> checked that FP EL is zero is in rebuild_hflags_a64.

aarch64_cpu_dump_state() calls sve_exception_el() and doesn't
check against the FP exception EL.

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 52 ++++++++++++++-------------------------------
>  1 file changed, 16 insertions(+), 36 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index edeab4e473..05baa73986 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6136,10 +6136,12 @@ static const ARMCPRegInfo minimal_ras_reginfo[] = {
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.vsesr_el2) },
>  };
>
> -/* Return the exception level to which exceptions should be taken
> - * via SVEAccessTrap.  If an exception should be routed through
> - * AArch64.AdvSIMDFPAccessTrap, return 0; fp_exception_el should
> - * take care of raising that exception.
> +/*
> + * Return the exception level to which exceptions should be taken
> + * via SVEAccessTrap.  This excludes the check for whether the exception
> + * should be routed through AArch64.AdvSIMDFPAccessTrap.  That can easily
> + * be found by testing 0 < fp_exception_el <= sve_exception_el.
> + *
>   * C.f. the ARM pseudocode function CheckSVEEnabled.

We should probably note that the pseudocode does *not* separate
out the FP trap checks, but has them all in the same function.

>   */

>          /*
> -         * If SVE is disabled, but FP is enabled,
> -         * then the effective len is 0.
> +         * If either FP or SVE are disabled, translator does not need len.
> +         * If SVE EL >= FP EL, FP exception has precedence, and translator
> +         * does not need SVE EL.  Save potential re-translations by forcing
> +         * the unneeded data to zero.
>           */

These comments say that if SVE EL == FP EL then the FP exception
has precedence, but looking at the pseudocode CheckNormalSVEEnabled()
it seems to be the other way around:  eg if CPACR_EL1 has
"disabled at EL0" for both the .ZEN bits and the .FPEN bits
then the SVEAccessTrap() is handled first.

thanks
-- PMM

On 5/19/22 04:36, Peter Maydell wrote:
> On Tue, 17 May 2022 at 07:00, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Instead of checking these bits in fp_exception_el and
>> also in sve_exception_el, document that we must compare
>> the results.  The only place where we have not already
>> checked that FP EL is zero is in rebuild_hflags_a64.
> 
> aarch64_cpu_dump_state() calls sve_exception_el() and doesn't
> check against the FP exception EL.

Yes it does, just 6 lines above -- fp_exc == 0.

>> -/* Return the exception level to which exceptions should be taken
>> - * via SVEAccessTrap.  If an exception should be routed through
>> - * AArch64.AdvSIMDFPAccessTrap, return 0; fp_exception_el should
>> - * take care of raising that exception.
>> +/*
>> + * Return the exception level to which exceptions should be taken
>> + * via SVEAccessTrap.  This excludes the check for whether the exception
>> + * should be routed through AArch64.AdvSIMDFPAccessTrap.  That can easily
>> + * be found by testing 0 < fp_exception_el <= sve_exception_el.
>> + *
>>    * C.f. the ARM pseudocode function CheckSVEEnabled.
> 
> We should probably note that the pseudocode does *not* separate
> out the FP trap checks, but has them all in the same function.

Sure.

>>           /*
>> -         * If SVE is disabled, but FP is enabled,
>> -         * then the effective len is 0.
>> +         * If either FP or SVE are disabled, translator does not need len.
>> +         * If SVE EL >= FP EL, FP exception has precedence, and translator
>> +         * does not need SVE EL.  Save potential re-translations by forcing
>> +         * the unneeded data to zero.
>>            */
> 
> These comments say that if SVE EL == FP EL then the FP exception
> has precedence, but looking at the pseudocode CheckNormalSVEEnabled()
> it seems to be the other way around:  eg if CPACR_EL1 has
> "disabled at EL0" for both the .ZEN bits and the .FPEN bits
> then the SVEAccessTrap() is handled first.
Yep.


r~