diff mbox

[v2,10/23] RISC-V: Hold rcu_read_lock when accessing memory

Message ID 1520568765-58189-11-git-send-email-mjc@sifive.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michael Clark March 9, 2018, 4:12 a.m. UTC 
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 <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
CC: Stefan O'Rear <sorear2@gmail.com>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 target/riscv/helper.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Philippe Mathieu-Daudé March 10, 2018, 8:45 p.m. UTC | #1 
Hi Michael,

On 03/09/2018 05:12 AM, 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 <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> CC: Stefan O'Rear <sorear2@gmail.com>
> Signed-off-by: Michael Clark <mjc@sifive.com>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>  target/riscv/helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target/riscv/helper.c b/target/riscv/helper.c
> index 02cbcea..228933c 100644
> --- a/target/riscv/helper.c
> +++ b/target/riscv/helper.c
> @@ -209,6 +209,7 @@ restart:
>                     as the PTE is no longer valid */
>                  MemoryRegion *mr;
>                  hwaddr l = sizeof(target_ulong), addr1;
> +                rcu_read_lock();
>                  mr = address_space_translate(cs->as, pte_addr,
>                      &addr1, &l, false);
>                  if (memory_access_is_direct(mr, true)) {
> @@ -222,16 +223,19 @@ restart:
>                      target_ulong old_pte =
>                          atomic_cmpxchg(pte_pa, pte, updated_pte);
>                      if (old_pte != pte) {
> +                        rcu_read_unlock();
>                          goto restart;
>                      } else {
>                          pte = updated_pte;
>                      }
>  #endif
>                  } else {
> +                    rcu_read_unlock();
>                      /* misconfigured PTE in ROM (AD bits are not preset) or
>                       * PTE is in IO space and can't be updated atomically */
>                      return TRANSLATE_FAIL;
>                  }
> +                rcu_read_unlock();

Can you refactor to have a unique pair of lock/unlock?
This would be less bug-prone.

Thanks,

Phil.

>              }
>  
>              /* for superpage mappings, make a fake leaf PTE for the TLB's
>
diff mbox

Patch

diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index 02cbcea..228933c 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -209,6 +209,7 @@  restart:
                    as the PTE is no longer valid */
                 MemoryRegion *mr;
                 hwaddr l = sizeof(target_ulong), addr1;
+                rcu_read_lock();
                 mr = address_space_translate(cs->as, pte_addr,
                     &addr1, &l, false);
                 if (memory_access_is_direct(mr, true)) {
@@ -222,16 +223,19 @@  restart:
                     target_ulong old_pte =
                         atomic_cmpxchg(pte_pa, pte, updated_pte);
                     if (old_pte != pte) {
+                        rcu_read_unlock();
                         goto restart;
                     } else {
                         pte = updated_pte;
                     }
 #endif
                 } else {
+                    rcu_read_unlock();
                     /* misconfigured PTE in ROM (AD bits are not preset) or
                      * PTE is in IO space and can't be updated atomically */
                     return TRANSLATE_FAIL;
                 }
+                rcu_read_unlock();
             }
 
             /* for superpage mappings, make a fake leaf PTE for the TLB's