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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 28/12/22 08:24, Philippe Mathieu-Daudé wrote:
> 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;
>>   }

Modulo using a boolean:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>