| Message ID | 20220517054850.177016-7-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: SME prep patches | expand |
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~
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);
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(-)