diff mbox series

[PULL,08/30] target/arm: Add ptw_idx to S1Translate

Message ID 20221025163952.4131046-9-peter.maydell@linaro.org
State Accepted
Commit 48da29e485af5c92e284fbb6ef279b2a98eea7c4
Headers show
Series [PULL,01/30] target/arm: Implement FEAT_E0PD | expand

Commit Message

Peter Maydell Oct. 25, 2022, 4:39 p.m. UTC
From: Richard Henderson <richard.henderson@linaro.org>

Hoist the computation of the mmu_idx for the ptw up to
get_phys_addr_with_struct and get_phys_addr_twostage.
This removes the duplicate check for stage2 disabled
from the middle of the walk, performing it only once.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20221024051851.3074715-3-richard.henderson@linaro.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 71 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 17 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 31, 2022, 11:14 p.m. UTC | #1
On 25/10/22 18:39, Peter Maydell wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Hoist the computation of the mmu_idx for the ptw up to
> get_phys_addr_with_struct and get_phys_addr_twostage.
> This removes the duplicate check for stage2 disabled
> from the middle of the walk, performing it only once.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Message-id: 20221024051851.3074715-3-richard.henderson@linaro.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 71 ++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 54 insertions(+), 17 deletions(-)
> 
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 32d64125865..3c153f68318 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -17,6 +17,7 @@
>   
>   typedef struct S1Translate {
>       ARMMMUIdx in_mmu_idx;
> +    ARMMMUIdx in_ptw_idx;
>       bool in_secure;
>       bool in_debug;
>       bool out_secure;
> @@ -214,33 +215,24 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
>   {
>       bool is_secure = ptw->in_secure;
>       ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
> -    ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> -    bool s2_phys = false;
> +    ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
>       uint8_t pte_attrs;
>       bool pte_secure;
>   
> -    if (!arm_mmu_idx_is_stage1_of_2(mmu_idx)
> -        || regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
> -        s2_mmu_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
> -        s2_phys = true;
> -    }
> -
>       if (unlikely(ptw->in_debug)) {
>           /*
>            * From gdbstub, do not use softmmu so that we don't modify the
>            * state of the cpu at all, including softmmu tlb contents.
>            */
> -        if (s2_phys) {
> -            ptw->out_phys = addr;
> -            pte_attrs = 0;
> -            pte_secure = is_secure;
> -        } else {
> +        if (regime_is_stage2(s2_mmu_idx)) {
>               S1Translate s2ptw = {
>                   .in_mmu_idx = s2_mmu_idx,
> +                .in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS,
>                   .in_secure = is_secure,
>                   .in_debug = true,
>               };
>               GetPhysAddrResult s2 = { };
> +
>               if (!get_phys_addr_lpae(env, &s2ptw, addr, MMU_DATA_LOAD,
>                                       false, &s2, fi)) {
>                   goto fail;
> @@ -248,6 +240,11 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
>               ptw->out_phys = s2.f.phys_addr;
>               pte_attrs = s2.cacheattrs.attrs;
>               pte_secure = s2.f.attrs.secure;
> +        } else {
> +            /* Regime is physical. */
> +            ptw->out_phys = addr;
> +            pte_attrs = 0;
> +            pte_secure = is_secure;
>           }
>           ptw->out_host = NULL;
>       } else {
> @@ -268,7 +265,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
>           pte_secure = full->attrs.secure;
>       }
>   
> -    if (!s2_phys) {
> +    if (regime_is_stage2(s2_mmu_idx)) {
>           uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
>   
>           if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
> @@ -1263,7 +1260,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>           descaddr |= (address >> (stride * (4 - level))) & indexmask;
>           descaddr &= ~7ULL;
>           nstable = extract32(tableattrs, 4, 1);
> -        ptw->in_secure = !nstable;
> +        if (!nstable) {
> +            /*
> +             * Stage2_S -> Stage2 or Phys_S -> Phys_NS
> +             * Assert that the non-secure idx are even, and relative order.
> +             */
> +            QEMU_BUILD_BUG_ON((ARMMMUIdx_Phys_NS & 1) != 0);
> +            QEMU_BUILD_BUG_ON((ARMMMUIdx_Stage2 & 1) != 0);
> +            QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_NS + 1 != ARMMMUIdx_Phys_S);
> +            QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2 + 1 != ARMMMUIdx_Stage2_S);
> +            ptw->in_ptw_idx &= ~1;
> +            ptw->in_secure = false;
> +        }
>           descriptor = arm_ldq_ptw(env, ptw, descaddr, fi);
>           if (fi->type != ARMFault_None) {
>               goto do_fault;
> @@ -2449,6 +2457,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
>   
>       is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
>       ptw->in_mmu_idx = s2walk_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> +    ptw->in_ptw_idx = s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
>       ptw->in_secure = s2walk_secure;
>   
>       /*
> @@ -2508,10 +2517,32 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>                                         ARMMMUFaultInfo *fi)
>   {
>       ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
> -    ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
>       bool is_secure = ptw->in_secure;
> +    ARMMMUIdx s1_mmu_idx;
>   
> -    if (mmu_idx != s1_mmu_idx) {
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_Phys_S:
> +    case ARMMMUIdx_Phys_NS:
> +        /* Checking Phys early avoids special casing later vs regime_el. */
> +        return get_phys_addr_disabled(env, address, access_type, mmu_idx,
> +                                      is_secure, result, fi);
> +
> +    case ARMMMUIdx_Stage1_E0:
> +    case ARMMMUIdx_Stage1_E1:
> +    case ARMMMUIdx_Stage1_E1_PAN:
> +        /* First stage lookup uses second stage for ptw. */
> +        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> +        break;
> +
> +    case ARMMMUIdx_E10_0:
> +        s1_mmu_idx = ARMMMUIdx_Stage1_E0;
> +        goto do_twostage;
> +    case ARMMMUIdx_E10_1:
> +        s1_mmu_idx = ARMMMUIdx_Stage1_E1;
> +        goto do_twostage;
> +    case ARMMMUIdx_E10_1_PAN:
> +        s1_mmu_idx = ARMMMUIdx_Stage1_E1_PAN;
> +    do_twostage:
>           /*
>            * Call ourselves recursively to do the stage 1 and then stage 2
>            * translations if mmu_idx is a two-stage regime, and EL2 present.
> @@ -2522,6 +2553,12 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>               return get_phys_addr_twostage(env, ptw, address, access_type,
>                                             result, fi);
>           }
> +        /* fall through */
> +
> +    default:
> +        /* Single stage and second stage uses physical for ptw. */
> +        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
> +        break;
>       }

Since this commit I can not boot Trusted Firmware on the SBSA-ref machine.

--- ok  2022-10-31 23:11:48.131829719 +0000
+++ bad 2022-10-31 23:11:27.210091574 +0000
@@ -176,68 +176,68 @@
-IN:
-0x000000003fcd4b14:
-OBJD-T: 030000d4
-
-Taking exception 13 [Secure Monitor Call] on CPU 0
-...from EL1 to EL3
-...with ESR 0x17/0x5e000000
-...with ELR 0x3fcd4b18
-...to EL3 PC 0x3400 PSTATE 0x3cd
+Taking exception 3 [Prefetch Abort] on CPU 0
+...from EL3 to EL3
+...with ESR 0x21/0x86000004
+...with FAR 0x28b8
+...with ELR 0x28b8
+...to EL3 PC 0x3000 PSTATE 0x3cd
Philippe Mathieu-Daudé Nov. 1, 2022, 10:10 a.m. UTC | #2
On 1/11/22 00:14, Philippe Mathieu-Daudé wrote:
> On 25/10/22 18:39, Peter Maydell wrote:
>> From: Richard Henderson <richard.henderson@linaro.org>
>>
>> Hoist the computation of the mmu_idx for the ptw up to
>> get_phys_addr_with_struct and get_phys_addr_twostage.
>> This removes the duplicate check for stage2 disabled
>> from the middle of the walk, performing it only once.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-id: 20221024051851.3074715-3-richard.henderson@linaro.org
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   target/arm/ptw.c | 71 ++++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 54 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
>> index 32d64125865..3c153f68318 100644
>> --- a/target/arm/ptw.c
>> +++ b/target/arm/ptw.c
>> @@ -17,6 +17,7 @@
>>   typedef struct S1Translate {
>>       ARMMMUIdx in_mmu_idx;
>> +    ARMMMUIdx in_ptw_idx;
>>       bool in_secure;
>>       bool in_debug;
>>       bool out_secure;
>> @@ -214,33 +215,24 @@ static bool S1_ptw_translate(CPUARMState *env, 
>> S1Translate *ptw,
>>   {
>>       bool is_secure = ptw->in_secure;
>>       ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
>> -    ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : 
>> ARMMMUIdx_Stage2;
>> -    bool s2_phys = false;
>> +    ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
>>       uint8_t pte_attrs;
>>       bool pte_secure;
>> -    if (!arm_mmu_idx_is_stage1_of_2(mmu_idx)
>> -        || regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
>> -        s2_mmu_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
>> -        s2_phys = true;
>> -    }
>> -
>>       if (unlikely(ptw->in_debug)) {
>>           /*
>>            * From gdbstub, do not use softmmu so that we don't modify the
>>            * state of the cpu at all, including softmmu tlb contents.
>>            */
>> -        if (s2_phys) {
>> -            ptw->out_phys = addr;
>> -            pte_attrs = 0;
>> -            pte_secure = is_secure;
>> -        } else {
>> +        if (regime_is_stage2(s2_mmu_idx)) {
>>               S1Translate s2ptw = {
>>                   .in_mmu_idx = s2_mmu_idx,
>> +                .in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : 
>> ARMMMUIdx_Phys_NS,
>>                   .in_secure = is_secure,
>>                   .in_debug = true,
>>               };
>>               GetPhysAddrResult s2 = { };
>> +
>>               if (!get_phys_addr_lpae(env, &s2ptw, addr, MMU_DATA_LOAD,
>>                                       false, &s2, fi)) {
>>                   goto fail;
>> @@ -248,6 +240,11 @@ static bool S1_ptw_translate(CPUARMState *env, 
>> S1Translate *ptw,
>>               ptw->out_phys = s2.f.phys_addr;
>>               pte_attrs = s2.cacheattrs.attrs;
>>               pte_secure = s2.f.attrs.secure;
>> +        } else {
>> +            /* Regime is physical. */
>> +            ptw->out_phys = addr;
>> +            pte_attrs = 0;
>> +            pte_secure = is_secure;
>>           }
>>           ptw->out_host = NULL;
>>       } else {
>> @@ -268,7 +265,7 @@ static bool S1_ptw_translate(CPUARMState *env, 
>> S1Translate *ptw,
>>           pte_secure = full->attrs.secure;
>>       }
>> -    if (!s2_phys) {
>> +    if (regime_is_stage2(s2_mmu_idx)) {
>>           uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
>>           if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
>> @@ -1263,7 +1260,18 @@ static bool get_phys_addr_lpae(CPUARMState 
>> *env, S1Translate *ptw,
>>           descaddr |= (address >> (stride * (4 - level))) & indexmask;
>>           descaddr &= ~7ULL;
>>           nstable = extract32(tableattrs, 4, 1);
>> -        ptw->in_secure = !nstable;
>> +        if (!nstable) {
>> +            /*
>> +             * Stage2_S -> Stage2 or Phys_S -> Phys_NS
>> +             * Assert that the non-secure idx are even, and relative 
>> order.
>> +             */
>> +            QEMU_BUILD_BUG_ON((ARMMMUIdx_Phys_NS & 1) != 0);
>> +            QEMU_BUILD_BUG_ON((ARMMMUIdx_Stage2 & 1) != 0);
>> +            QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_NS + 1 != 
>> ARMMMUIdx_Phys_S);
>> +            QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2 + 1 != 
>> ARMMMUIdx_Stage2_S);
>> +            ptw->in_ptw_idx &= ~1;
>> +            ptw->in_secure = false;
>> +        }
>>           descriptor = arm_ldq_ptw(env, ptw, descaddr, fi);
>>           if (fi->type != ARMFault_None) {
>>               goto do_fault;
>> @@ -2449,6 +2457,7 @@ static bool get_phys_addr_twostage(CPUARMState 
>> *env, S1Translate *ptw,
>>       is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
>>       ptw->in_mmu_idx = s2walk_secure ? ARMMMUIdx_Stage2_S : 
>> ARMMMUIdx_Stage2;
>> +    ptw->in_ptw_idx = s2walk_secure ? ARMMMUIdx_Phys_S : 
>> ARMMMUIdx_Phys_NS;
>>       ptw->in_secure = s2walk_secure;
>>       /*
>> @@ -2508,10 +2517,32 @@ static bool 
>> get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>>                                         ARMMMUFaultInfo *fi)
>>   {
>>       ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
>> -    ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
>>       bool is_secure = ptw->in_secure;
>> +    ARMMMUIdx s1_mmu_idx;
>> -    if (mmu_idx != s1_mmu_idx) {
>> +    switch (mmu_idx) {
>> +    case ARMMMUIdx_Phys_S:
>> +    case ARMMMUIdx_Phys_NS:
>> +        /* Checking Phys early avoids special casing later vs 
>> regime_el. */
>> +        return get_phys_addr_disabled(env, address, access_type, 
>> mmu_idx,
>> +                                      is_secure, result, fi);
>> +
>> +    case ARMMMUIdx_Stage1_E0:
>> +    case ARMMMUIdx_Stage1_E1:
>> +    case ARMMMUIdx_Stage1_E1_PAN:
>> +        /* First stage lookup uses second stage for ptw. */
>> +        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : 
>> ARMMMUIdx_Stage2;
>> +        break;
>> +
>> +    case ARMMMUIdx_E10_0:
>> +        s1_mmu_idx = ARMMMUIdx_Stage1_E0;
>> +        goto do_twostage;
>> +    case ARMMMUIdx_E10_1:
>> +        s1_mmu_idx = ARMMMUIdx_Stage1_E1;
>> +        goto do_twostage;
>> +    case ARMMMUIdx_E10_1_PAN:
>> +        s1_mmu_idx = ARMMMUIdx_Stage1_E1_PAN;
>> +    do_twostage:
>>           /*
>>            * Call ourselves recursively to do the stage 1 and then 
>> stage 2
>>            * translations if mmu_idx is a two-stage regime, and EL2 
>> present.
>> @@ -2522,6 +2553,12 @@ static bool 
>> get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>>               return get_phys_addr_twostage(env, ptw, address, 
>> access_type,
>>                                             result, fi);
>>           }
>> +        /* fall through */
>> +
>> +    default:
>> +        /* Single stage and second stage uses physical for ptw. */
>> +        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : 
>> ARMMMUIdx_Phys_NS;
>> +        break;
>>       }
> 
> Since this commit I can not boot Trusted Firmware on the SBSA-ref machine.

Do we need to set in_ptw_idx in get_phys_addr_with_secure()?
Philippe Mathieu-Daudé Nov. 1, 2022, 4:57 p.m. UTC | #3
On 1/11/22 11:10, Philippe Mathieu-Daudé wrote:
> On 1/11/22 00:14, Philippe Mathieu-Daudé wrote:
>> On 25/10/22 18:39, Peter Maydell wrote:
>>> From: Richard Henderson <richard.henderson@linaro.org>
>>>
>>> Hoist the computation of the mmu_idx for the ptw up to
>>> get_phys_addr_with_struct and get_phys_addr_twostage.
>>> This removes the duplicate check for stage2 disabled
>>> from the middle of the walk, performing it only once.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-id: 20221024051851.3074715-3-richard.henderson@linaro.org
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>   target/arm/ptw.c | 71 ++++++++++++++++++++++++++++++++++++------------
>>>   1 file changed, 54 insertions(+), 17 deletions(-)

>> Since this commit I can not boot Trusted Firmware on the SBSA-ref 
>> machine.
> 
> Do we need to set in_ptw_idx in get_phys_addr_with_secure()?

I opened https://gitlab.com/qemu-project/qemu/-/issues/1293 to track.
diff mbox series

Patch

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 32d64125865..3c153f68318 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -17,6 +17,7 @@ 
 
 typedef struct S1Translate {
     ARMMMUIdx in_mmu_idx;
+    ARMMMUIdx in_ptw_idx;
     bool in_secure;
     bool in_debug;
     bool out_secure;
@@ -214,33 +215,24 @@  static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
 {
     bool is_secure = ptw->in_secure;
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
-    ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
-    bool s2_phys = false;
+    ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
     uint8_t pte_attrs;
     bool pte_secure;
 
-    if (!arm_mmu_idx_is_stage1_of_2(mmu_idx)
-        || regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
-        s2_mmu_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
-        s2_phys = true;
-    }
-
     if (unlikely(ptw->in_debug)) {
         /*
          * From gdbstub, do not use softmmu so that we don't modify the
          * state of the cpu at all, including softmmu tlb contents.
          */
-        if (s2_phys) {
-            ptw->out_phys = addr;
-            pte_attrs = 0;
-            pte_secure = is_secure;
-        } else {
+        if (regime_is_stage2(s2_mmu_idx)) {
             S1Translate s2ptw = {
                 .in_mmu_idx = s2_mmu_idx,
+                .in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS,
                 .in_secure = is_secure,
                 .in_debug = true,
             };
             GetPhysAddrResult s2 = { };
+
             if (!get_phys_addr_lpae(env, &s2ptw, addr, MMU_DATA_LOAD,
                                     false, &s2, fi)) {
                 goto fail;
@@ -248,6 +240,11 @@  static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
             ptw->out_phys = s2.f.phys_addr;
             pte_attrs = s2.cacheattrs.attrs;
             pte_secure = s2.f.attrs.secure;
+        } else {
+            /* Regime is physical. */
+            ptw->out_phys = addr;
+            pte_attrs = 0;
+            pte_secure = is_secure;
         }
         ptw->out_host = NULL;
     } else {
@@ -268,7 +265,7 @@  static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
         pte_secure = full->attrs.secure;
     }
 
-    if (!s2_phys) {
+    if (regime_is_stage2(s2_mmu_idx)) {
         uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
 
         if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
@@ -1263,7 +1260,18 @@  static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
         descaddr |= (address >> (stride * (4 - level))) & indexmask;
         descaddr &= ~7ULL;
         nstable = extract32(tableattrs, 4, 1);
-        ptw->in_secure = !nstable;
+        if (!nstable) {
+            /*
+             * Stage2_S -> Stage2 or Phys_S -> Phys_NS
+             * Assert that the non-secure idx are even, and relative order.
+             */
+            QEMU_BUILD_BUG_ON((ARMMMUIdx_Phys_NS & 1) != 0);
+            QEMU_BUILD_BUG_ON((ARMMMUIdx_Stage2 & 1) != 0);
+            QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_NS + 1 != ARMMMUIdx_Phys_S);
+            QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2 + 1 != ARMMMUIdx_Stage2_S);
+            ptw->in_ptw_idx &= ~1;
+            ptw->in_secure = false;
+        }
         descriptor = arm_ldq_ptw(env, ptw, descaddr, fi);
         if (fi->type != ARMFault_None) {
             goto do_fault;
@@ -2449,6 +2457,7 @@  static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
 
     is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
     ptw->in_mmu_idx = s2walk_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
+    ptw->in_ptw_idx = s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
     ptw->in_secure = s2walk_secure;
 
     /*
@@ -2508,10 +2517,32 @@  static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
                                       ARMMMUFaultInfo *fi)
 {
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
-    ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
     bool is_secure = ptw->in_secure;
+    ARMMMUIdx s1_mmu_idx;
 
-    if (mmu_idx != s1_mmu_idx) {
+    switch (mmu_idx) {
+    case ARMMMUIdx_Phys_S:
+    case ARMMMUIdx_Phys_NS:
+        /* Checking Phys early avoids special casing later vs regime_el. */
+        return get_phys_addr_disabled(env, address, access_type, mmu_idx,
+                                      is_secure, result, fi);
+
+    case ARMMMUIdx_Stage1_E0:
+    case ARMMMUIdx_Stage1_E1:
+    case ARMMMUIdx_Stage1_E1_PAN:
+        /* First stage lookup uses second stage for ptw. */
+        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
+        break;
+
+    case ARMMMUIdx_E10_0:
+        s1_mmu_idx = ARMMMUIdx_Stage1_E0;
+        goto do_twostage;
+    case ARMMMUIdx_E10_1:
+        s1_mmu_idx = ARMMMUIdx_Stage1_E1;
+        goto do_twostage;
+    case ARMMMUIdx_E10_1_PAN:
+        s1_mmu_idx = ARMMMUIdx_Stage1_E1_PAN;
+    do_twostage:
         /*
          * Call ourselves recursively to do the stage 1 and then stage 2
          * translations if mmu_idx is a two-stage regime, and EL2 present.
@@ -2522,6 +2553,12 @@  static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
             return get_phys_addr_twostage(env, ptw, address, access_type,
                                           result, fi);
         }
+        /* fall through */
+
+    default:
+        /* Single stage and second stage uses physical for ptw. */
+        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
+        break;
     }
 
     /*