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 <address@hidden>
> Cc: Bastian Koppelmann <address@hidden>
> Signed-off-by: Michael Clark <address@hidden>
> Signed-off-by: Palmer Dabbelt <address@hidden>
> ---
>  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;
>                  }
>              }
>  
>