| Message ID | 20250121170626.1992570-3-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: throw debug exception before page fault | expand |
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~
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 > >
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);
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(+)