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;
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;
+ 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) {
+ mmap_unlock();
+ }
+ return ret;
}
void page_protect(tb_page_addr_t address)
--
2.34.1On 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;
> }
On 12/27/22 23: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?
No, we may have entered with mmap_locked. Only unlocking if the lock was taken locally.
r~
On 28/12/22 18:36, Richard Henderson wrote:
> On 12/27/22 23: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?
>
> No, we may have entered with mmap_locked. Only unlocking if the lock
> was taken locally.
Oh, so you are using this variable as tri-state?
On 12/28/22 10:27, Philippe Mathieu-Daudé wrote: > Oh, so you are using this variable as tri-state? Yes. Perhaps a comment on the -1? r~
On 28/12/22 19:30, Richard Henderson wrote: > On 12/28/22 10:27, Philippe Mathieu-Daudé wrote: >> Oh, so you are using this variable as tri-state? > > Yes. Perhaps a comment on the -1? Or name the variable tristate_lock? And a comment :) (No need to respin, you can keep my R-b).
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>
© 2016 - 2025 Red Hat, Inc.