Re: [PATCH RFC 1/1] tcg: Always pass the full write size to notdirty_write()

[Richard Henderson]

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);
>           }
>       }
>