[PATCH-for-9.1 v2 0/2] target/mips: Use correct MMU index in get_pte()

[PATCH-for-9.1 v2 0/2] target/mips: Use correct MMU index in get_pte()

[Philippe Mathieu-Daudé]

Propage ptw_mmu_idx to get_pte() and use it via
the cpu_ld/st_code_mmu() API.

Philippe Mathieu-Daudé (2):
  target/mips: Pass page table entry size in bytes to get_pte()
  target/mips: Use correct MMU index in get_pte()

 target/mips/tcg/sysemu/tlb_helper.c | 35 ++++++++++++++++-------------
 1 file changed, 20 insertions(+), 15 deletions(-)

-- 
2.45.2

[PATCH-for-9.1 v2 1/2] target/mips: Pass page table entry size in bytes to get_pte()

[Philippe Mathieu-Daudé]

In order to simplify a bit, pass the PTE size in
bytes rather than bits.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/tcg/sysemu/tlb_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
index 3ba6d369a6..a8caf3ade8 100644
--- a/target/mips/tcg/sysemu/tlb_helper.c
+++ b/target/mips/tcg/sysemu/tlb_helper.c
@@ -592,13 +592,13 @@ static void raise_mmu_exception(CPUMIPSState *env, target_ulong address,
  * resulting in a TLB or XTLB Refill exception.
  */
 
-static bool get_pte(CPUMIPSState *env, uint64_t vaddr, int entry_size,
+static bool get_pte(CPUMIPSState *env, uint64_t vaddr, unsigned entry_bytes,
         uint64_t *pte)
 {
-    if ((vaddr & ((entry_size >> 3) - 1)) != 0) {
+    if ((vaddr & (entry_bytes - 1)) != 0) {
         return false;
     }
-    if (entry_size == 64) {
+    if (entry_bytes == 8) {
         *pte = cpu_ldq_code(env, vaddr);
     } else {
         *pte = cpu_ldl_code(env, vaddr);
@@ -607,11 +607,11 @@ static bool get_pte(CPUMIPSState *env, uint64_t vaddr, int entry_size,
 }
 
 static uint64_t get_tlb_entry_layout(CPUMIPSState *env, uint64_t entry,
-        int entry_size, int ptei)
+                                     unsigned entry_bytes, int ptei)
 {
     uint64_t result = entry;
     uint64_t rixi;
-    if (ptei > entry_size) {
+    if (ptei > entry_bytes >> 3) {
         ptei -= 32;
     }
     result >>= (ptei - 2);
@@ -630,8 +630,8 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
     int psn = (env->CP0_PWCtl >> CP0PC_PSN) & 0x3F;
     int hugepg = (env->CP0_PWCtl >> CP0PC_HUGEPG) & 0x1;
     int pf_ptew = (env->CP0_PWField >> CP0PF_PTEW) & 0x3F;
-    uint32_t direntry_size = 1 << (directory_shift + 3);
-    uint32_t leafentry_size = 1 << (leaf_shift + 3);
+    uint32_t direntry_size = 1 << directory_shift;
+    uint32_t leafentry_size = 1 << leaf_shift;
     uint64_t entry;
     uint64_t paddr;
     int prot;
@@ -768,7 +768,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
     ptoffset0 = (ptindex >> 1) << (leaf_shift + 1);
     ptoffset1 = ptoffset0 | (1 << (leaf_shift));
 
-    leafentry_size = 1 << (leaf_shift + 3);
+    leafentry_size = 1 << leaf_shift;
 
     /* Global Directory */
     if (gdw > 0) {
-- 
2.45.2

[PATCH-for-9.1 v2 2/2] target/mips: Use correct MMU index in get_pte()

[Philippe Mathieu-Daudé]

When refactoring page_table_walk_refill() in commit 4e999bf419
we missed the indirect call to cpu_mmu_index() in get_pte():

  page_table_walk_refill()
  -> get_pte()
     -> cpu_ld[lq]_code()
        -> cpu_mmu_index()

Since we don't mask anymore the modes in hflags, cpu_mmu_index()
can return UM or SM, while we only expect KM or ERL.

Fix by propagating ptw_mmu_idx to get_pte(), and use the
cpu_ld/st_code_mmu() API with the correct MemOpIdx.

Reported-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Reported-by: Waldemar Brodkorb <wbx@uclibc-ng.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2470
Fixes: 4e999bf419 ("target/mips: Pass ptw_mmu_idx down from mips_cpu_tlb_fill")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/tcg/sysemu/tlb_helper.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
index a8caf3ade8..4e64c226a5 100644
--- a/target/mips/tcg/sysemu/tlb_helper.c
+++ b/target/mips/tcg/sysemu/tlb_helper.c
@@ -593,16 +593,21 @@ static void raise_mmu_exception(CPUMIPSState *env, target_ulong address,
  */
 
 static bool get_pte(CPUMIPSState *env, uint64_t vaddr, unsigned entry_bytes,
-        uint64_t *pte)
+                    uint64_t *pte, unsigned ptw_mmu_idx)
 {
+    MemOpIdx oi;
+
     if ((vaddr & (entry_bytes - 1)) != 0) {
         return false;
     }
+
+    oi = make_memop_idx(size_memop(entry_bytes) | MO_TE, ptw_mmu_idx);
     if (entry_bytes == 8) {
-        *pte = cpu_ldq_code(env, vaddr);
+        *pte = cpu_ldq_code_mmu(env, vaddr, oi, 0);
     } else {
-        *pte = cpu_ldl_code(env, vaddr);
+        *pte = cpu_ldl_code_mmu(env, vaddr, oi, 0);
     }
+
     return true;
 }
 
@@ -643,7 +648,7 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
         /* wrong base address */
         return 0;
     }
-    if (!get_pte(env, *vaddr, direntry_size, &entry)) {
+    if (!get_pte(env, *vaddr, direntry_size, &entry, ptw_mmu_idx)) {
         return 0;
     }
 
@@ -669,7 +674,7 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
                                      ptw_mmu_idx) != TLBRET_MATCH) {
                 return 0;
             }
-            if (!get_pte(env, vaddr2, leafentry_size, &entry)) {
+            if (!get_pte(env, vaddr2, leafentry_size, &entry, ptw_mmu_idx)) {
                 return 0;
             }
             entry = get_tlb_entry_layout(env, entry, leafentry_size, pf_ptew);
@@ -827,7 +832,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
                              ptw_mmu_idx) != TLBRET_MATCH) {
         return false;
     }
-    if (!get_pte(env, vaddr, leafentry_size, &dir_entry)) {
+    if (!get_pte(env, vaddr, leafentry_size, &dir_entry, ptw_mmu_idx)) {
         return false;
     }
     dir_entry = get_tlb_entry_layout(env, dir_entry, leafentry_size, pf_ptew);
@@ -839,7 +844,7 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
                              ptw_mmu_idx) != TLBRET_MATCH) {
         return false;
     }
-    if (!get_pte(env, vaddr, leafentry_size, &dir_entry)) {
+    if (!get_pte(env, vaddr, leafentry_size, &dir_entry, ptw_mmu_idx)) {
         return false;
     }
     dir_entry = get_tlb_entry_layout(env, dir_entry, leafentry_size, pf_ptew);
-- 
2.45.2

Re: [PATCH-for-9.1 v2 1/2] target/mips: Pass page table entry size in bytes to get_pte()

[Richard Henderson]

On 8/13/24 20:18, Philippe Mathieu-Daudé wrote:
> -    if (ptei > entry_size) {
> +    if (ptei > entry_bytes >> 3) {

Shifting the wrong way here.

r~

Re: [PATCH-for-9.1 v2 1/2] target/mips: Pass page table entry size in bytes to get_pte()

[Richard Henderson]

On 8/13/24 20:18, Philippe Mathieu-Daudé wrote:
> In order to simplify a bit, pass the PTE size in
> bytes rather than bits.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/mips/tcg/sysemu/tlb_helper.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
> index 3ba6d369a6..a8caf3ade8 100644
> --- a/target/mips/tcg/sysemu/tlb_helper.c
> +++ b/target/mips/tcg/sysemu/tlb_helper.c
> @@ -592,13 +592,13 @@ static void raise_mmu_exception(CPUMIPSState *env, target_ulong address,
>    * resulting in a TLB or XTLB Refill exception.
>    */
>   
> -static bool get_pte(CPUMIPSState *env, uint64_t vaddr, int entry_size,
> +static bool get_pte(CPUMIPSState *env, uint64_t vaddr, unsigned entry_bytes,
>           uint64_t *pte)
>   {
> -    if ((vaddr & ((entry_size >> 3) - 1)) != 0) {
> +    if ((vaddr & (entry_bytes - 1)) != 0) {
>           return false;
>       }
> -    if (entry_size == 64) {
> +    if (entry_bytes == 8) {
>           *pte = cpu_ldq_code(env, vaddr);
>       } else {
>           *pte = cpu_ldl_code(env, vaddr);

Considering the next patch, where you need to make the MemOpIdx,
why not pass in the size as MemOp?

> @@ -630,8 +630,8 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
>       int psn = (env->CP0_PWCtl >> CP0PC_PSN) & 0x3F;
>       int hugepg = (env->CP0_PWCtl >> CP0PC_HUGEPG) & 0x1;
>       int pf_ptew = (env->CP0_PWField >> CP0PF_PTEW) & 0x3F;
> -    uint32_t direntry_size = 1 << (directory_shift + 3);
> -    uint32_t leafentry_size = 1 << (leaf_shift + 3);
> +    uint32_t direntry_size = 1 << directory_shift;
> +    uint32_t leafentry_size = 1 << leaf_shift;

This would then be just directory_shift/leaf_shift unchanged.


r~

Re: [PATCH-for-9.1 v2 2/2] target/mips: Use correct MMU index in get_pte()

[Richard Henderson]

On 8/13/24 20:18, Philippe Mathieu-Daudé wrote:
> When refactoring page_table_walk_refill() in commit 4e999bf419
> we missed the indirect call to cpu_mmu_index() in get_pte():
> 
>    page_table_walk_refill()
>    -> get_pte()
>       -> cpu_ld[lq]_code()
>          -> cpu_mmu_index()
> 
> Since we don't mask anymore the modes in hflags, cpu_mmu_index()
> can return UM or SM, while we only expect KM or ERL.
> 
> Fix by propagating ptw_mmu_idx to get_pte(), and use the
> cpu_ld/st_code_mmu() API with the correct MemOpIdx.
> 
> Reported-by: Thomas Petazzoni<thomas.petazzoni@bootlin.com>
> Reported-by: Waldemar Brodkorb<wbx@uclibc-ng.org>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2470
> Fixes: 4e999bf419 ("target/mips: Pass ptw_mmu_idx down from mips_cpu_tlb_fill")
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/mips/tcg/sysemu/tlb_helper.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.1 v2 1/2] target/mips: Pass page table entry size in bytes to get_pte()

[Philippe Mathieu-Daudé]

On 13/8/24 13:02, Richard Henderson wrote:
> On 8/13/24 20:18, Philippe Mathieu-Daudé wrote:
>> In order to simplify a bit, pass the PTE size in
>> bytes rather than bits.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/mips/tcg/sysemu/tlb_helper.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/mips/tcg/sysemu/tlb_helper.c 
>> b/target/mips/tcg/sysemu/tlb_helper.c
>> index 3ba6d369a6..a8caf3ade8 100644
>> --- a/target/mips/tcg/sysemu/tlb_helper.c
>> +++ b/target/mips/tcg/sysemu/tlb_helper.c
>> @@ -592,13 +592,13 @@ static void raise_mmu_exception(CPUMIPSState 
>> *env, target_ulong address,
>>    * resulting in a TLB or XTLB Refill exception.
>>    */
>> -static bool get_pte(CPUMIPSState *env, uint64_t vaddr, int entry_size,
>> +static bool get_pte(CPUMIPSState *env, uint64_t vaddr, unsigned 
>> entry_bytes,
>>           uint64_t *pte)
>>   {
>> -    if ((vaddr & ((entry_size >> 3) - 1)) != 0) {
>> +    if ((vaddr & (entry_bytes - 1)) != 0) {
>>           return false;
>>       }
>> -    if (entry_size == 64) {
>> +    if (entry_bytes == 8) {
>>           *pte = cpu_ldq_code(env, vaddr);
>>       } else {
>>           *pte = cpu_ldl_code(env, vaddr);
> 
> Considering the next patch, where you need to make the MemOpIdx,
> why not pass in the size as MemOp?

Clever.