| Message ID | CA+tJHD7FcrBTetGRO0vZn-XGPmZmQraMrw1dw9ia6jzHQniB0w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Make semihosting configurable for all privilege modes | expand |
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
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>
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>
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
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>
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
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
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~
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
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~
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~
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~
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~
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~
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~
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
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 >
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 > >
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 */
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(-) /* @@ -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