Re: [PATCH v3 30/42] target/arm: Add ptw_idx argument to S1_ptw_translate

2022-10-07 Thread Richard Henderson 

On 10/7/22 02:19, Peter Maydell wrote:

I don't think this works, because the s2_mmu_idx is not necessarily
the same through the whole of a page table walk. See the comment in
get_phys_addr_lpae():
 /*
  * Secure accesses start with the page table in secure memory and
  * can be downgraded to non-secure at any step. Non-secure accesses
  * remain non-secure. We implement this by just ORing in the NSTable/NS
  * bits at each step.
  */

Currently get_phys_addr_lpae() updates the nstable bit in tableattrs and
passes that to arm_ldq_ptw() for each level of the page tables, which in
turn causes S1_ptw_translate() to select ARMMMUIdx_Stage2_S or ARMMMUIdx_Stage2.


Ouch.  I had missed this subtlety.

We could play lsb games with the mmu_idx itself, knowing that we have either 
ARMMMUIdx_{Stage2,Phys}_S and generate ARMMMUIdx_{Stage2,Phys}.  I'll have another good 
long look at this.




  if (regime_translation_disabled(env, mmu_idx, is_secure)) {
-return get_phys_addr_disabled(env, address, access_type, mmu_idx,
-  is_secure, result, fi);
+goto do_disabled;
  }


I'd prefer to avoid this goto back up into the middle of an unrelated
switch statement.


Oops, I guess I missed this one when I went back through to eliminate the gotos.


r~

Re: [PATCH v3 30/42] target/arm: Add ptw_idx argument to S1_ptw_translate

2022-10-07 Thread Peter Maydell 
On Sat, 1 Oct 2022 at 17:42, Richard Henderson
 wrote:
>
> Hoist the computation of the mmu_idx for the ptw up to
> get_phys_addr_with_secure_debug and get_phys_addr_twostage.
> This removes the duplicate check for stage2 disabled
> from the middle of the walk, performing it only once.
>
> Pass ptw_idx through get_phys_addr_{v5,v6,lpae} and arm_{ldl,ldq}_ptw.
>
> Signed-off-by: Richard Henderson 



>  /* Translate a S1 pagetable walk through S2 if needed.  */
> -static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx, hwaddr 
> addr,
> +static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
> + ARMMMUIdx s2_mmu_idx, hwaddr addr,
>   bool *is_secure_ptr, void **hphys, hwaddr 
> *gphys,
>   bool debug, ARMMMUFaultInfo *fi)
>  {
>  bool is_secure = *is_secure_ptr;
> -ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> -bool s2_phys = false;

I don't think this works, because the s2_mmu_idx is not necessarily
the same through the whole of a page table walk. See the comment in
get_phys_addr_lpae():
/*
 * Secure accesses start with the page table in secure memory and
 * can be downgraded to non-secure at any step. Non-secure accesses
 * remain non-secure. We implement this by just ORing in the NSTable/NS
 * bits at each step.
 */

Currently get_phys_addr_lpae() updates the nstable bit in tableattrs and
passes that to arm_ldq_ptw() for each level of the page tables, which in
turn causes S1_ptw_translate() to select ARMMMUIdx_Stage2_S or ARMMMUIdx_Stage2.

Alternatively, maybe our existing behaviour is a bug -- but then we need
to separate out the bug fix from the refactoring patch.

> @@ -2604,18 +2643,17 @@ static bool 
> get_phys_addr_with_secure_debug(CPUARMState *env,
>  /* Definitely a real MMU, not an MPU */
>
>  if (regime_translation_disabled(env, mmu_idx, is_secure)) {
> -return get_phys_addr_disabled(env, address, access_type, mmu_idx,
> -  is_secure, result, fi);
> +goto do_disabled;
>  }

I'd prefer to avoid this goto back up into the middle of an unrelated
switch statement.

thanks
-- PMM

[PATCH v3 30/42] target/arm: Add ptw_idx argument to S1_ptw_translate

2022-10-01 Thread Richard Henderson 
Hoist the computation of the mmu_idx for the ptw up to
get_phys_addr_with_secure_debug and get_phys_addr_twostage.
This removes the duplicate check for stage2 disabled
from the middle of the walk, performing it only once.

Pass ptw_idx through get_phys_addr_{v5,v6,lpae} and arm_{ldl,ldq}_ptw.

Signed-off-by: Richard Henderson 
---
 target/arm/ptw.c | 104 ---
 1 file changed, 71 insertions(+), 33 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 445382ab03..7a77bea2c7 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -17,7 +17,8 @@
 
 static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
-   bool is_secure, bool s1_is_el0, bool debug,
+   ARMMMUIdx ptw_idx, bool is_secure,
+   bool s1_is_el0, bool debug,
GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
 __attribute__((nonnull));
 
@@ -220,21 +221,16 @@ static bool S2_attrs_are_device(uint64_t hcr, uint8_t 
attrs)
 }
 
 /* Translate a S1 pagetable walk through S2 if needed.  */
-static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx, hwaddr addr,
+static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
+ ARMMMUIdx s2_mmu_idx, hwaddr addr,
  bool *is_secure_ptr, void **hphys, hwaddr *gphys,
  bool debug, ARMMMUFaultInfo *fi)
 {
 bool is_secure = *is_secure_ptr;
-ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
-bool s2_phys = false;
 uint8_t pte_attrs;
-bool pte_secure;
+bool s2_phys, 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;
-}
+s2_phys = s2_mmu_idx == ARMMMUIdx_Phys_S || s2_mmu_idx == 
ARMMMUIdx_Phys_NS;
 
 if (unlikely(debug)) {
 /*
@@ -247,8 +243,12 @@ static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx 
mmu_idx, hwaddr addr,
 pte_secure = is_secure;
 } else {
 GetPhysAddrResult s2 = { };
+ARMMMUIdx phys_idx = (is_secure ? ARMMMUIdx_Phys_S
+  : ARMMMUIdx_Phys_NS);
+
 if (!get_phys_addr_lpae(env, addr, MMU_DATA_LOAD, s2_mmu_idx,
-is_secure, false, debug, &s2, fi)) {
+phys_idx, is_secure, false, debug,
+&s2, fi)) {
 goto fail;
 }
 *gphys = s2.f.phys_addr;
@@ -310,7 +310,8 @@ static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx 
mmu_idx, hwaddr addr,
 
 /* All loads done in the course of a page table walk go through here. */
 static uint32_t arm_ldl_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
-ARMMMUIdx mmu_idx, bool debug, ARMMMUFaultInfo *fi)
+ARMMMUIdx mmu_idx, ARMMMUIdx ptw_idx,
+bool debug, ARMMMUFaultInfo *fi)
 {
 CPUState *cs = env_cpu(env);
 void *hphys;
@@ -318,7 +319,7 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, hwaddr addr, 
bool is_secure,
 uint32_t data;
 bool be;
 
-if (!S1_ptw_translate(env, mmu_idx, addr, &is_secure,
+if (!S1_ptw_translate(env, mmu_idx, ptw_idx, addr, &is_secure,
   &hphys, &gphys, debug, fi)) {
 /* Failure. */
 assert(fi->s1ptw);
@@ -354,7 +355,8 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, hwaddr addr, 
bool is_secure,
 }
 
 static uint64_t arm_ldq_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
-ARMMMUIdx mmu_idx, bool debug, ARMMMUFaultInfo *fi)
+ARMMMUIdx mmu_idx, ARMMMUIdx ptw_idx,
+bool debug, ARMMMUFaultInfo *fi)
 {
 CPUState *cs = env_cpu(env);
 void *hphys;
@@ -362,7 +364,7 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, hwaddr addr, 
bool is_secure,
 uint64_t data;
 bool be;
 
-if (!S1_ptw_translate(env, mmu_idx, addr, &is_secure,
+if (!S1_ptw_translate(env, mmu_idx, ptw_idx, addr, &is_secure,
   &hphys, &gphys, debug, fi)) {
 /* Failure. */
 assert(fi->s1ptw);
@@ -507,7 +509,7 @@ static int simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx 
mmu_idx, int ap)
 
 static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
  MMUAccessType access_type, ARMMMUIdx mmu_idx,
- bool is_secure, bool debug,
+ ARMMMUIdx ptw_idx, bool is_secure, bool debug,
  GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
 {
 int level = 1;
@@ -527,7