Re: [PATCH] riscv: Make semihosting configurable for all privilege modes

[Furquan Shaikh]

On Fri, Aug 12, 2022 at 4:42 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/12/22 16:27, Richard Henderson wrote:
> > On 8/11/22 13:41, Furquan Shaikh wrote:
> >> Unlike ARM, RISC-V does not define a separate breakpoint type for
> >> semihosting. Instead, it is entirely ABI. Thus, we need an option
> >> to allow users to configure what the ebreak behavior should be for
> >> different privilege levels - M, S, U, VS, VU. As per the RISC-V
> >> privilege specification[1], ebreak traps into the execution
> >> environment. However, RISC-V debug specification[2] provides
> >> ebreak{m,s,u,vs,vu} configuration bits to allow ebreak behavior to
> >> be configured to trap into debug mode instead. This change adds
> >> settable properties for RISC-V CPUs - `ebreakm`, `ebreaks`, `ebreaku`,
> >> `ebreakvs` and `ebreakvu` to allow user to configure whether qemu
> >> should treat ebreak as semihosting traps or trap according to the
> >> privilege specification.
> >>
> >> [1]
> >> https://github.com/riscv/riscv-isa-manual/releases/download/draft-20220723-10eea63/riscv-privileged.pdf
> >>
> >> [2] 
> >> https://github.com/riscv/riscv-debug-spec/blob/release/riscv-debug-release.pdf
> >
> > I don't see why you need to change anything at all.
> >
> > Semihosting doesn't only use 'ebreak', but a sequence of 3 insns:
> >
> >      slli x0, x0, 0x1f       # 0x01f01013    Entry NOP
> >      ebreak                  # 0x00100073    Break to debugger
> >      srai x0, x0, 7          # 0x40705013    NOP encoding the semihosting 
> > call number 7
> >
> > If the -semihosting command-line argument is absent, then the new DSCR 
> > fields apply as
> > normal.  If the -semihosting command-line argument is present, and the 
> > ebreak is not
> > surrounded by the required nops, then the new DSCR fields apply as normal.  
> > But if the
> > command-line argument is present and the nops are present, then semihosting 
> > overrides the
> > architecture and DSCR does not apply at all.
>
> I note that there's a missing test of semihosting_enabled() in
> target/riscv/insn_trans/trans_privileged.c.inc, and the PRV_S check in
> riscv_cpu_do_interrupt can be done at translation via ctx->mem_idx >= PRV_S.

I am not sure if I understood your comment correctly. Currently, qemu
has a check in `riscv_cpu_do_interrupt` to allow semihosting calls
only in S and M modes. This prevents semihosting calls from U mode.
This patch changes the default behavior of checking `>= PRV_S` for
semihosting and instead replaces it with the usage of
ebreak{m,s,u,vs,vu} properties to allow the user to configure what
modes should allow semihosting calls when `-semihosting` argument is
selected.

>
>
> r~