diff mbox series

[PULL,46/47] accel/tcg: Handle false negative lookup in page_check_range

Message ID 20221230000221.2764875-47-richard.henderson@linaro.org
State Accepted
Commit e630c0126c561b7db26790e8288c4fe9b61ae8dc
Headers show
Series [PULL,01/47] tcg: convert tcg/README to rst | expand

Commit Message

Richard Henderson Dec. 30, 2022, 12:02 a.m. UTC 
As in page_get_flags, we need to try again with the mmap
lock held if we fail a page lookup.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/user-exec.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

Comments

Philippe Mathieu-Daudé Dec. 30, 2022, 1:17 p.m. UTC | #1 
On 30/12/22 01:02, Richard Henderson wrote:
> As in page_get_flags, we need to try again with the mmap
> lock held if we fail a page lookup.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/user-exec.c | 41 ++++++++++++++++++++++++++++++++++-------
>   1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 2c5c10d2e6..a8eb63ab96 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -525,6 +525,8 @@ 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;  /* tri-state: =0: unlocked, +1: global, -1: local */

Thanks :)
diff mbox series

Patch

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 2c5c10d2e6..a8eb63ab96 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -525,6 +525,8 @@  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;  /* tri-state: =0: unlocked, +1: global, -1: local */
+    int ret;
 
     if (len == 0) {
         return 0;  /* trivial length */
@@ -535,42 +537,67 @@  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;
+                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;
     }
+
+    /* Release the lock if acquired locally. */
+    if (locked < 0) {
+        mmap_unlock();
+    }
+    return ret;
 }
 
 void page_protect(tb_page_addr_t address)