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.hender...@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; }
