Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

[Peter Maydell]

On Fri, 26 May 2023 at 01:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> PAGE_WRITE is current writability, as modified by TB protection;
> PAGE_WRITE_ORG is the original page writability.
>
> Fixes: cdfac37be0d ("accel/tcg: Honor atomicity of loads")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/ldst_atomicity.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
> index 0f6b3f8ab6..57163f5ca2 100644
> --- a/accel/tcg/ldst_atomicity.c.inc
> +++ b/accel/tcg/ldst_atomicity.c.inc
> @@ -191,7 +191,7 @@ static Int128 load_atomic16_or_exit(CPUArchState *env, 
> uintptr_t ra, void *pv)
>       * another process, because the fallback start_exclusive solution
>       * provides no protection across processes.
>       */
> -    if (!page_check_range(h2g(p), 16, PAGE_WRITE)) {
> +    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
>          return *p;
>      }
>  #endif
> --
> 2.34.1

load_atomic8_or_exit() has a similar condition, so
we should change either both or neither.

So, if I understand this correctly, !PAGE_WRITE_ORG is a
stricter test than !PAGE_WRITE, so we're saying "don't
do a simple non-atomic load if the page was only read-only
because we've translated code out of it". Why is it
not OK to do the non-atomic load in that case? I guess
because we don't have the mmap lock, so some other thread
might nip in and do an access that causes us to invalidate
the TBs and move the page back to writeable?

thanks
-- PMM