Re: [PATCH for-8.1] accel/tcg: Take mmap_lock in load_atomic16_or_exit

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 16 Jul 2023 at 18:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For user-only, the probe for page writability may race with another
> thread's mprotect.  Take the mmap_lock around the operation.  This
> is still faster than the start/end_exclusive fallback.
>
> Remove the write probe in load_atomic8_or_exit.  There we don't have
> the same machinery for testing the existance of an 8-byte cmpxchg.

"existence"

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/ldst_atomicity.c.inc | 54 +++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 30 deletions(-)
>
> diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
> index 4de0a80492..e7170f8ba2 100644
> --- a/accel/tcg/ldst_atomicity.c.inc
> +++ b/accel/tcg/ldst_atomicity.c.inc
> @@ -152,19 +152,6 @@ static uint64_t load_atomic8_or_exit(CPUArchState *env, uintptr_t ra, void *pv)
>          return load_atomic8(pv);
>      }
>
> -#ifdef CONFIG_USER_ONLY
> -    /*
> -     * If the page is not writable, then assume the value is immutable
> -     * and requires no locking.  This ignores the case of MAP_SHARED with
> -     * another process, because the fallback start_exclusive solution
> -     * provides no protection across processes.
> -     */
> -    if (page_check_range(h2g(pv), 8, PAGE_WRITE_ORG)) {
> -        uint64_t *p = __builtin_assume_aligned(pv, 8);
> -        return *p;
> -    }
> -#endif

I don't really understand the comment in the commit message:
why would it be wrong to wrap this "test writeability and
do the operation" in the mmap-lock, the same way we do for the
16-byte case?

thanks
-- PMM