[PATCH] riscv: don't look at SUM when accessing memory from a debugger context

Jade Fink posted 1 patch 3 years ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210406113109.1031033-1-qemu@jade.fyi
Maintainers: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>
target/riscv/cpu_helper.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
[PATCH] riscv: don't look at SUM when accessing memory from a debugger context
Posted by Jade Fink 3 years ago
Previously the qemu monitor and gdbstub looked at SUM and refused to
perform accesses to user memory if it is off, which was an impediment to
debugging.

Signed-off-by: Jade Fink <qemu@jade.fyi>
---
 target/riscv/cpu_helper.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 83a6bcfad0..18ea2cba57 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -299,12 +299,14 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
  * @first_stage: Are we in first stage translation?
  *               Second stage is used for hypervisor guest translation
  * @two_stage: Are we going to perform two stage translation
+ * @is_debug: Is this access from a debugger or the monitor?
  */
 static int get_physical_address(CPURISCVState *env, hwaddr *physical,
                                 int *prot, target_ulong addr,
                                 target_ulong *fault_pte_addr,
                                 int access_type, int mmu_idx,
-                                bool first_stage, bool two_stage)
+                                bool first_stage, bool two_stage,
+                                bool is_debug)
 {
     /* NOTE: the env->pc value visible here will not be
      * correct, but the value visible to the exception handler
@@ -369,7 +371,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
         widened = 2;
     }
     /* status.SUM will be ignored if execute on background */
-    sum = get_field(env->mstatus, MSTATUS_SUM) || use_background;
+    sum = get_field(env->mstatus, MSTATUS_SUM) || use_background || is_debug;
     switch (vm) {
     case VM_1_10_SV32:
       levels = 2; ptidxbits = 10; ptesize = 4; break;
@@ -428,7 +430,8 @@ restart:
             /* Do the second stage translation on the base PTE address. */
             int vbase_ret = get_physical_address(env, &vbase, &vbase_prot,
                                                  base, NULL, MMU_DATA_LOAD,
-                                                 mmu_idx, false, true);
+                                                 mmu_idx, false, true,
+                                                 is_debug);
 
             if (vbase_ret != TRANSLATE_SUCCESS) {
                 if (fault_pte_addr) {
@@ -616,13 +619,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
     int mmu_idx = cpu_mmu_index(&cpu->env, false);
 
     if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx,
-                             true, riscv_cpu_virt_enabled(env))) {
+                             true, riscv_cpu_virt_enabled(env), true)) {
         return -1;
     }
 
     if (riscv_cpu_virt_enabled(env)) {
         if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL,
-                                 0, mmu_idx, false, true)) {
+                                 0, mmu_idx, false, true, true)) {
             return -1;
         }
     }
@@ -714,7 +717,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
         /* Two stage lookup */
         ret = get_physical_address(env, &pa, &prot, address,
                                    &env->guest_phys_fault_addr, access_type,
-                                   mmu_idx, true, true);
+                                   mmu_idx, true, true, false);
 
         /*
          * A G-stage exception may be triggered during two state lookup.
@@ -736,7 +739,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
             im_address = pa;
 
             ret = get_physical_address(env, &pa, &prot2, im_address, NULL,
-                                       access_type, mmu_idx, false, true);
+                                       access_type, mmu_idx, false, true,
+                                       false);
 
             qemu_log_mask(CPU_LOG_MMU,
                     "%s 2nd-stage address=%" VADDR_PRIx " ret %d physical "
@@ -765,7 +769,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     } else {
         /* Single stage lookup */
         ret = get_physical_address(env, &pa, &prot, address, NULL,
-                                   access_type, mmu_idx, true, false);
+                                   access_type, mmu_idx, true, false, false);
 
         qemu_log_mask(CPU_LOG_MMU,
                       "%s address=%" VADDR_PRIx " ret %d physical "
-- 
2.31.1


Re: [PATCH] riscv: don't look at SUM when accessing memory from a debugger context
Posted by Alistair Francis 3 years ago
On Tue, Apr 6, 2021 at 9:10 AM Jade Fink <qemu@jade.fyi> wrote:
>
> Previously the qemu monitor and gdbstub looked at SUM and refused to
> perform accesses to user memory if it is off, which was an impediment to
> debugging.
>
> Signed-off-by: Jade Fink <qemu@jade.fyi>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 83a6bcfad0..18ea2cba57 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -299,12 +299,14 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>   * @first_stage: Are we in first stage translation?
>   *               Second stage is used for hypervisor guest translation
>   * @two_stage: Are we going to perform two stage translation
> + * @is_debug: Is this access from a debugger or the monitor?
>   */
>  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>                                  int *prot, target_ulong addr,
>                                  target_ulong *fault_pte_addr,
>                                  int access_type, int mmu_idx,
> -                                bool first_stage, bool two_stage)
> +                                bool first_stage, bool two_stage,
> +                                bool is_debug)
>  {
>      /* NOTE: the env->pc value visible here will not be
>       * correct, but the value visible to the exception handler
> @@ -369,7 +371,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>          widened = 2;
>      }
>      /* status.SUM will be ignored if execute on background */
> -    sum = get_field(env->mstatus, MSTATUS_SUM) || use_background;
> +    sum = get_field(env->mstatus, MSTATUS_SUM) || use_background || is_debug;
>      switch (vm) {
>      case VM_1_10_SV32:
>        levels = 2; ptidxbits = 10; ptesize = 4; break;
> @@ -428,7 +430,8 @@ restart:
>              /* Do the second stage translation on the base PTE address. */
>              int vbase_ret = get_physical_address(env, &vbase, &vbase_prot,
>                                                   base, NULL, MMU_DATA_LOAD,
> -                                                 mmu_idx, false, true);
> +                                                 mmu_idx, false, true,
> +                                                 is_debug);
>
>              if (vbase_ret != TRANSLATE_SUCCESS) {
>                  if (fault_pte_addr) {
> @@ -616,13 +619,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>      int mmu_idx = cpu_mmu_index(&cpu->env, false);
>
>      if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx,
> -                             true, riscv_cpu_virt_enabled(env))) {
> +                             true, riscv_cpu_virt_enabled(env), true)) {
>          return -1;
>      }
>
>      if (riscv_cpu_virt_enabled(env)) {
>          if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL,
> -                                 0, mmu_idx, false, true)) {
> +                                 0, mmu_idx, false, true, true)) {
>              return -1;
>          }
>      }
> @@ -714,7 +717,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>          /* Two stage lookup */
>          ret = get_physical_address(env, &pa, &prot, address,
>                                     &env->guest_phys_fault_addr, access_type,
> -                                   mmu_idx, true, true);
> +                                   mmu_idx, true, true, false);
>
>          /*
>           * A G-stage exception may be triggered during two state lookup.
> @@ -736,7 +739,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>              im_address = pa;
>
>              ret = get_physical_address(env, &pa, &prot2, im_address, NULL,
> -                                       access_type, mmu_idx, false, true);
> +                                       access_type, mmu_idx, false, true,
> +                                       false);
>
>              qemu_log_mask(CPU_LOG_MMU,
>                      "%s 2nd-stage address=%" VADDR_PRIx " ret %d physical "
> @@ -765,7 +769,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      } else {
>          /* Single stage lookup */
>          ret = get_physical_address(env, &pa, &prot, address, NULL,
> -                                   access_type, mmu_idx, true, false);
> +                                   access_type, mmu_idx, true, false, false);
>
>          qemu_log_mask(CPU_LOG_MMU,
>                        "%s address=%" VADDR_PRIx " ret %d physical "
> --
> 2.31.1
>
>

Re: [PATCH] riscv: don't look at SUM when accessing memory from a debugger context
Posted by Alistair Francis 3 years ago
On Tue, Apr 6, 2021 at 9:10 AM Jade Fink <qemu@jade.fyi> wrote:
>
> Previously the qemu monitor and gdbstub looked at SUM and refused to
> perform accesses to user memory if it is off, which was an impediment to
> debugging.
>
> Signed-off-by: Jade Fink <qemu@jade.fyi>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/cpu_helper.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 83a6bcfad0..18ea2cba57 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -299,12 +299,14 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>   * @first_stage: Are we in first stage translation?
>   *               Second stage is used for hypervisor guest translation
>   * @two_stage: Are we going to perform two stage translation
> + * @is_debug: Is this access from a debugger or the monitor?
>   */
>  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>                                  int *prot, target_ulong addr,
>                                  target_ulong *fault_pte_addr,
>                                  int access_type, int mmu_idx,
> -                                bool first_stage, bool two_stage)
> +                                bool first_stage, bool two_stage,
> +                                bool is_debug)
>  {
>      /* NOTE: the env->pc value visible here will not be
>       * correct, but the value visible to the exception handler
> @@ -369,7 +371,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>          widened = 2;
>      }
>      /* status.SUM will be ignored if execute on background */
> -    sum = get_field(env->mstatus, MSTATUS_SUM) || use_background;
> +    sum = get_field(env->mstatus, MSTATUS_SUM) || use_background || is_debug;
>      switch (vm) {
>      case VM_1_10_SV32:
>        levels = 2; ptidxbits = 10; ptesize = 4; break;
> @@ -428,7 +430,8 @@ restart:
>              /* Do the second stage translation on the base PTE address. */
>              int vbase_ret = get_physical_address(env, &vbase, &vbase_prot,
>                                                   base, NULL, MMU_DATA_LOAD,
> -                                                 mmu_idx, false, true);
> +                                                 mmu_idx, false, true,
> +                                                 is_debug);
>
>              if (vbase_ret != TRANSLATE_SUCCESS) {
>                  if (fault_pte_addr) {
> @@ -616,13 +619,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>      int mmu_idx = cpu_mmu_index(&cpu->env, false);
>
>      if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx,
> -                             true, riscv_cpu_virt_enabled(env))) {
> +                             true, riscv_cpu_virt_enabled(env), true)) {
>          return -1;
>      }
>
>      if (riscv_cpu_virt_enabled(env)) {
>          if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL,
> -                                 0, mmu_idx, false, true)) {
> +                                 0, mmu_idx, false, true, true)) {
>              return -1;
>          }
>      }
> @@ -714,7 +717,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>          /* Two stage lookup */
>          ret = get_physical_address(env, &pa, &prot, address,
>                                     &env->guest_phys_fault_addr, access_type,
> -                                   mmu_idx, true, true);
> +                                   mmu_idx, true, true, false);
>
>          /*
>           * A G-stage exception may be triggered during two state lookup.
> @@ -736,7 +739,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>              im_address = pa;
>
>              ret = get_physical_address(env, &pa, &prot2, im_address, NULL,
> -                                       access_type, mmu_idx, false, true);
> +                                       access_type, mmu_idx, false, true,
> +                                       false);
>
>              qemu_log_mask(CPU_LOG_MMU,
>                      "%s 2nd-stage address=%" VADDR_PRIx " ret %d physical "
> @@ -765,7 +769,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      } else {
>          /* Single stage lookup */
>          ret = get_physical_address(env, &pa, &prot, address, NULL,
> -                                   access_type, mmu_idx, true, false);
> +                                   access_type, mmu_idx, true, false, false);
>
>          qemu_log_mask(CPU_LOG_MMU,
>                        "%s address=%" VADDR_PRIx " ret %d physical "
> --
> 2.31.1
>
>