[PATCH v3 0/2] target/riscv: throw debug exception before page fault

[PATCH v3 0/2] target/riscv: throw debug exception before page fault

[Daniel Henrique Barboza]

Hi,

In this new version, in patch 2,  we're using the address 'size' val from
riscv_cpu_tlb_fill() instead of infering it from the CPU XLEN.

No other changes made. Patches based on master.

Changes from v2:
- patch 2:
  - use 'size' instead of infering wp_len using the CPU XLEN
- v2 link: https://lore.kernel.org/qemu-riscv/20250120204910.1317013-1-dbarboza@ventanamicro.com/

Daniel Henrique Barboza (2):
  target/riscv/debug.c: use wp size = 4 for 32-bit CPUs
  target/riscv: throw debug exception before page fault

 target/riscv/cpu_helper.c | 18 ++++++++++++++++++
 target/riscv/debug.c      |  6 ++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.47.1

[PATCH v3 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Daniel Henrique Barboza]

The mcontrol select bit (19) is always zero, meaning our triggers will
always match virtual addresses. In this condition, if the user does not
specify a size for the trigger, the access size defaults to XLEN.

At this moment we're using def_size = 8 regardless of CPU XLEN. Use
def_size = 4 in case we're running 32 bits.

Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig extension")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/debug.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index f6241a80be..9db4048523 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -478,7 +478,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
     bool enabled = type2_breakpoint_enabled(ctrl);
     CPUState *cs = env_cpu(env);
     int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
-    uint32_t size;
+    uint32_t size, def_size;
 
     if (!enabled) {
         return;
@@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
             cpu_watchpoint_insert(cs, addr, size, flags,
                                   &env->cpu_watchpoint[index]);
         } else {
-            cpu_watchpoint_insert(cs, addr, 8, flags,
+            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
+
+            cpu_watchpoint_insert(cs, addr, def_size, flags,
                                   &env->cpu_watchpoint[index]);
         }
     }
-- 
2.47.1

Re: [PATCH v3 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Philippe Mathieu-Daudé]

On 21/1/25 18:06, Daniel Henrique Barboza wrote:
> The mcontrol select bit (19) is always zero, meaning our triggers will
> always match virtual addresses. In this condition, if the user does not
> specify a size for the trigger, the access size defaults to XLEN.
> 
> At this moment we're using def_size = 8 regardless of CPU XLEN. Use
> def_size = 4 in case we're running 32 bits.
> 
> Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig extension")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/debug.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index f6241a80be..9db4048523 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -478,7 +478,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>       bool enabled = type2_breakpoint_enabled(ctrl);
>       CPUState *cs = env_cpu(env);
>       int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
> -    uint32_t size;
> +    uint32_t size, def_size;
>   
>       if (!enabled) {
>           return;
> @@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>               cpu_watchpoint_insert(cs, addr, size, flags,
>                                     &env->cpu_watchpoint[index]);
>           } else {
> -            cpu_watchpoint_insert(cs, addr, 8, flags,
> +            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Alistair Francis]

On Wed, Jan 22, 2025 at 3:07 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The mcontrol select bit (19) is always zero, meaning our triggers will
> always match virtual addresses. In this condition, if the user does not
> specify a size for the trigger, the access size defaults to XLEN.
>
> At this moment we're using def_size = 8 regardless of CPU XLEN. Use
> def_size = 4 in case we're running 32 bits.
>
> Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig extension")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/debug.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index f6241a80be..9db4048523 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -478,7 +478,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>      bool enabled = type2_breakpoint_enabled(ctrl);
>      CPUState *cs = env_cpu(env);
>      int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
> -    uint32_t size;
> +    uint32_t size, def_size;
>
>      if (!enabled) {
>          return;
> @@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>              cpu_watchpoint_insert(cs, addr, size, flags,
>                                    &env->cpu_watchpoint[index]);
>          } else {
> -            cpu_watchpoint_insert(cs, addr, 8, flags,
> +            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
> +
> +            cpu_watchpoint_insert(cs, addr, def_size, flags,
>                                    &env->cpu_watchpoint[index]);
>          }
>      }
> --
> 2.47.1
>
>

[PATCH v3 2/2] target/riscv: throw debug exception before page fault

[Daniel Henrique Barboza]

In the RISC-V privileged ISA section 3.1.15 table 15, it is determined
that a debug exception that is triggered from a load/store has a higher
priority than a possible fault that this access might trigger.

This is not the case ATM as shown in [1]. Adding a breakpoint in an
address that deliberately will fault is causing a load page fault
instead of a debug exception. The reason is that we're throwing in the
page fault as soon as the fault occurs (end of riscv_cpu_tlb_fill(),
raise_mmu_exception()), not allowing the installed watchpoints to
trigger.

Call cpu_check_watchpoint() in the page fault path to search and execute
any watchpoints that might exist for the address, never returning back
to the fault path. If no watchpoints are found cpu_check_watchpoint()
will return and we'll fall-through the regular path to
raise_mmu_exception().

[1] https://gitlab.com/qemu-project/qemu/-/issues/2627

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2627
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu_helper.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e1dfc4ecbf..df5de53379 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -27,6 +27,7 @@
 #include "exec/page-protection.h"
 #include "instmap.h"
 #include "tcg/tcg-op.h"
+#include "hw/core/tcg-cpu-ops.h"
 #include "trace.h"
 #include "semihosting/common-semi.h"
 #include "system/cpu-timers.h"
@@ -1708,6 +1709,23 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     } else if (probe) {
         return false;
     } else {
+        int wp_access = 0;
+
+        if (access_type == MMU_DATA_LOAD) {
+            wp_access |= BP_MEM_READ;
+        } else if (access_type == MMU_DATA_STORE) {
+            wp_access |= BP_MEM_WRITE;
+        }
+
+        /*
+         * If a watchpoint isn't found for 'addr' this will
+         * be a no-op and we'll resume the mmu_exception path.
+         * Otherwise we'll throw a debug exception and execution
+         * will continue elsewhere.
+         */
+        cpu_check_watchpoint(cs, address, size, MEMTXATTRS_UNSPECIFIED,
+                             wp_access, retaddr);
+
         raise_mmu_exception(env, address, access_type, pmp_violation,
                             first_stage_error, two_stage_lookup,
                             two_stage_indirect_error);
-- 
2.47.1

Re: [PATCH v3 2/2] target/riscv: throw debug exception before page fault

[Richard Henderson]

On 1/21/25 09:06, Daniel Henrique Barboza wrote:
> In the RISC-V privileged ISA section 3.1.15 table 15, it is determined
> that a debug exception that is triggered from a load/store has a higher
> priority than a possible fault that this access might trigger.
> 
> This is not the case ATM as shown in [1]. Adding a breakpoint in an
> address that deliberately will fault is causing a load page fault
> instead of a debug exception. The reason is that we're throwing in the
> page fault as soon as the fault occurs (end of riscv_cpu_tlb_fill(),
> raise_mmu_exception()), not allowing the installed watchpoints to
> trigger.
> 
> Call cpu_check_watchpoint() in the page fault path to search and execute
> any watchpoints that might exist for the address, never returning back
> to the fault path. If no watchpoints are found cpu_check_watchpoint()
> will return and we'll fall-through the regular path to
> raise_mmu_exception().
> 
> [1]https://gitlab.com/qemu-project/qemu/-/issues/2627
> 
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2627
> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu_helper.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 2/2] target/riscv: throw debug exception before page fault

[Alistair Francis]

On Wed, Jan 22, 2025 at 3:08 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> In the RISC-V privileged ISA section 3.1.15 table 15, it is determined
> that a debug exception that is triggered from a load/store has a higher
> priority than a possible fault that this access might trigger.
>
> This is not the case ATM as shown in [1]. Adding a breakpoint in an
> address that deliberately will fault is causing a load page fault
> instead of a debug exception. The reason is that we're throwing in the
> page fault as soon as the fault occurs (end of riscv_cpu_tlb_fill(),
> raise_mmu_exception()), not allowing the installed watchpoints to
> trigger.
>
> Call cpu_check_watchpoint() in the page fault path to search and execute
> any watchpoints that might exist for the address, never returning back
> to the fault path. If no watchpoints are found cpu_check_watchpoint()
> will return and we'll fall-through the regular path to
> raise_mmu_exception().
>
> [1] https://gitlab.com/qemu-project/qemu/-/issues/2627
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2627
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e1dfc4ecbf..df5de53379 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -27,6 +27,7 @@
>  #include "exec/page-protection.h"
>  #include "instmap.h"
>  #include "tcg/tcg-op.h"
> +#include "hw/core/tcg-cpu-ops.h"
>  #include "trace.h"
>  #include "semihosting/common-semi.h"
>  #include "system/cpu-timers.h"
> @@ -1708,6 +1709,23 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      } else if (probe) {
>          return false;
>      } else {
> +        int wp_access = 0;
> +
> +        if (access_type == MMU_DATA_LOAD) {
> +            wp_access |= BP_MEM_READ;
> +        } else if (access_type == MMU_DATA_STORE) {
> +            wp_access |= BP_MEM_WRITE;
> +        }
> +
> +        /*
> +         * If a watchpoint isn't found for 'addr' this will
> +         * be a no-op and we'll resume the mmu_exception path.
> +         * Otherwise we'll throw a debug exception and execution
> +         * will continue elsewhere.
> +         */
> +        cpu_check_watchpoint(cs, address, size, MEMTXATTRS_UNSPECIFIED,
> +                             wp_access, retaddr);
> +
>          raise_mmu_exception(env, address, access_type, pmp_violation,
>                              first_stage_error, two_stage_lookup,
>                              two_stage_indirect_error);
> --
> 2.47.1
>
>

Re: [PATCH v3 0/2] target/riscv: throw debug exception before page fault

[Alistair Francis]

On Wed, Jan 22, 2025 at 3:07 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> In this new version, in patch 2,  we're using the address 'size' val from
> riscv_cpu_tlb_fill() instead of infering it from the CPU XLEN.
>
> No other changes made. Patches based on master.
>
> Changes from v2:
> - patch 2:
>   - use 'size' instead of infering wp_len using the CPU XLEN
> - v2 link: https://lore.kernel.org/qemu-riscv/20250120204910.1317013-1-dbarboza@ventanamicro.com/
>
> Daniel Henrique Barboza (2):
>   target/riscv/debug.c: use wp size = 4 for 32-bit CPUs
>   target/riscv: throw debug exception before page fault

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu_helper.c | 18 ++++++++++++++++++
>  target/riscv/debug.c      |  6 ++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> --
> 2.47.1
>
>

Re: [PATCH v3 0/2] target/riscv: throw debug exception before page fault

[Michael Tokarev]

21.01.2025 20:06, Daniel Henrique Barboza wrote:
> Hi,
> 
> In this new version, in patch 2,  we're using the address 'size' val from
> riscv_cpu_tlb_fill() instead of infering it from the CPU XLEN.
> 
> No other changes made. Patches based on master.
> 
> Changes from v2:
> - patch 2:
>    - use 'size' instead of infering wp_len using the CPU XLEN
> - v2 link: https://lore.kernel.org/qemu-riscv/20250120204910.1317013-1-dbarboza@ventanamicro.com/
> 
> Daniel Henrique Barboza (2):
>    target/riscv/debug.c: use wp size = 4 for 32-bit CPUs
>    target/riscv: throw debug exception before page fault

Hi!

Is this a qemu-stable material?

If yes, is it worth to pick it up for older stable series
(currently active series are 7.2 and 8.2)?

Please keep Cc: qemu-stable@ for fixes which should be picked up
for the stable series.

Thanks,

/mjt

Re: [PATCH v3 0/2] target/riscv: throw debug exception before page fault

[Alistair Francis]

On Thu, Mar 6, 2025 at 4:33 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 21.01.2025 20:06, Daniel Henrique Barboza wrote:
> > Hi,
> >
> > In this new version, in patch 2,  we're using the address 'size' val from
> > riscv_cpu_tlb_fill() instead of infering it from the CPU XLEN.
> >
> > No other changes made. Patches based on master.
> >
> > Changes from v2:
> > - patch 2:
> >    - use 'size' instead of infering wp_len using the CPU XLEN
> > - v2 link: https://lore.kernel.org/qemu-riscv/20250120204910.1317013-1-dbarboza@ventanamicro.com/
> >
> > Daniel Henrique Barboza (2):
> >    target/riscv/debug.c: use wp size = 4 for 32-bit CPUs
> >    target/riscv: throw debug exception before page fault
>
> Hi!
>
> Is this a qemu-stable material?

Yes

>
> If yes, is it worth to pick it up for older stable series
> (currently active series are 7.2 and 8.2)?

If it applies then it probably is

>
> Please keep Cc: qemu-stable@ for fixes which should be picked up
> for the stable series.

Sorry, I always forget :(

Alistair

>
> Thanks,
>
> /mjt
>