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

[Furquan Shaikh]

On Fri, Aug 12, 2022 at 4:04 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Aug 11, 2022 at 01:41:04PM -0700, 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
> >
> > Signed-off-by: Furquan Shaikh <furquan@rivosinc.com>
> > ---
> >  target/riscv/cpu.c        |  8 ++++++++
> >  target/riscv/cpu.h        |  7 +++++++
> >  target/riscv/cpu_helper.c | 26 +++++++++++++++++++++++++-
> >  3 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index ac6f82ebd0..082194652b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -953,6 +953,14 @@ static Property riscv_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("short-isa-string", RISCVCPU,
> > cfg.short_isa_string, false),
> >
> >      DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, cfg.rvv_ta_all_1s, false),
> > +
> > +    /* Debug spec */
> > +    DEFINE_PROP_BOOL("ebreakm", RISCVCPU, cfg.ebreakm, true),
> > +    DEFINE_PROP_BOOL("ebreaks", RISCVCPU, cfg.ebreaks, false),
> > +    DEFINE_PROP_BOOL("ebreaku", RISCVCPU, cfg.ebreaku, false),
> > +    DEFINE_PROP_BOOL("ebreakvs", RISCVCPU, cfg.ebreakvs, false),
> > +    DEFINE_PROP_BOOL("ebreakvu", RISCVCPU, cfg.ebreakvu, false),
> > +
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 5c7acc055a..eee8e487a6 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -454,6 +454,13 @@ struct RISCVCPUConfig {
> >      bool epmp;
> >      bool aia;
> >      bool debug;
> > +
> > +    /* Debug spec */
> > +    bool ebreakm;
> > +    bool ebreaks;
> > +    bool ebreaku;
> > +    bool ebreakvs;
> > +    bool ebreakvu;
>
> There's only five of these, so having each separate probably makes the
> most sense, but I wanted to point out that we could keep the properties
> independent booleans, as we should, but still consolidate the values
> into a single bitmap like we did for the sve vq bitmap for arm (see
> cpu_arm_get/set_vq). Maybe worth considering?

Thanks for the review and feedback, Andrew! I gave your suggestion a
try and updated the independent booleans to a single bitmap. It works,
but I am not sure if we really need all that additional code for this.
Like you mentioned, it is just five of these and having independent
booleans isn't too bad. If you or others feel strongly about switching
this to a bitmap, I can push a revised patchset. Else, I will keep the
change as is.

>
> >      uint64_t resetvec;
> >
> >      bool short_isa_string;
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 59b3680b1b..be09abbe27 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1314,6 +1314,30 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr
> > address, int size,
> >
> >      return true;
> >  }
> > +
> > +static bool semihosting_enabled(RISCVCPU *cpu)
> > +{
> > +    CPURISCVState *env = &cpu->env;
> > +
> > +    switch (env->priv) {
> > +    case PRV_M:
> > +        return cpu->cfg.ebreakm;
> > +    case PRV_S:
> > +        if (riscv_cpu_virt_enabled(env)) {
> > +            return cpu->cfg.ebreakvs;
> > +        } else {
> > +            return cpu->cfg.ebreaks;
> > +        }
> > +    case PRV_U:
> > +        if (riscv_cpu_virt_enabled(env)) {
> > +            return cpu->cfg.ebreakvu;
> > +        } else {
> > +            return cpu->cfg.ebreaku;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> >  #endif /* !CONFIG_USER_ONLY */
> >
> >  /*
> > @@ -1342,7 +1366,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >      target_ulong mtval2 = 0;
> >
> >      if  (cause == RISCV_EXCP_SEMIHOST) {
> > -        if (env->priv >= PRV_S) {
> > +        if (semihosting_enabled(cpu)) {
> >              do_common_semihosting(cs);
> >              env->pc += 4;
> >              return;
> > --
> > 2.34.1
> >
>
> Bitmap or no bitmap,
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>