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

[Daniel Henrique Barboza]

On 1/21/25 12:47 PM, Richard Henderson wrote:
> On 1/20/25 12:49, 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 | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index e1dfc4ecbf..ae0a659ec7 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,24 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> > size,
> >       } else if (probe) {
> >           return false;
> >       } else {
> > +        int wp_len = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
>
> Surely 'size' may be relevant?

Ooops. Yeah, no need to infer wp_len if we can use the access size. I'll do a 
v3.


Thanks,

Daniel

> r~
>
> > +        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, wp_len, MEMTXATTRS_UNSPECIFIED,
> > +                             wp_access, retaddr);
> > +
> >           raise_mmu_exception(env, address, access_type, pmp_violation,
> >                               first_stage_error, two_stage_lookup,
> >                               two_stage_indirect_error);