Rather than adjust env->hflags so that the value computed
by cpu_mmu_index() changes, compute the mmu_idx that we
want directly and pass it down.

Introduce symbolic constants for MMU_{KERNEL,ERL}_IDX.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/cpu.h                   |  4 +++-
 target/mips/tcg/sysemu/tlb_helper.c | 32 ++++++++++++-----------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 1163a71f3c..3ba8dccd2d 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1242,12 +1242,14 @@ uint32_t cpu_rddsp(uint32_t mask_num, CPUMIPSState *env);
  * MMU modes definitions. We carefully match the indices with our
  * hflags layout.
  */
+#define MMU_KERNEL_IDX 0
 #define MMU_USER_IDX 2
+#define MMU_ERL_IDX 3
 
 static inline int hflags_mmu_index(uint32_t hflags)
 {
     if (hflags & MIPS_HFLAG_ERL) {
-        return 3; /* ERL */
+        return MMU_ERL_IDX;
     } else {
         return hflags & MIPS_HFLAG_KSU;
     }
diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
index 4ede904800..b715449114 100644
--- a/target/mips/tcg/sysemu/tlb_helper.c
+++ b/target/mips/tcg/sysemu/tlb_helper.c
@@ -623,7 +623,7 @@ static uint64_t get_tlb_entry_layout(CPUMIPSState *env, uint64_t entry,
 static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
         int directory_index, bool *huge_page, bool *hgpg_directory_hit,
         uint64_t *pw_entrylo0, uint64_t *pw_entrylo1,
-        unsigned directory_shift, unsigned leaf_shift)
+        unsigned directory_shift, unsigned leaf_shift, int ptw_mmu_idx)
 {
     int dph = (env->CP0_PWCtl >> CP0PC_DPH) & 0x1;
     int psn = (env->CP0_PWCtl >> CP0PC_PSN) & 0x3F;
@@ -638,8 +638,7 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
     uint64_t w = 0;
 
     if (get_physical_address(env, &paddr, &prot, *vaddr, MMU_DATA_LOAD,
-                             cpu_mmu_index(env, false)) !=
-                             TLBRET_MATCH) {
+                             ptw_mmu_idx) != TLBRET_MATCH) {
         /* wrong base address */
         return 0;
     }
@@ -666,8 +665,7 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
                 *pw_entrylo0 = entry;
             }
             if (get_physical_address(env, &paddr, &prot, vaddr2, MMU_DATA_LOAD,
-                                     cpu_mmu_index(env, false)) !=
-                                     TLBRET_MATCH) {
+                                     ptw_mmu_idx) != TLBRET_MATCH) {
                 return 0;
             }
             if (!get_pte(env, vaddr2, leafentry_size, &entry)) {
@@ -690,7 +688,7 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
 }
 
 static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
-                                   int mmu_idx)
+                                   int ptw_mmu_idx)
 {
     int gdw = (env->CP0_PWSize >> CP0PS_GDW) & 0x3F;
     int udw = (env->CP0_PWSize >> CP0PS_UDW) & 0x3F;
@@ -776,7 +774,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
         vaddr |= goffset;
         switch (walk_directory(env, &vaddr, pf_gdw, &huge_page, &hgpg_gdhit,
                                &pw_entrylo0, &pw_entrylo1,
-                               directory_shift, leaf_shift))
+                               directory_shift, leaf_shift, ptw_mmu_idx))
         {
         case 0:
             return false;
@@ -793,7 +791,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
         vaddr |= uoffset;
         switch (walk_directory(env, &vaddr, pf_udw, &huge_page, &hgpg_udhit,
                                &pw_entrylo0, &pw_entrylo1,
-                               directory_shift, leaf_shift))
+                               directory_shift, leaf_shift, ptw_mmu_idx))
         {
         case 0:
             return false;
@@ -810,7 +808,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
         vaddr |= moffset;
         switch (walk_directory(env, &vaddr, pf_mdw, &huge_page, &hgpg_mdhit,
                                &pw_entrylo0, &pw_entrylo1,
-                               directory_shift, leaf_shift))
+                               directory_shift, leaf_shift, ptw_mmu_idx))
         {
         case 0:
             return false;
@@ -825,8 +823,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
     /* Leaf Level Page Table - First half of PTE pair */
     vaddr |= ptoffset0;
     if (get_physical_address(env, &paddr, &prot, vaddr, MMU_DATA_LOAD,
-                             cpu_mmu_index(env, false)) !=
-                             TLBRET_MATCH) {
+                             ptw_mmu_idx) != TLBRET_MATCH) {
         return false;
     }
     if (!get_pte(env, vaddr, leafentry_size, &dir_entry)) {
@@ -838,8 +835,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
     /* Leaf Level Page Table - Second half of PTE pair */
     vaddr |= ptoffset1;
     if (get_physical_address(env, &paddr, &prot, vaddr, MMU_DATA_LOAD,
-                             cpu_mmu_index(env, false)) !=
-                             TLBRET_MATCH) {
+                             ptw_mmu_idx) != TLBRET_MATCH) {
         return false;
     }
     if (!get_pte(env, vaddr, leafentry_size, &dir_entry)) {
@@ -944,12 +940,10 @@ bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
          * Memory reads during hardware page table walking are performed
          * as if they were kernel-mode load instructions.
          */
-        int mode = (env->hflags & MIPS_HFLAG_KSU);
-        bool ret_walker;
-        env->hflags &= ~MIPS_HFLAG_KSU;
-        ret_walker = page_table_walk_refill(env, address, mmu_idx);
-        env->hflags |= mode;
-        if (ret_walker) {
+        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
+                           MMU_ERL_IDX : MMU_KERNEL_IDX);
+
+        if (page_table_walk_refill(env, address, ptw_mmu_idx)) {
             ret = get_physical_address(env, &physical, &prot, address,
                                        access_type, mmu_idx);
             if (ret == TLBRET_MATCH) {
-- 
2.34.1

Hi Richard,

On 2/2/24 06:49, Richard Henderson wrote:
> Rather than adjust env->hflags so that the value computed
> by cpu_mmu_index() changes, compute the mmu_idx that we
> want directly and pass it down.
> 
> Introduce symbolic constants for MMU_{KERNEL,ERL}_IDX.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/mips/cpu.h                   |  4 +++-
>   target/mips/tcg/sysemu/tlb_helper.c | 32 ++++++++++++-----------------
>   2 files changed, 16 insertions(+), 20 deletions(-)


> @@ -944,12 +940,10 @@ bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>            * Memory reads during hardware page table walking are performed
>            * as if they were kernel-mode load instructions.
>            */
> -        int mode = (env->hflags & MIPS_HFLAG_KSU);
> -        bool ret_walker;
> -        env->hflags &= ~MIPS_HFLAG_KSU;
> -        ret_walker = page_table_walk_refill(env, address, mmu_idx);
> -        env->hflags |= mode;
> -        if (ret_walker) {
> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
> +                           MMU_ERL_IDX : MMU_KERNEL_IDX);

Checking https://gitlab.com/qemu-project/qemu/-/issues/2470.

Parenthesis are mis-placed.

           int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL) ?
                              MMU_ERL_IDX : MMU_KERNEL_IDX;

Revisiting, we loose possible MMU_USER_IDX value but
- we don't use it
- this is sysemu code so we only expect MMU_KERNEL_IDX

Is that right?

> +
> +        if (page_table_walk_refill(env, address, ptw_mmu_idx)) {
>               ret = get_physical_address(env, &physical, &prot, address,
>                                          access_type, mmu_idx);
>               if (ret == TLBRET_MATCH) {

On 8/10/24 04:11, Philippe Mathieu-Daudé wrote:
> Hi Richard,
> 
> On 2/2/24 06:49, Richard Henderson wrote:
>> Rather than adjust env->hflags so that the value computed
>> by cpu_mmu_index() changes, compute the mmu_idx that we
>> want directly and pass it down.
>>
>> Introduce symbolic constants for MMU_{KERNEL,ERL}_IDX.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/mips/cpu.h                   |  4 +++-
>>   target/mips/tcg/sysemu/tlb_helper.c | 32 ++++++++++++-----------------
>>   2 files changed, 16 insertions(+), 20 deletions(-)
> 
> 
>> @@ -944,12 +940,10 @@ bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>            * Memory reads during hardware page table walking are performed
>>            * as if they were kernel-mode load instructions.
>>            */
>> -        int mode = (env->hflags & MIPS_HFLAG_KSU);
>> -        bool ret_walker;
>> -        env->hflags &= ~MIPS_HFLAG_KSU;
>> -        ret_walker = page_table_walk_refill(env, address, mmu_idx);
>> -        env->hflags |= mode;
>> -        if (ret_walker) {
>> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
>> +                           MMU_ERL_IDX : MMU_KERNEL_IDX);
> 
> Checking https://gitlab.com/qemu-project/qemu/-/issues/2470.
> 
> Parenthesis are mis-placed.
> 
>            int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL) ?
>                               MMU_ERL_IDX : MMU_KERNEL_IDX;

This makes no difference to the evaluation of this expression.

> 
> Revisiting, we loose possible MMU_USER_IDX value but
> - we don't use it
> - this is sysemu code so we only expect MMU_KERNEL_IDX
> 
> Is that right?

The comment above is correct that ptw reads are performed in kernel mode.

The code previously saved the current mode, cleared the user bit, performed the operation, 
and then restored the previous mode.  There was no possible MMU_USER_IDX during that interval.

The code currently skips the save/restore and simply selects MMU_KERNEL_IDX.


r~