| Message ID | 20220503163904.22575-1-leandro.lupori@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Fix tlbie | expand |
On 5/3/22 18:39, Leandro Lupori wrote: > Commit 74c4912f097bab98 changed check_tlb_flush() to use > tlb_flush_all_cpus_synced() instead of calling tlb_flush() on each > CPU. However, as side effect of this, a CPU executing a ptesync > after a tlbie will have its TLB flushed only after exiting its > current Translation Block (TB). > > This causes memory accesses to invalid pages to succeed, if they > happen to be on the same TB as the ptesync. How did you track the issue ? Do you have a test case ? Thanks, C. > To fix this, use tlb_flush_all_cpus() instead, that immediately > flushes the TLB of the CPU executing the ptesync instruction. > > Fixes: 74c4912f097bab98 ("target/ppc: Fix synchronization of mttcg with broadcast TLB flushes") > Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> > --- > target/ppc/helper_regs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 9a691d6833..1fa032e4d0 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -293,7 +293,7 @@ void check_tlb_flush(CPUPPCState *env, bool global) > if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) { > env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH; > env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH; > - tlb_flush_all_cpus_synced(cs); > + tlb_flush_all_cpus(cs); > return; > } >
On 5/3/22 13:54, Cédric Le Goater wrote: > On 5/3/22 18:39, Leandro Lupori wrote: >> Commit 74c4912f097bab98 changed check_tlb_flush() to use >> tlb_flush_all_cpus_synced() instead of calling tlb_flush() on each >> CPU. However, as side effect of this, a CPU executing a ptesync >> after a tlbie will have its TLB flushed only after exiting its >> current Translation Block (TB). >> >> This causes memory accesses to invalid pages to succeed, if they >> happen to be on the same TB as the ptesync. > > How did you track the issue ? Do you have a test case ? > I've initially found it with a hacked Linux kernel module that I was using to test tlbie behavior, before trying to improve its implementation to only invalidate the needed entries. Now I've added a new test to those MMU tests from pnv-test, to be able to reproduce and test it more easily. I've not included it because it depends on other code from MMU tests and semihosting or attn. But you can check it here: https://github.com/PPC64/qemu/commit/ccb60e4b950d1376b7f5d72843f6ce082a1a9edb (mmu_test_18) Thanks, Leandro > Thanks, > > C. >
Leandro Lupori <leandro.lupori@eldorado.org.br> writes: > Commit 74c4912f097bab98 changed check_tlb_flush() to use > tlb_flush_all_cpus_synced() instead of calling tlb_flush() on each > CPU. However, as side effect of this, a CPU executing a ptesync > after a tlbie will have its TLB flushed only after exiting its > current Translation Block (TB). > > This causes memory accesses to invalid pages to succeed, if they > happen to be on the same TB as the ptesync. > > To fix this, use tlb_flush_all_cpus() instead, that immediately > flushes the TLB of the CPU executing the ptesync instruction. > > Fixes: 74c4912f097bab98 ("target/ppc: Fix synchronization of mttcg with broadcast TLB flushes") > Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > target/ppc/helper_regs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 9a691d6833..1fa032e4d0 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -293,7 +293,7 @@ void check_tlb_flush(CPUPPCState *env, bool global) > if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) { > env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH; > env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH; > - tlb_flush_all_cpus_synced(cs); > + tlb_flush_all_cpus(cs); > return; > }
Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel On 5/3/22 13:39, Leandro Lupori wrote: > Commit 74c4912f097bab98 changed check_tlb_flush() to use > tlb_flush_all_cpus_synced() instead of calling tlb_flush() on each > CPU. However, as side effect of this, a CPU executing a ptesync > after a tlbie will have its TLB flushed only after exiting its > current Translation Block (TB). > > This causes memory accesses to invalid pages to succeed, if they > happen to be on the same TB as the ptesync. > > To fix this, use tlb_flush_all_cpus() instead, that immediately > flushes the TLB of the CPU executing the ptesync instruction. > > Fixes: 74c4912f097bab98 ("target/ppc: Fix synchronization of mttcg with broadcast TLB flushes") > Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> > --- > target/ppc/helper_regs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 9a691d6833..1fa032e4d0 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -293,7 +293,7 @@ void check_tlb_flush(CPUPPCState *env, bool global) > if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) { > env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH; > env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH; > - tlb_flush_all_cpus_synced(cs); > + tlb_flush_all_cpus(cs); > return; > } >
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c index 9a691d6833..1fa032e4d0 100644 --- a/target/ppc/helper_regs.c +++ b/target/ppc/helper_regs.c @@ -293,7 +293,7 @@ void check_tlb_flush(CPUPPCState *env, bool global) if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) { env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH; env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH; - tlb_flush_all_cpus_synced(cs); + tlb_flush_all_cpus(cs); return; }
Commit 74c4912f097bab98 changed check_tlb_flush() to use tlb_flush_all_cpus_synced() instead of calling tlb_flush() on each CPU. However, as side effect of this, a CPU executing a ptesync after a tlbie will have its TLB flushed only after exiting its current Translation Block (TB). This causes memory accesses to invalid pages to succeed, if they happen to be on the same TB as the ptesync. To fix this, use tlb_flush_all_cpus() instead, that immediately flushes the TLB of the CPU executing the ptesync instruction. Fixes: 74c4912f097bab98 ("target/ppc: Fix synchronization of mttcg with broadcast TLB flushes") Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> --- target/ppc/helper_regs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)