[PATCH v3 07/17] target/loongarch: Add parameter mmu_context with loongarch_page_table_walker

Bibo Mao posted 17 patches 3 months, 3 weeks ago
Maintainers: Song Gao <gaosong@loongson.cn>, Bibo Mao <maobibo@loongson.cn>, Jiaxun Yang <jiaxun.yang@flygoat.com>
There is a newer version of this series
[PATCH v3 07/17] target/loongarch: Add parameter mmu_context with loongarch_page_table_walker
Posted by Bibo Mao 3 months, 3 weeks ago
With function loongarch_page_table_walker(), some output parameters
such as physical address and prot can be moved to structure mmu_context.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 target/loongarch/cpu_helper.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/target/loongarch/cpu_helper.c b/target/loongarch/cpu_helper.c
index 9e6de2908f..a26bb8b11b 100644
--- a/target/loongarch/cpu_helper.c
+++ b/target/loongarch/cpu_helper.c
@@ -106,17 +106,18 @@ int loongarch_check_pte(CPULoongArchState *env, mmu_context *context,
     return TLBRET_MATCH;
 }
 
-static int loongarch_page_table_walker(CPULoongArchState *env, hwaddr *physical,
-                                       int *prot, target_ulong address,
+static int loongarch_page_table_walker(CPULoongArchState *env,
+                                       mmu_context *context,
                                        int access_type, int mmu_idx)
 {
     CPUState *cs = env_cpu(env);
     target_ulong index, phys;
     uint64_t dir_base, dir_width;
     uint64_t base;
-    int level, ret;
-    mmu_context context;
+    int level;
+    target_ulong address;
 
+    address = context->vaddr;
     if ((address >> 63) & 0x1) {
         base = env->CSR_PGDH;
     } else {
@@ -158,16 +159,9 @@ static int loongarch_page_table_walker(CPULoongArchState *env, hwaddr *physical,
         base = ldq_phys(cs->as, phys);
     }
 
-    context.vaddr = address;
-    context.ps = dir_base;
-    context.pte = base;
-    ret = loongarch_check_pte(env, &context, access_type, mmu_idx);
-    if (ret == TLBRET_MATCH) {
-        *physical = context.physical;
-        *prot = context.prot;
-    }
-
-    return ret;
+    context->ps = dir_base;
+    context->pte = base;
+    return loongarch_check_pte(env, context, access_type, mmu_idx);
 }
 
 static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
@@ -176,7 +170,9 @@ static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
                                  int is_debug)
 {
     int ret;
+    mmu_context context;
 
+    context.vaddr = address;
     if (tcg_enabled()) {
         ret = loongarch_get_addr_from_tlb(env, physical, prot, address,
                                           access_type, mmu_idx);
@@ -191,8 +187,7 @@ static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
          * legal mapping, even if the mapping is not yet in TLB. return 0 if
          * there is a valid map, else none zero.
          */
-        return loongarch_page_table_walker(env, physical, prot, address,
-                                           access_type, mmu_idx);
+        return loongarch_page_table_walker(env, &context, access_type, mmu_idx);
     }
 
     return TLBRET_NOMATCH;
-- 
2.39.3
Re: [PATCH v3 07/17] target/loongarch: Add parameter mmu_context with loongarch_page_table_walker
Posted by Richard Henderson 3 months, 3 weeks ago
On 7/24/25 15:37, Bibo Mao wrote:
> @@ -191,8 +187,7 @@ static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
>            * legal mapping, even if the mapping is not yet in TLB. return 0 if
>            * there is a valid map, else none zero.
>            */
> -        return loongarch_page_table_walker(env, physical, prot, address,
> -                                           access_type, mmu_idx);
> +        return loongarch_page_table_walker(env, &context, access_type, mmu_idx);
>       }

You haven't stored to the physical/prot arguments to loongarch_map_address.  I'm sure this 
gets fixed somewhere in patches 7 through 11, but it means that this patch set isn't 
bisectable.

It *might* be easier to start from the other end of the call stack.
Then you can do things like

   return loongarch_page_table_walker(env, &context->physical, &context->prot, etc)

in the intermediate steps.


r~
Re: [PATCH v3 07/17] target/loongarch: Add parameter mmu_context with loongarch_page_table_walker
Posted by Bibo Mao 3 months, 2 weeks ago

On 2025/7/26 上午9:31, Richard Henderson wrote:
> On 7/24/25 15:37, Bibo Mao wrote:
>> @@ -191,8 +187,7 @@ static int loongarch_map_address(CPULoongArchState 
>> *env, hwaddr *physical,
>>            * legal mapping, even if the mapping is not yet in TLB. 
>> return 0 if
>>            * there is a valid map, else none zero.
>>            */
>> -        return loongarch_page_table_walker(env, physical, prot, address,
>> -                                           access_type, mmu_idx);
>> +        return loongarch_page_table_walker(env, &context, 
>> access_type, mmu_idx);
>>       }
> 
> You haven't stored to the physical/prot arguments to 
> loongarch_map_address.  I'm sure this gets fixed somewhere in patches 7 
> through 11, but it means that this patch set isn't bisectable.
> 
> It *might* be easier to start from the other end of the call stack.
> Then you can do things like
> 
>    return loongarch_page_table_walker(env, &context->physical, 
> &context->prot, etc)
> 
> in the intermediate steps.
Good catch. This patch has problem and will fix it in next round.

Regards
Bibo Mao
> 
> 
> r~