| Message ID | 20230526002334.1760495-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Improvements to atomic128 | expand |
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
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~
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
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~
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
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~
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
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
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(-)