Re: [PATCH 4/4] target/arm: adjust CPTR_EL2 according to HCR_EL2.E2H

[Richard Henderson]

On 12/6/20 10:46 PM, LIU Zhiwei wrote:
> From DDI0487Fc_armv8_arm.pdf, the CPTR_EL2 has two kinds
> of layouts according to HCR_EL2.E2H.
> 
> When HCR_EL2.E2H is 1, fp_exception_el should refer to
> HCR_EL2.FPEN and sve_exception_el should refer to HCR_EL2.ZEN.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>  target/arm/helper.c | 55 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 38cd35c049..6cc9f2bb50 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6147,11 +6147,30 @@ int sve_exception_el(CPUARMState *env, int el)
>       * they will be zero when EL2 is not present.
>       */
>      if (el <= 2 && !arm_is_secure_below_el3(env)) {
> -        if (env->cp15.cptr_el[2] & CPTR_TZ) {
> -            return 2;
> -        }
> -        if (env->cp15.cptr_el[2] & CPTR_TFP) {
> -            return 0;
> +        /* Since we exclude secure first, we may read HCR_EL2 directly. */
> +        if (env->cp15.hcr_el2 & HCR_E2H) {

We have already pulled the correct value into a local variable here: hcr_el2.
No need for the comment.

> +            int zen = extract32(env->cp15.cptr_el[2], 16, 2);
> +            switch (zen) {
> +            case 0:
> +            case 2:
> +                return 2;
> +            case 1:
> +                if (env->cp15.hcr_el2 & HCR_TGE) {

Likewise.

> +                    if (el == 0) {
> +                        return 2;
> +                    }
> +                }
> +                break;
> +            case 3:
> +                break;
> +            }
> +        } else {
> +            if (env->cp15.cptr_el[2] & CPTR_TZ) {
> +                return 2;
> +            }
> +            if (env->cp15.cptr_el[2] & CPTR_TFP) {
> +                return 0;
> +            }
>          }
>      }
>  
> @@ -12635,12 +12654,30 @@ int fp_exception_el(CPUARMState *env, int cur_el)
>       */
>  
>      /* CPTR_EL2 : present in v7VE or v8 */
> -    if (cur_el <= 2 && extract32(env->cp15.cptr_el[2], 10, 1)
> -        && !arm_is_secure_below_el3(env)) {
> +    if ((cur_el <= 2) && !arm_is_secure_below_el3(env)) {
>          /* Trap FP ops at EL2, NS-EL1 or NS-EL0 to EL2 */
> -        return 2;
> +        if ((arm_hcr_el2_eff(env) & HCR_E2H) == HCR_E2H) {

There is a prior use of arm_hcr_el2_eff in this function.  It should be called
once and stored in a local variable, just like in sve_exception_el.

No need for == in testing a single bit.

> +            int fpen = extract32(env->cp15.cptr_el[2], 20, 2);
> +            switch (fpen) {
> +            case 0:
> +            case 2:
> +                return 2;
> +            case 1:
> +                if ((arm_hcr_el2_eff(env) & HCR_TGE) == HCR_TGE) {

Likewise.

> +                    if (cur_el == 0) {
> +                        return 2;
> +                    }
> +                }
> +                break;
> +            case 3:
> +                break;
> +            }
> +        } else {
> +            if (extract32(env->cp15.cptr_el[2], 10, 1)) {

Use CPTR_TFP.

> +                return 2;
> +            }
> +        }
>      }
> -

Whitespace removal.

>      /* CPTR_EL3 : present in v8 */
>      if (extract32(env->cp15.cptr_el[3], 10, 1)) {
>          /* Trap all FP ops to EL3 */
> 

Would you also change this to use CPTR_TFP please?


r~