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 <richard.henderson@linaro.org>
---
 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;
-- 
2.17.1

On Tue, 3 Dec 2019 at 02:30, Richard Henderson
<richard.henderson@linaro.org> 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 <richard.henderson@linaro.org>
> ---
>  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

On 12/6/19 8:03 AM, Peter Maydell wrote:
> 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?

Correct.  Much better wording, thanks.  Will update.


r~

On Fri, 6 Dec 2019 at 18:51, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/6/19 8:03 AM, Peter Maydell wrote:
> > 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?
>
> Correct.  Much better wording, thanks.  Will update.

In that case
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM