When update_pagemask was split from helper_mtc0_pagemask,
we failed to actually write to the new parameter but continue
to write to env->CP0_PageMask.  Thus the use within
page_table_walk_refill modifies cpu state and not the local
variable as expected.

Simplify by renaming to compute_pagemask and returning the
value directly.  No need for either env or pointer return.

Fixes: 074cfcb4dae ("target/mips: Implement hardware page table walker for MIPS32")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/tcg/tcg-internal.h      |  2 +-
 target/mips/tcg/system/cp0_helper.c | 10 +++++-----
 target/mips/tcg/system/tlb_helper.c |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/mips/tcg/tcg-internal.h b/target/mips/tcg/tcg-internal.h
index 74fc1309a7..950e6afc3f 100644
--- a/target/mips/tcg/tcg-internal.h
+++ b/target/mips/tcg/tcg-internal.h
@@ -47,7 +47,7 @@ bool mips_cpu_exec_interrupt(CPUState *cpu, int int_req);
 
 void mmu_init(CPUMIPSState *env, const mips_def_t *def);
 
-void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask);
+uint32_t compute_pagemask(uint32_t val);
 
 void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra);
 uint32_t cpu_mips_get_random(CPUMIPSState *env);
diff --git a/target/mips/tcg/system/cp0_helper.c b/target/mips/tcg/system/cp0_helper.c
index 5db8166d45..78e422b0ca 100644
--- a/target/mips/tcg/system/cp0_helper.c
+++ b/target/mips/tcg/system/cp0_helper.c
@@ -864,24 +864,24 @@ void helper_mtc0_memorymapid(CPUMIPSState *env, target_ulong arg1)
     }
 }
 
-void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
+uint32_t compute_pagemask(uint32_t val)
 {
     /* Don't care MASKX as we don't support 1KB page */
-    uint32_t mask = extract32((uint32_t)arg1, CP0PM_MASK, 16);
+    uint32_t mask = extract32(val, CP0PM_MASK, 16);
     int maskbits = cto32(mask);
 
     /* Ensure no more set bit after first zero, and maskbits even. */
     if ((mask >> maskbits) == 0 && maskbits % 2 == 0) {
-        env->CP0_PageMask = mask << CP0PM_MASK;
+        return mask << CP0PM_MASK;
     } else {
         /* When invalid, set to default target page size. */
-        env->CP0_PageMask = 0;
+        return 0;
     }
 }
 
 void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
 {
-    update_pagemask(env, arg1, &env->CP0_PageMask);
+    env->CP0_PageMask = compute_pagemask(arg1);
 }
 
 void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
diff --git a/target/mips/tcg/system/tlb_helper.c b/target/mips/tcg/system/tlb_helper.c
index 123639fa18..df80301a41 100644
--- a/target/mips/tcg/system/tlb_helper.c
+++ b/target/mips/tcg/system/tlb_helper.c
@@ -876,7 +876,7 @@ refill:
         }
     }
     pw_pagemask = m >> TARGET_PAGE_BITS;
-    update_pagemask(env, pw_pagemask << CP0PM_MASK, &pw_pagemask);
+    pw_pagemask = compute_pagemask(pw_pagemask << CP0PM_MASK);
     pw_entryhi = (address & ~0x1fff) | (env->CP0_EntryHi & 0xFF);
     {
         target_ulong tmp_entryhi = env->CP0_EntryHi;
-- 
2.43.0

On 28/3/25 18:55, Richard Henderson wrote:
> When update_pagemask was split from helper_mtc0_pagemask,
> we failed to actually write to the new parameter but continue
> to write to env->CP0_PageMask.  Thus the use within
> page_table_walk_refill modifies cpu state and not the local
> variable as expected.
> 
> Simplify by renaming to compute_pagemask and returning the
> value directly.  No need for either env or pointer return.
> 
> Fixes: 074cfcb4dae ("target/mips: Implement hardware page table walker for MIPS32")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/mips/tcg/tcg-internal.h      |  2 +-
>   target/mips/tcg/system/cp0_helper.c | 10 +++++-----
>   target/mips/tcg/system/tlb_helper.c |  2 +-
>   3 files changed, 7 insertions(+), 7 deletions(-)


>   void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
> diff --git a/target/mips/tcg/system/tlb_helper.c b/target/mips/tcg/system/tlb_helper.c
> index 123639fa18..df80301a41 100644
> --- a/target/mips/tcg/system/tlb_helper.c
> +++ b/target/mips/tcg/system/tlb_helper.c
> @@ -876,7 +876,7 @@ refill:
>           }
>       }
>       pw_pagemask = m >> TARGET_PAGE_BITS;
> -    update_pagemask(env, pw_pagemask << CP0PM_MASK, &pw_pagemask);
> +    pw_pagemask = compute_pagemask(pw_pagemask << CP0PM_MASK);

Nice catch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>