tcg: Improvements to atomic128

[PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Richard Henderson 2 years, 8 months ago

PAGE_WRITE is current writability, as modified by TB protection;
PAGE_WRITE_ORG is the original page writability.

Fixes: cdfac37be0d ("accel/tcg: Honor atomicity of loads")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/ldst_atomicity.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
index 0f6b3f8ab6..57163f5ca2 100644
--- a/accel/tcg/ldst_atomicity.c.inc
+++ b/accel/tcg/ldst_atomicity.c.inc
@@ -191,7 +191,7 @@ static Int128 load_atomic16_or_exit(CPUArchState *env, uintptr_t ra, void *pv)
      * another process, because the fallback start_exclusive solution
      * provides no protection across processes.
      */
-    if (!page_check_range(h2g(p), 16, PAGE_WRITE)) {
+    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
         return *p;
     }
 #endif
-- 
2.34.1

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Peter Maydell 2 years, 8 months ago

On Fri, 26 May 2023 at 01:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> PAGE_WRITE is current writability, as modified by TB protection;
> PAGE_WRITE_ORG is the original page writability.
>
> Fixes: cdfac37be0d ("accel/tcg: Honor atomicity of loads")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/ldst_atomicity.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
> index 0f6b3f8ab6..57163f5ca2 100644
> --- a/accel/tcg/ldst_atomicity.c.inc
> +++ b/accel/tcg/ldst_atomicity.c.inc
> @@ -191,7 +191,7 @@ static Int128 load_atomic16_or_exit(CPUArchState *env, uintptr_t ra, void *pv)
>       * another process, because the fallback start_exclusive solution
>       * provides no protection across processes.
>       */
> -    if (!page_check_range(h2g(p), 16, PAGE_WRITE)) {
> +    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
>          return *p;
>      }
>  #endif
> --
> 2.34.1

load_atomic8_or_exit() has a similar condition, so
we should change either both or neither.

So, if I understand this correctly, !PAGE_WRITE_ORG is a
stricter test than !PAGE_WRITE, so we're saying "don't
do a simple non-atomic load if the page was only read-only
because we've translated code out of it". Why is it
not OK to do the non-atomic load in that case? I guess
because we don't have the mmap lock, so some other thread
might nip in and do an access that causes us to invalidate
the TBs and move the page back to writeable?

thanks
-- PMM

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Richard Henderson 2 years, 8 months ago

On 5/30/23 06:44, Peter Maydell wrote:
> On Fri, 26 May 2023 at 01:24, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> PAGE_WRITE is current writability, as modified by TB protection;
>> PAGE_WRITE_ORG is the original page writability.
>>
>> Fixes: cdfac37be0d ("accel/tcg: Honor atomicity of loads")
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   accel/tcg/ldst_atomicity.c.inc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
>> index 0f6b3f8ab6..57163f5ca2 100644
>> --- a/accel/tcg/ldst_atomicity.c.inc
>> +++ b/accel/tcg/ldst_atomicity.c.inc
>> @@ -191,7 +191,7 @@ static Int128 load_atomic16_or_exit(CPUArchState *env, uintptr_t ra, void *pv)
>>        * another process, because the fallback start_exclusive solution
>>        * provides no protection across processes.
>>        */
>> -    if (!page_check_range(h2g(p), 16, PAGE_WRITE)) {
>> +    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
>>           return *p;
>>       }
>>   #endif
>> --
>> 2.34.1
> 
> load_atomic8_or_exit() has a similar condition, so
> we should change either both or neither.
> 
> So, if I understand this correctly, !PAGE_WRITE_ORG is a
> stricter test than !PAGE_WRITE, so we're saying "don't
> do a simple non-atomic load if the page was only read-only
> because we've translated code out of it". Why is it
> not OK to do the non-atomic load in that case? I guess
> because we don't have the mmap lock, so some other thread
> might nip in and do an access that causes us to invalidate
> the TBs and move the page back to writeable?

This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then the page is 
really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will kill the process.


r~

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Peter Maydell 2 years, 8 months ago

On Tue, 30 May 2023 at 14:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/30/23 06:44, Peter Maydell wrote:
> > On Fri, 26 May 2023 at 01:24, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> PAGE_WRITE is current writability, as modified by TB protection;
> >> PAGE_WRITE_ORG is the original page writability.
> >>
> >> Fixes: cdfac37be0d ("accel/tcg: Honor atomicity of loads")
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   accel/tcg/ldst_atomicity.c.inc | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
> >> index 0f6b3f8ab6..57163f5ca2 100644
> >> --- a/accel/tcg/ldst_atomicity.c.inc
> >> +++ b/accel/tcg/ldst_atomicity.c.inc
> >> @@ -191,7 +191,7 @@ static Int128 load_atomic16_or_exit(CPUArchState *env, uintptr_t ra, void *pv)
> >>        * another process, because the fallback start_exclusive solution
> >>        * provides no protection across processes.
> >>        */
> >> -    if (!page_check_range(h2g(p), 16, PAGE_WRITE)) {
> >> +    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
> >>           return *p;
> >>       }
> >>   #endif
> >> --
> >> 2.34.1
> >
> > load_atomic8_or_exit() has a similar condition, so
> > we should change either both or neither.
> >
> > So, if I understand this correctly, !PAGE_WRITE_ORG is a
> > stricter test than !PAGE_WRITE, so we're saying "don't
> > do a simple non-atomic load if the page was only read-only
> > because we've translated code out of it". Why is it
> > not OK to do the non-atomic load in that case? I guess
> > because we don't have the mmap lock, so some other thread
> > might nip in and do an access that causes us to invalidate
> > the TBs and move the page back to writeable?
>
> This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then the page is
> really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will kill the process.

Right, but if !PAGE_WRITE_ORG then that implies also !PAGE_WRITE,
so we do that even without this change ?

-- PMM

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Richard Henderson 2 years, 8 months ago

On 5/30/23 07:06, Peter Maydell wrote:
>> This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then the page is
>> really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will kill the process.
> 
> Right, but if !PAGE_WRITE_ORG then that implies also !PAGE_WRITE,
> so we do that even without this change ?

But !PAGE_WRITE does not imply !PAGE_WRITE_ORG.
This fails tests/tcg/multiarch/munmap-pthread.c.


r~

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Peter Maydell 2 years, 8 months ago

On Tue, 30 May 2023 at 15:29, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/30/23 07:06, Peter Maydell wrote:
> >> This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then the page is
> >> really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will kill the process.
> >
> > Right, but if !PAGE_WRITE_ORG then that implies also !PAGE_WRITE,
> > so we do that even without this change ?
>
> But !PAGE_WRITE does not imply !PAGE_WRITE_ORG.

Hmm. In what situation do we mark a page writeable when the
guest didn't ask for it to be writeable ?

-- PMM

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Richard Henderson 2 years, 8 months ago

On 5/30/23 07:48, Peter Maydell wrote:
> On Tue, 30 May 2023 at 15:29, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 5/30/23 07:06, Peter Maydell wrote:
>>>> This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then the page is
>>>> really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will kill the process.
>>>
>>> Right, but if !PAGE_WRITE_ORG then that implies also !PAGE_WRITE,
>>> so we do that even without this change ?
>>
>> But !PAGE_WRITE does not imply !PAGE_WRITE_ORG.
> 
> Hmm. In what situation do we mark a page writeable when the
> guest didn't ask for it to be writeable ?

I don't know -- it seems backward, I know.

I *think* it's a race condition, where PAGE_WRITE changes.
That's what the test case is trying to provoke, anyway.


r~

Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

Posted by Peter Maydell 2 years, 8 months ago

On Tue, 30 May 2023 at 16:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/30/23 07:48, Peter Maydell wrote:
> > On Tue, 30 May 2023 at 15:29, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 5/30/23 07:06, Peter Maydell wrote:
> >>>> This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then the page is
> >>>> really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will kill the process.
> >>>
> >>> Right, but if !PAGE_WRITE_ORG then that implies also !PAGE_WRITE,
> >>> so we do that even without this change ?
> >>
> >> But !PAGE_WRITE does not imply !PAGE_WRITE_ORG.
> >
> > Hmm. In what situation do we mark a page writeable when the
> > guest didn't ask for it to be writeable ?
>
> I don't know -- it seems backward, I know.
>
> I *think* it's a race condition, where PAGE_WRITE changes.
> That's what the test case is trying to provoke, anyway.

That sounds like the theory I had earlier, that we
don't have the mmap lock, so the other thread can
get in and turn the RO-only-because-of-the-JIT page
back to RW, so we don't want to do the non-atomic
access for the "RO-only-because-of-JIT" cases.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM