[PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[Philippe Mathieu-Daudé]

When refactoring page_table_walk_refill() in commit 4e999bf419
we replaced the execution mode and forced it to kernel mode.
Restore the previous behavior to also get supervisor / user modes.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
index 3ba6d369a6..e7ae4f0bef 100644
--- a/target/mips/tcg/sysemu/tlb_helper.c
+++ b/target/mips/tcg/sysemu/tlb_helper.c
@@ -940,8 +940,9 @@ 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 ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
-                           MMU_ERL_IDX : MMU_KERNEL_IDX);
+        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL)
+                          ? MMU_ERL_IDX
+                          : (env->hflags & MIPS_HFLAG_KSU);
 
         if (page_table_walk_refill(env, address, ptw_mmu_idx)) {
             ret = get_physical_address(env, &physical, &prot, address,
-- 
2.45.2

Re: [PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[Richard Henderson]

On 8/12/24 02:54, Philippe Mathieu-Daudé wrote:
> When refactoring page_table_walk_refill() in commit 4e999bf419
> we replaced the execution mode and forced it to kernel mode.
> Restore the previous behavior to also get supervisor / user modes.
> 
> 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 | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
> index 3ba6d369a6..e7ae4f0bef 100644
> --- a/target/mips/tcg/sysemu/tlb_helper.c
> +++ b/target/mips/tcg/sysemu/tlb_helper.c
> @@ -940,8 +940,9 @@ 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 ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
> -                           MMU_ERL_IDX : MMU_KERNEL_IDX);
> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL)
> +                          ? MMU_ERL_IDX
> +                          : (env->hflags & MIPS_HFLAG_KSU);

This contradicts the comment above.
If this code change is correct, then the comment isn't.

But the comment certainly makes sense -- page tables are never accessible to user mode.


r~

Re: [PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[Philippe Mathieu-Daudé]

On 12/8/24 02:48, Richard Henderson wrote:
> On 8/12/24 02:54, Philippe Mathieu-Daudé wrote:
>> When refactoring page_table_walk_refill() in commit 4e999bf419
>> we replaced the execution mode and forced it to kernel mode.
>> Restore the previous behavior to also get supervisor / user modes.
>>
>> 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 | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/mips/tcg/sysemu/tlb_helper.c 
>> b/target/mips/tcg/sysemu/tlb_helper.c
>> index 3ba6d369a6..e7ae4f0bef 100644
>> --- a/target/mips/tcg/sysemu/tlb_helper.c
>> +++ b/target/mips/tcg/sysemu/tlb_helper.c
>> @@ -940,8 +940,9 @@ 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 ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
>> -                           MMU_ERL_IDX : MMU_KERNEL_IDX);
>> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL)
>> +                          ? MMU_ERL_IDX
>> +                          : (env->hflags & MIPS_HFLAG_KSU);
> 
> This contradicts the comment above.
> If this code change is correct, then the comment isn't.

OK.

> But the comment certainly makes sense -- page tables are never 
> accessible to user mode.

But we are still dropping the supervisor mode, so OK if I
reword as:

"Restore the previous behavior to also get supervisor modes."

?

Re: [PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[Richard Henderson]

On 8/12/24 15:35, Philippe Mathieu-Daudé wrote:
> On 12/8/24 02:48, Richard Henderson wrote:
>> On 8/12/24 02:54, Philippe Mathieu-Daudé wrote:
>>> When refactoring page_table_walk_refill() in commit 4e999bf419
>>> we replaced the execution mode and forced it to kernel mode.
>>> Restore the previous behavior to also get supervisor / user modes.
>>>
>>> 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 | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
>>> index 3ba6d369a6..e7ae4f0bef 100644
>>> --- a/target/mips/tcg/sysemu/tlb_helper.c
>>> +++ b/target/mips/tcg/sysemu/tlb_helper.c
>>> @@ -940,8 +940,9 @@ 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 ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
>>> -                           MMU_ERL_IDX : MMU_KERNEL_IDX);
>>> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL)
>>> +                          ? MMU_ERL_IDX
>>> +                          : (env->hflags & MIPS_HFLAG_KSU);
>>
>> This contradicts the comment above.
>> If this code change is correct, then the comment isn't.
> 
> OK.
> 
>> But the comment certainly makes sense -- page tables are never accessible to user mode.
> 
> But we are still dropping the supervisor mode, so OK if I
> reword as:
> 
> "Restore the previous behavior to also get supervisor modes."

The old code

-        env->hflags &= ~MIPS_HFLAG_KSU;

drops both supervisor and user bits, so my comment still stands.


r~

Re: [PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[Philippe Mathieu-Daudé]

On 12/8/24 09:02, Richard Henderson wrote:
> On 8/12/24 15:35, Philippe Mathieu-Daudé wrote:
>> On 12/8/24 02:48, Richard Henderson wrote:
>>> On 8/12/24 02:54, Philippe Mathieu-Daudé wrote:
>>>> When refactoring page_table_walk_refill() in commit 4e999bf419
>>>> we replaced the execution mode and forced it to kernel mode.
>>>> Restore the previous behavior to also get supervisor / user modes.
>>>>
>>>> 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 | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/mips/tcg/sysemu/tlb_helper.c 
>>>> b/target/mips/tcg/sysemu/tlb_helper.c
>>>> index 3ba6d369a6..e7ae4f0bef 100644
>>>> --- a/target/mips/tcg/sysemu/tlb_helper.c
>>>> +++ b/target/mips/tcg/sysemu/tlb_helper.c
>>>> @@ -940,8 +940,9 @@ 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 ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
>>>> -                           MMU_ERL_IDX : MMU_KERNEL_IDX);
>>>> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL)
>>>> +                          ? MMU_ERL_IDX
>>>> +                          : (env->hflags & MIPS_HFLAG_KSU);
>>>
>>> This contradicts the comment above.
>>> If this code change is correct, then the comment isn't.
>>
>> OK.
>>
>>> But the comment certainly makes sense -- page tables are never 
>>> accessible to user mode.
>>
>> But we are still dropping the supervisor mode, so OK if I
>> reword as:
>>
>> "Restore the previous behavior to also get supervisor modes."
> 
> The old code
> 
> -        env->hflags &= ~MIPS_HFLAG_KSU;
> 
> drops both supervisor and user bits, so my comment still stands.

Right, I missed that.

The issue is in get_pte(), we have:

   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.

I'll respin the fix.

Phil.

Re: [PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill()

[Jiaxun Yang]

在2024年8月11日八月 下午5:54，Philippe Mathieu-Daudé写道：
> When refactoring page_table_walk_refill() in commit 4e999bf419
> we replaced the execution mode and forced it to kernel mode.
> Restore the previous behavior to also get supervisor / user modes.
>
> 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>

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Thanks!

> ---
>  target/mips/tcg/sysemu/tlb_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/mips/tcg/sysemu/tlb_helper.c 
> b/target/mips/tcg/sysemu/tlb_helper.c
> index 3ba6d369a6..e7ae4f0bef 100644
> --- a/target/mips/tcg/sysemu/tlb_helper.c
> +++ b/target/mips/tcg/sysemu/tlb_helper.c
> @@ -940,8 +940,9 @@ 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 ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL ?
> -                           MMU_ERL_IDX : MMU_KERNEL_IDX);
> +        int ptw_mmu_idx = (env->hflags & MIPS_HFLAG_ERL)
> +                          ? MMU_ERL_IDX
> +                          : (env->hflags & MIPS_HFLAG_KSU);
> 
>          if (page_table_walk_refill(env, address, ptw_mmu_idx)) {
>              ret = get_physical_address(env, &physical, &prot, address,
> -- 
> 2.45.2

-- 
- Jiaxun