Re: [PATCH 3/4] accel/tcg: Handle false negative lookup in page_check_range

[Philippe Mathieu-Daudé]

On 24/12/22 16:18, Richard Henderson wrote:
> As in page_get_flags, we need to try again with the mmap
> lock held if we fail a page lookup.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 2c5c10d2e6..c72a806203 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -525,6 +525,7 @@ void page_set_flags(target_ulong start, target_ulong end, 
> int flags)
>   int page_check_range(target_ulong start, target_ulong len, int flags)
>   {
>       target_ulong last;
> +    int locked, ret;

have_mmap_lock() returns a boolean, can we declare 'locked'
as such, ...

>   
>       if (len == 0) {
>           return 0;  /* trivial length */
> @@ -535,42 +536,66 @@ int page_check_range(target_ulong start, target_ulong 
> len, int flags)
>           return -1; /* wrap around */
>       }
>   
> +    locked = have_mmap_lock();
>       while (true) {
>           PageFlagsNode *p = pageflags_find(start, last);
>           int missing;
>   
>           if (!p) {
> -            return -1; /* entire region invalid */
> +            if (!locked) {
> +                /*
> +                 * Lockless lookups have false negatives.
> +                 * Retry with the lock held.
> +                 */
> +                mmap_lock();
> +                locked = -1;

... skip this confusing assignation, ...

> +                p = pageflags_find(start, last);
> +            }
> +            if (!p) {
> +                ret = -1; /* entire region invalid */
> +                break;
> +            }
>           }
>           if (start < p->itree.start) {
> -            return -1; /* initial bytes invalid */
> +            ret = -1; /* initial bytes invalid */
> +            break;
>           }
>   
>           missing = flags & ~p->flags;
>           if (missing & PAGE_READ) {
> -            return -1; /* page not readable */
> +            ret = -1; /* page not readable */
> +            break;
>           }
>           if (missing & PAGE_WRITE) {
>               if (!(p->flags & PAGE_WRITE_ORG)) {
> -                return -1; /* page not writable */
> +                ret = -1; /* page not writable */
> +                break;
>               }
>               /* Asking about writable, but has been protected: undo. */
>               if (!page_unprotect(start, 0)) {
> -                return -1;
> +                ret = -1;
> +                break;
>               }
>               /* TODO: page_unprotect should take a range, not a single page. */
>               if (last - start < TARGET_PAGE_SIZE) {
> -                return 0; /* ok */
> +                ret = 0; /* ok */
> +                break;
>               }
>               start += TARGET_PAGE_SIZE;
>               continue;
>           }
>   
>           if (last <= p->itree.last) {
> -            return 0; /* ok */
> +            ret = 0; /* ok */
> +            break;
>           }
>           start = p->itree.last + 1;
>       }
> +
> +    if (locked < 0) {

... and check for !locked here?

> +        mmap_unlock();
> +    }
> +    return ret;
>   }