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

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

[Furquan Shaikh]

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;
     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

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

[Furquan Shaikh]

Sorry for the top-posting. I noticed that the patch appears really
weird in patchwork [1] where part of the diff is above the PATCH
section. It looks fine in the archives[2] though.

[1] https://patchwork.ozlabs.org/project/qemu-devel/patch/CA+tJHD7FcrBTetGRO0vZn-XGPmZmQraMrw1dw9ia6jzHQniB0w@mail.gmail.com/
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01905.html

On Thu, Aug 11, 2022 at 1:41 PM Furquan Shaikh <furquan@rivosinc.com> 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;
>      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

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

On 11/8/22 22: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
> 
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Philippe Mathieu-Daudé]

On 11/8/22 22: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
> 
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Andrew Jones]

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?

>      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>

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>

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

[Palmer Dabbelt]

On Fri, 12 Aug 2022 15:05:08 PDT (-0700), furquan@rivosinc.com wrote:
> 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>

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>

Also no strong feelings on the bitmap, but I do like the feature.  When 
using an external JTAG debugger to talk to a this would probably be 
configured via something like GDB's monitor commands.  We could probably 
hook this up to QEMU's monitor as well, but doing it this way seems 
easier to implement and use.

We should document it, though.  Maybe just something like this?

diff --git a/qemu-options.hx b/qemu-options.hx
index 3f23a42fa8..f9444a1e4b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4635,7 +4635,9 @@ SRST
     open/read/write/seek/select. Tensilica baremetal libc for ISS and
     linux platform "sim" use this interface.

-    On RISC-V this implements the standard semihosting API, version 0.2.
+    On RISC-V this implements the standard semihosting API, version 0.2.  See
+    the ebreak{m,s,u,vs,vu} CPU properties to control which modes treat
+    breakpoints as semihosting calls.

     ``target=native|gdb|auto``
         Defines where the semihosting calls will be addressed, to QEMU

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

[Furquan Shaikh]

On Fri, Aug 12, 2022 at 4:00 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Fri, 12 Aug 2022 15:05:08 PDT (-0700), furquan@rivosinc.com wrote:
> > 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>
>
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> Also no strong feelings on the bitmap, but I do like the feature.  When
> using an external JTAG debugger to talk to a this would probably be
> configured via something like GDB's monitor commands.  We could probably
> hook this up to QEMU's monitor as well, but doing it this way seems
> easier to implement and use.
>
> We should document it, though.  Maybe just something like this?

Thanks, Palmer. I sent a v2 with the change you suggested.

>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3f23a42fa8..f9444a1e4b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4635,7 +4635,9 @@ SRST
>      open/read/write/seek/select. Tensilica baremetal libc for ISS and
>      linux platform "sim" use this interface.
>
> -    On RISC-V this implements the standard semihosting API, version 0.2.
> +    On RISC-V this implements the standard semihosting API, version 0.2.  See
> +    the ebreak{m,s,u,vs,vu} CPU properties to control which modes treat
> +    breakpoints as semihosting calls.
>
>      ``target=native|gdb|auto``
>          Defines where the semihosting calls will be addressed, to QEMU

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

[Peter Maydell]

On Thu, 11 Aug 2022 at 21:47, Furquan Shaikh <furquan@rivosinc.com> 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

As a general rule we don't allow userspace to make semihosting
calls, as a (rather weak) attempt at fencing off unprivileged
guest code from being able to scribble all over the host
filesystem. We should try to be consistent across architectures
about that, and in particular about how we enable it.

I have a half-finished patchset where I was planning to add
a --semihosting-config userspace-enable=on option or similar
to that effect.

It sounds like these ebreak bits are somewhat architectural,
so maybe they make sense as a riscv specific thing, but we
should consider how they ought to interact with the general
behaviour of semihosting. As it stands in QEMU today, we
(at least in theory) ought not to permit userspace to make
semihosting ebreak calls at all I think.

thanks
-- PMM

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

[Furquan Shaikh]

On Fri, Aug 12, 2022 at 4:28 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 11 Aug 2022 at 21:47, Furquan Shaikh <furquan@rivosinc.com> 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
>
> As a general rule we don't allow userspace to make semihosting
> calls, as a (rather weak) attempt at fencing off unprivileged
> guest code from being able to scribble all over the host
> filesystem. We should try to be consistent across architectures
> about that, and in particular about how we enable it.
>
> I have a half-finished patchset where I was planning to add
> a --semihosting-config userspace-enable=on option or similar
> to that effect.
>
> It sounds like these ebreak bits are somewhat architectural,
> so maybe they make sense as a riscv specific thing, but we
> should consider how they ought to interact with the general
> behaviour of semihosting. As it stands in QEMU today, we
> (at least in theory) ought not to permit userspace to make
> semihosting ebreak calls at all I think.

Thanks for the feedback, Peter. IIUC, the option that you are planning
to add "--semihosting-config userspace-enable=on" would allow
userspace to make semihosting calls on all architectures. The ebreak
bits here are risc-v specific and hence this change adds it as a
property for RISC-V CPUs. I agree with you that we should be
consistent about how we enable semihosting behavior within qemu.
Without seeing more details of how the config you plan to implement
works, it might be difficult to say how both these fit together. But,
just based on the above understanding, I am thinking we can use the
generic semihosting-config as the top-level config to allow
non-supervisor semihosting. Once your change is ready, we can apply
the additional restriction that these RISC-V ebreak bits for user mode
be settable only if the top-level config is enabled. There are other
ways of handling this too. I am new here and still learning, so open
to any ideas that might make this integration easier.

>
> thanks
> -- PMM

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

[Richard Henderson]

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.


r~

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

[Richard Henderson]

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.


r~

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~

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

[Richard Henderson]

On 8/12/22 16:57, Furquan Shaikh wrote:
> 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.

Why do you need such fine-grained control?  What is the use-case?

r~

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

[Furquan Shaikh]

On Fri, Aug 12, 2022 at 5:30 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/12/22 16:57, Furquan Shaikh wrote:
> > 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.
>
> Why do you need such fine-grained control?  What is the use-case?

I ran into a problem when I was testing a project (with a microkernel
in M-mode and tasks in U-mode) that uses semihosting for debugging.
The semihosting worked fine for M-mode but not in U-mode. As I started
digging into this, I realized that this is because qemu restricts
semihosting to only M and S modes. From reading the debug spec, I
understood that the DCSR presents options for ebreak behavior in each
mode including VS and VU. Adding CPU settable features to enable
ebreak behavior seemed like a flexible solution to support different
cases:
1. Old behavior of restricting semihosting to M and S modes by setting
only ebreakm and ebreaks.
2. My case of enabling semihosting to work in U-mode by setting only ebreaku.
3. Any future case where users would want to enable semihosting in
either M,S,U,VS or VU modes or any combination of these.

>
> r~

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

[Richard Henderson]

On 8/12/22 17:50, Furquan Shaikh wrote:
>> Why do you need such fine-grained control?  What is the use-case?
> 
> I ran into a problem when I was testing a project (with a microkernel
> in M-mode and tasks in U-mode) that uses semihosting for debugging.
> The semihosting worked fine for M-mode but not in U-mode.

Sure.  This would be handled by Peter's proposed userspace-enable=on property.

> As I started
> digging into this, I realized that this is because qemu restricts
> semihosting to only M and S modes. From reading the debug spec, I
> understood that the DCSR presents options for ebreak behavior in each
> mode including VS and VU.

I strongly suspect that VS also already works, since that's just

   env->priv == PRV_S && riscv_cpu_virt_enabled(env)

VU would also be handled by userspace-enable=on.

I do not see any use for 5 separate properties.


r~

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

[Furquan Shaikh]

On Fri, Aug 12, 2022 at 7:32 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/12/22 17:50, Furquan Shaikh wrote:
> >> Why do you need such fine-grained control?  What is the use-case?
> >
> > I ran into a problem when I was testing a project (with a microkernel
> > in M-mode and tasks in U-mode) that uses semihosting for debugging.
> > The semihosting worked fine for M-mode but not in U-mode.
>
> Sure.  This would be handled by Peter's proposed userspace-enable=on property.
>
> > As I started
> > digging into this, I realized that this is because qemu restricts
> > semihosting to only M and S modes. From reading the debug spec, I
> > understood that the DCSR presents options for ebreak behavior in each
> > mode including VS and VU.
>
> I strongly suspect that VS also already works, since that's just
>
>    env->priv == PRV_S && riscv_cpu_virt_enabled(env)
>
> VU would also be handled by userspace-enable=on.
>
> I do not see any use for 5 separate properties.

It felt more natural to mimic the knobs that are provided by the debug
spec to allow users to easily control the ebreak behavior for
semihosting in all possible modes. I agree that it is possible to just
allow semihosting to be turned on/off for U-mode using the above
proposed config. It would work for the use case I ran into, but if
finer control is required for other cases, it makes sense to provide
the ebreak options. Anyways, if others here feel that the
userspace-enable option is sufficient, we can go ahead with that.

>
>
> r~

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

[Peter Maydell]

On Sat, 13 Aug 2022 at 01:53, Furquan Shaikh <furquan@rivosinc.com> wrote:
> I ran into a problem when I was testing a project (with a microkernel
> in M-mode and tasks in U-mode) that uses semihosting for debugging.
> The semihosting worked fine for M-mode but not in U-mode. As I started
> digging into this, I realized that this is because qemu restricts
> semihosting to only M and S modes.

Right. We should fix this by having a generic works-for-all-architectures
semihosting config knob, because this is not a riscv specific problem,
and we already have issues with different target architectures
developing their own solutions to things.

I'll see if I can dig out the half-done patchset I had for this.

thanks
-- PMM

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

[Alistair Francis]

On Sat, Aug 13, 2022 at 8:20 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 13 Aug 2022 at 01:53, Furquan Shaikh <furquan@rivosinc.com> wrote:
> > I ran into a problem when I was testing a project (with a microkernel
> > in M-mode and tasks in U-mode) that uses semihosting for debugging.
> > The semihosting worked fine for M-mode but not in U-mode. As I started
> > digging into this, I realized that this is because qemu restricts
> > semihosting to only M and S modes.
>
> Right. We should fix this by having a generic works-for-all-architectures
> semihosting config knob, because this is not a riscv specific problem,
> and we already have issues with different target architectures
> developing their own solutions to things.

I agree with Peter here. I don't see a strong use case for a RISC-V
specific fine grained control. A user can either enable/disable
semihosting for privilege or usermodes (with Peter's patchset). That
seems like enough configuration options, it is unlikely that someone
will need semihosting only available to S-mode for example.

Alistair

>
> I'll see if I can dig out the half-done patchset I had for this.
>
> thanks
> -- PMM
>

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

[Furquan Shaikh]

On Sun, Aug 14, 2022 at 3:04 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Aug 13, 2022 at 8:20 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Sat, 13 Aug 2022 at 01:53, Furquan Shaikh <furquan@rivosinc.com> wrote:
> > > I ran into a problem when I was testing a project (with a microkernel
> > > in M-mode and tasks in U-mode) that uses semihosting for debugging.
> > > The semihosting worked fine for M-mode but not in U-mode. As I started
> > > digging into this, I realized that this is because qemu restricts
> > > semihosting to only M and S modes.
> >
> > Right. We should fix this by having a generic works-for-all-architectures
> > semihosting config knob, because this is not a riscv specific problem,
> > and we already have issues with different target architectures
> > developing their own solutions to things.
>
> I agree with Peter here. I don't see a strong use case for a RISC-V
> specific fine grained control. A user can either enable/disable
> semihosting for privilege or usermodes (with Peter's patchset). That
> seems like enough configuration options, it is unlikely that someone
> will need semihosting only available to S-mode for example.

Sounds good. Thanks for the feedback. I can continue with the changes
that Peter has.

>
> Alistair
>
> >
> > I'll see if I can dig out the half-done patchset I had for this.

Let me know if you want to collaborate on this to get this to
completion. Thanks!

> >
> > thanks
> > -- PMM
> >