Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Peter Maydell]

On 21 March 2018 at 13:55, Paolo Bonzini  wrote:
> On 19/03/2018 22:07, Michael Clark wrote:
>> We need to use atomic_cmpxchg
>> in the PTE update to detect the case where the PTE has changed between
>> reading it and updating the accessed dirty bits.
>
> Yes, this makes sense.  In fact having such a function (more precisely
> address_space_atomic_cmpxchg) would be useful for x86 too.  Right now
> x86 is wrong in not using cmpxchg.

Yeah, this is a known missing feature in our APIs for memory
accesses (it only starts to matter with MTTCG, really). We
ought to have functions that guarantee that they do the
access as a single 32/64 bit load/store, as well as
having atomic support. PPC and Arm TLB walk code will need
these. For the moment we just ignore the possibility of
races here, but for the 2.13 timeframe we really ought to
design a solution to this properly.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Paolo Bonzini]

On 19/03/2018 22:07, Michael Clark wrote:
> 
> 
> On Mon, Mar 19, 2018 at 2:41 AM, Paolo Bonzini  > wrote:
> 
> On 16/03/2018 20:41, Michael Clark wrote:
> > From reading other code that accesses memory regions directly,
> > it appears that the rcu_read_lock needs to be held. Note: the
> > original code for accessing RAM directly was added because
> > there is no other way to use atomic_cmpxchg on guest physical
> > address space.
> >
> > Cc: Sagar Karandikar  >
> > Cc: Bastian Koppelmann  >
> > Signed-off-by: Michael Clark mailto:m...@sifive.com>>
> > Signed-off-by: Palmer Dabbelt  >
> > ---
> >  target/riscv/helper.c | 14 --
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> I think the bug here is that rcu_read_lock/unlock is missing in
> cpu_memory_rw_debug.  Are there any other callers you had in mind?
> 
> 
> So this is not a bug in our patch per se, rather a bug in
> cpu_memory_rw_debug?

Yes.

> It seems that most other code that that uses the address_space_*
> interfaces (e.g. hw/virtio/virtio.c) holds the rcu_read_lock, presumably
> to handle cases where the memory map might change at runtime, e.g.
> during ballooning. This would be exhibited if a page walk collided with
> a balloon.

Note that hw/virtio/virtio.c calls rcu_read_lock() not so much to
protect from memory map changes, but rather to protect reads of
vq->vring.caches.

In general, exec.c and memory.c take care of taking the lock.  Functions
that need the caller to take the lock are annotated with something like

/* Called from RCU critical section.  */

(see for example flatview_write).

> Ideally we could add a cpu_physical_memory_atomic_cmpxchg API to generic
> code, and we could avoid holding the rcu_read_lock in target/riscv i.e.
> encapsulate guest physical memory atomic_cmpxchg. The atomic_cmpxchg
> wrapper handles multiple word widthes, so we would
> need cpu_physical_memory_atomic_cmpxchg32
> and cpu_physical_memory_atomic_cmpxchg64. We need to use atomic_cmpxchg
> in the PTE update to detect the case where the PTE has changed between
> reading it and updating the accessed dirty bits.

Yes, this makes sense.  In fact having such a function (more precisely
address_space_atomic_cmpxchg) would be useful for x86 too.  Right now
x86 is wrong in not using cmpxchg.

Even without such a function, however, I have two more things that I
noticed:

1) you're using a very low-level function, which complicates things a
bit for yourself.  You can use address_space_map/unmap too, which is a
bit simpler.

2) I'm wondering if the page walk needs to use load-acquire instructions
(and I cannot really find the relevant text in the privileged spec)

> This is an interesting constraint. I believe the intent is that we just
> AMOOR bit A+D bits on the PTE (although we don't have AMOOR, we have
> atomic_cmpxchg), however we have read a stronger interpretation (while
> stronger, it is not incorrect),

Stronger is never incorrect. :)

> and that is that the atomicity guarantee
> extends from the page walker reading the PTE entry, checking its
> permissions and then updating it, which in hardware would require the
> page walker to take load reservations, and I don't think it does.

Indeed.  But maybe another possibility could be to do an atomic_cmpxchg
and ignore it if the comparison failed?  This would be susceptible to
ABA problems, but apart from that it would be the same as taking a load
reservation (in assembly language tems, that's
load-link/or/store-conditional).

> Apparently the hardware just AMOORs the bits, so the PTE could actually
> be pointing somewhere else by the time we go to update the bits,
> although it is the same virtual address has been accessed or dirtied
> (albiet with a different physical translation). In any case, we have a
> very strong interpretation of the specification wording, potentially
> stronger than hardware may provide. We had a long discussion about this
> on the RISC-V QEMU issue tracker. Stefan O'Rear mentioned he might make
> a test that hammers a PTE from another thread while one thread is doing
> a page walk to try to cause the page walker to set the A+D bits on a PTE
> that is different to the one it used to create the virtual to physical
> mapping. That's some background around why the code exists in the first
> place, which provides the strongest possible gaurantee on PTE atomicity.
> 
> - https://github.com/riscv/riscv-qemu/pull/103
> - https://github.com/riscv/riscv-qemu/pull/105
> 
> The get_physical_address bug that this patch fixes does not show up in
> practice. i.e. we aren't actually hitting cases where we have page walks
> colliding with address space changes, however I think
> holding rcu_read_lock seems to be correct and this bug may show up in
> the future.

Yes, it is correct, but it s

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Michael Clark]

On Mon, Mar 19, 2018 at 2:41 AM, Paolo Bonzini  wrote:

> On 16/03/2018 20:41, Michael Clark wrote:
> > From reading other code that accesses memory regions directly,
> > it appears that the rcu_read_lock needs to be held. Note: the
> > original code for accessing RAM directly was added because
> > there is no other way to use atomic_cmpxchg on guest physical
> > address space.
> >
> > Cc: Sagar Karandikar 
> > Cc: Bastian Koppelmann 
> > Signed-off-by: Michael Clark 
> > Signed-off-by: Palmer Dabbelt 
> > ---
> >  target/riscv/helper.c | 14 --
> >  1 file changed, 12 insertions(+), 2 deletions(-)
>
> I think the bug here is that rcu_read_lock/unlock is missing in
> cpu_memory_rw_debug.  Are there any other callers you had in mind?


So this is not a bug in our patch per se, rather a bug in
cpu_memory_rw_debug?

It seems that most other code that that uses the address_space_* interfaces
(e.g. hw/virtio/virtio.c) holds the rcu_read_lock, presumably to handle
cases where the memory map might change at runtime, e.g. during ballooning.
This would be exhibited if a page walk collided with a balloon.

Ideally we could add a cpu_physical_memory_atomic_cmpxchg API to generic
code, and we could avoid holding the rcu_read_lock in target/riscv i.e.
encapsulate guest physical memory atomic_cmpxchg. The atomic_cmpxchg
wrapper handles multiple word widthes, so we would need
cpu_physical_memory_atomic_cmpxchg32
and cpu_physical_memory_atomic_cmpxchg64. We need to use atomic_cmpxchg in
the PTE update to detect the case where the PTE has changed between reading
it and updating the accessed dirty bits. As far as I can tell, the code is
correct. In fact we are reading a very strong interpretation of the RISC-V
specification.

This is what The RISC-V Instruction Set Manual Volume II: Privileged
Architecture says about accessed and dirty updates, which is the reason for
the code in question:

When a virtual page is accessed and the A bit is clear, or is written
and the D bit is clear,
the implementation sets the corresponding bit in the PTE. The PTE
update must be atomic
with respect to other accesses to the PTE, and must atomically check
that the PTE is valid
and grants suffi cient permissions. The PTE update must be exact (i.e.,
not speculative), and
observed in program order by the local hart. The ordering on loads and
stores provided by
FENCE instructions and the acquire/release bits on atomic instructions
also orders the PTE
updates associated with those loads and stores as observed by remote
harts.

This is an interesting constraint. I believe the intent is that we just
AMOOR bit A+D bits on the PTE (although we don't have AMOOR, we have
atomic_cmpxchg), however we have read a stronger interpretation (while
stronger, it is not incorrect), and that is that the atomicity guarantee
extends from the page walker reading the PTE entry, checking its
permissions and then updating it, which in hardware would require the page
walker to take load reservations, and I don't think it does. Apparently the
hardware just AMOORs the bits, so the PTE could actually be pointing
somewhere else by the time we go to update the bits, although it is the
same virtual address has been accessed or dirtied (albiet with a different
physical translation). In any case, we have a very strong interpretation of
the specification wording, potentially stronger than hardware may provide.
We had a long discussion about this on the RISC-V QEMU issue tracker.
Stefan O'Rear mentioned he might make a test that hammers a PTE from
another thread while one thread is doing a page walk to try to cause the
page walker to set the A+D bits on a PTE that is different to the one it
used to create the virtual to physical mapping. That's some background
around why the code exists in the first place, which provides the strongest
possible gaurantee on PTE atomicity.

- https://github.com/riscv/riscv-qemu/pull/103
- https://github.com/riscv/riscv-qemu/pull/105

The get_physical_address bug that this patch fixes does not show up in
practice. i.e. we aren't actually hitting cases where we have page walks
colliding with address space changes, however I think holding rcu_read_lock
seems to be correct and this bug may show up in the future.

> diff --git a/target/riscv/helper.c b/target/riscv/helper.c
> > index 02cbcea..e71633a 100644
> > --- a/target/riscv/helper.c
> > +++ b/target/riscv/helper.c
> > @@ -209,6 +209,9 @@ restart:
> > as the PTE is no longer valid */
> >  MemoryRegion *mr;
> >  hwaddr l = sizeof(target_ulong), addr1;
> > +enum { success, translate_fail, restart_walk} action =
> success;
> > +
> > +rcu_read_lock();
> >  mr = address_space_translate(cs->as, pte_addr,
> >  &addr1, &l, false);
> >  if (memory_access_is_direct(mr, true)) {
> > @@ -222,7 +225,7 @@ restart:
> >  ta

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Paolo Bonzini]

On 16/03/2018 20:41, Michael Clark wrote:
> From reading other code that accesses memory regions directly,
> it appears that the rcu_read_lock needs to be held. Note: the
> original code for accessing RAM directly was added because
> there is no other way to use atomic_cmpxchg on guest physical
> address space.
> 
> Cc: Sagar Karandikar 
> Cc: Bastian Koppelmann 
> Signed-off-by: Michael Clark 
> Signed-off-by: Palmer Dabbelt 
> ---
>  target/riscv/helper.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)

I think the bug here is that rcu_read_lock/unlock is missing in
cpu_memory_rw_debug.  Are there any other callers you had in mind?

Paolo

> diff --git a/target/riscv/helper.c b/target/riscv/helper.c
> index 02cbcea..e71633a 100644
> --- a/target/riscv/helper.c
> +++ b/target/riscv/helper.c
> @@ -209,6 +209,9 @@ restart:
> as the PTE is no longer valid */
>  MemoryRegion *mr;
>  hwaddr l = sizeof(target_ulong), addr1;
> +enum { success, translate_fail, restart_walk} action = 
> success;
> +
> +rcu_read_lock();
>  mr = address_space_translate(cs->as, pte_addr,
>  &addr1, &l, false);
>  if (memory_access_is_direct(mr, true)) {
> @@ -222,7 +225,7 @@ restart:
>  target_ulong old_pte =
>  atomic_cmpxchg(pte_pa, pte, updated_pte);
>  if (old_pte != pte) {
> -goto restart;
> +action = restart_walk;
>  } else {
>  pte = updated_pte;
>  }
> @@ -230,7 +233,14 @@ restart:
>  } else {
>  /* misconfigured PTE in ROM (AD bits are not preset) or
>   * PTE is in IO space and can't be updated atomically */
> -return TRANSLATE_FAIL;
> +action = translate_fail;
> +}
> +rcu_read_unlock();
> +
> +switch (action) {
> +case success: break;
> +case translate_fail: return TRANSLATE_FAIL;
> +case restart_walk: goto restart;
>  }
>  }
>  
> 

[Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Michael Clark]

>From reading other code that accesses memory regions directly,
it appears that the rcu_read_lock needs to be held. Note: the
original code for accessing RAM directly was added because
there is no other way to use atomic_cmpxchg on guest physical
address space.

Cc: Sagar Karandikar 
Cc: Bastian Koppelmann 
Signed-off-by: Michael Clark 
Signed-off-by: Palmer Dabbelt 
---
 target/riscv/helper.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index 02cbcea..e71633a 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -209,6 +209,9 @@ restart:
as the PTE is no longer valid */
 MemoryRegion *mr;
 hwaddr l = sizeof(target_ulong), addr1;
+enum { success, translate_fail, restart_walk} action = success;
+
+rcu_read_lock();
 mr = address_space_translate(cs->as, pte_addr,
 &addr1, &l, false);
 if (memory_access_is_direct(mr, true)) {
@@ -222,7 +225,7 @@ restart:
 target_ulong old_pte =
 atomic_cmpxchg(pte_pa, pte, updated_pte);
 if (old_pte != pte) {
-goto restart;
+action = restart_walk;
 } else {
 pte = updated_pte;
 }
@@ -230,7 +233,14 @@ restart:
 } else {
 /* misconfigured PTE in ROM (AD bits are not preset) or
  * PTE is in IO space and can't be updated atomically */
-return TRANSLATE_FAIL;
+action = translate_fail;
+}
+rcu_read_unlock();
+
+switch (action) {
+case success: break;
+case translate_fail: return TRANSLATE_FAIL;
+case restart_walk: goto restart;
 }
 }
 
-- 
2.7.0