Re: [PATCH v4 35/40] target/arm: Update arm_cpu_do_interrupt_aarch64 for VHE

[Peter Maydell]

On Tue, 3 Dec 2019 at 02:30, Richard Henderson
<address@hidden> wrote:
>
> When VHE is enabled, we need to take the aa32-ness of EL0
> from PSTATE not HCR_EL2, which is controlling EL1.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/helper.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f2d18bd51a..f3785d5ad6 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8887,14 +8887,19 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>           * immediately lower than the target level is using AArch32 or 
> AArch64
>           */
>          bool is_aa64;
> +        uint64_t hcr;
>
>          switch (new_el) {
>          case 3:
>              is_aa64 = (env->cp15.scr_el3 & SCR_RW) != 0;
>              break;
>          case 2:
> -            is_aa64 = (env->cp15.hcr_el2 & HCR_RW) != 0;
> -            break;
> +            hcr = arm_hcr_el2_eff(env);
> +            if ((hcr & (HCR_E2H | HCR_TGE)) != (HCR_E2H | HCR_TGE)) {
> +                is_aa64 = (hcr & HCR_RW) != 0;
> +                break;
> +            }
> +            /* fall through */
>          case 1:
>              is_aa64 = is_a64(env);
>              break;
> --

The commit message is confusing me. Per the comment
in the code, what we're asking is "is the EL immediately
lower than the target level using AArch64?". We never
took the aa32ness of EL0 from HCR_EL2: that code is
looking at "what's the aa32ness of EL1", because in a non-VHE
setup EL1 is always the EL immediately lower than EL2.

So I *think* what the code is doing is:

 When VHE is enabled, the exception level below EL2 is
 not EL1, but EL0, and so to identify the entry vector
 offset for exceptions targeting EL2 we need to look
 at the width of EL0, not of EL1.

Is that right?

thanks
-- PMM