| Message ID | 20230807141846.786530-2-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Always pass the full write size to notdirty_write() | expand |
On 8/7/23 06:56, Ilya Leoshkevich wrote: > One of notdirty_write()'s responsibilities is detecting self-modifying > code. Some functions pass the full size of a write to it, some pass 1. > When a write to a code section begins before a TB start, but then > overlaps the TB, the paths that pass 1 don't flush a TB and don't > return to the translator loop. > > This may be masked, one example being HELPER(vstl). There, > probe_write_access() ultimately calls notdirty_write() with a size of > 1 and misses self-modifying code. However, cpu_stq_be_data_ra() > ultimately calls mmu_watch_or_dirty(), which in turn calls > notdirty_write() with the full size. > > It's still worth improving this, because there may still be > user-visible adverse effects in other helpers. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> IIRC there are some uses of probe_access_* that set size == 0. Should we adjust addr+size to cover the whole page for that case? That seems to be the intent, anyway. r~ > --- > accel/tcg/cputlb.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index d68fa6867ce..aa3cffbc11a 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1582,7 +1582,7 @@ int probe_access_full(CPUArchState *env, vaddr addr, int size, > > /* Handle clean RAM pages. */ > if (unlikely(flags & TLB_NOTDIRTY)) { > - notdirty_write(env_cpu(env), addr, 1, *pfull, retaddr); > + notdirty_write(env_cpu(env), addr, size, *pfull, retaddr); > flags &= ~TLB_NOTDIRTY; > } > > @@ -1605,7 +1605,7 @@ int probe_access_full_mmu(CPUArchState *env, vaddr addr, int size, > > /* Handle clean RAM pages. */ > if (unlikely(flags & TLB_NOTDIRTY)) { > - notdirty_write(env_cpu(env), addr, 1, *pfull, 0); > + notdirty_write(env_cpu(env), addr, size, *pfull, 0); > flags &= ~TLB_NOTDIRTY; > } > > @@ -1626,7 +1626,7 @@ int probe_access_flags(CPUArchState *env, vaddr addr, int size, > > /* Handle clean RAM pages. */ > if (unlikely(flags & TLB_NOTDIRTY)) { > - notdirty_write(env_cpu(env), addr, 1, full, retaddr); > + notdirty_write(env_cpu(env), addr, size, full, retaddr); > flags &= ~TLB_NOTDIRTY; > } > > @@ -1661,7 +1661,7 @@ void *probe_access(CPUArchState *env, vaddr addr, int size, > > /* Handle clean RAM pages. */ > if (flags & TLB_NOTDIRTY) { > - notdirty_write(env_cpu(env), addr, 1, full, retaddr); > + notdirty_write(env_cpu(env), addr, size, full, retaddr); > } > } >
On Mon, 2023-08-07 at 11:21 -0700, Richard Henderson wrote: > On 8/7/23 06:56, Ilya Leoshkevich wrote: > > One of notdirty_write()'s responsibilities is detecting self- > > modifying > > code. Some functions pass the full size of a write to it, some pass > > 1. > > When a write to a code section begins before a TB start, but then > > overlaps the TB, the paths that pass 1 don't flush a TB and don't > > return to the translator loop. > > > > This may be masked, one example being HELPER(vstl). There, > > probe_write_access() ultimately calls notdirty_write() with a size > > of > > 1 and misses self-modifying code. However, cpu_stq_be_data_ra() > > ultimately calls mmu_watch_or_dirty(), which in turn calls > > notdirty_write() with the full size. > > > > It's still worth improving this, because there may still be > > user-visible adverse effects in other helpers. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > IIRC there are some uses of probe_access_* that set size == 0. > Should we adjust addr+size to cover the whole page for that case? > That seems to be the intent, anyway. There is a comment that says we shouldn't do watchpoint/smc detection in this case: /* Per the interface, size == 0 merely faults the access. */ if (size == 0) { return NULL; } Come to think of it, qemu-user works this way too: SMC is detected on the actual access, not the probe: helper_vstl() cpu_stq_be_data_ra() ... stq_he_p() <signal handler called> host_signal_handler() handle_sigsegv_accerr_write() page_unprotect() tb_invalidate_phys_page_unwind() cpu_loop_exit_noexc() So all this is probably fine, I now think it's better to leave the code as is, especially given that I cannot reproduce the original problem anymore.
On 8/8/23 02:59, Ilya Leoshkevich wrote: > On Mon, 2023-08-07 at 11:21 -0700, Richard Henderson wrote: >> IIRC there are some uses of probe_access_* that set size == 0. >> Should we adjust addr+size to cover the whole page for that case? >> That seems to be the intent, anyway. > > There is a comment that says we shouldn't do watchpoint/smc detection > in this case: > > /* Per the interface, size == 0 merely faults the access. */ > if (size == 0) { > return NULL; > } > > Come to think of it, qemu-user works this way too: SMC is detected on > the actual access, not the probe: > > helper_vstl() > cpu_stq_be_data_ra() > ... > stq_he_p() > <signal handler called> > host_signal_handler() > handle_sigsegv_accerr_write() > page_unprotect() > tb_invalidate_phys_page_unwind() > cpu_loop_exit_noexc() > > So all this is probably fine, I now think it's better to leave the code > as is, especially given that I cannot reproduce the original problem > anymore. Ok then. r~
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index d68fa6867ce..aa3cffbc11a 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1582,7 +1582,7 @@ int probe_access_full(CPUArchState *env, vaddr addr, int size, /* Handle clean RAM pages. */ if (unlikely(flags & TLB_NOTDIRTY)) { - notdirty_write(env_cpu(env), addr, 1, *pfull, retaddr); + notdirty_write(env_cpu(env), addr, size, *pfull, retaddr); flags &= ~TLB_NOTDIRTY; } @@ -1605,7 +1605,7 @@ int probe_access_full_mmu(CPUArchState *env, vaddr addr, int size, /* Handle clean RAM pages. */ if (unlikely(flags & TLB_NOTDIRTY)) { - notdirty_write(env_cpu(env), addr, 1, *pfull, 0); + notdirty_write(env_cpu(env), addr, size, *pfull, 0); flags &= ~TLB_NOTDIRTY; } @@ -1626,7 +1626,7 @@ int probe_access_flags(CPUArchState *env, vaddr addr, int size, /* Handle clean RAM pages. */ if (unlikely(flags & TLB_NOTDIRTY)) { - notdirty_write(env_cpu(env), addr, 1, full, retaddr); + notdirty_write(env_cpu(env), addr, size, full, retaddr); flags &= ~TLB_NOTDIRTY; } @@ -1661,7 +1661,7 @@ void *probe_access(CPUArchState *env, vaddr addr, int size, /* Handle clean RAM pages. */ if (flags & TLB_NOTDIRTY) { - notdirty_write(env_cpu(env), addr, 1, full, retaddr); + notdirty_write(env_cpu(env), addr, size, full, retaddr); } }
One of notdirty_write()'s responsibilities is detecting self-modifying code. Some functions pass the full size of a write to it, some pass 1. When a write to a code section begins before a TB start, but then overlaps the TB, the paths that pass 1 don't flush a TB and don't return to the translator loop. This may be masked, one example being HELPER(vstl). There, probe_write_access() ultimately calls notdirty_write() with a size of 1 and misses self-modifying code. However, cpu_stq_be_data_ra() ultimately calls mmu_watch_or_dirty(), which in turn calls notdirty_write() with the full size. It's still worth improving this, because there may still be user-visible adverse effects in other helpers. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- accel/tcg/cputlb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)