| Message ID | 20240811165407.26312-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-9.1] target/mips: Fix execution mode in page_table_walk_refill() | expand |
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~
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." ?
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~
在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
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.
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,
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(-)