| Message ID | 20220815190303.2061559-8-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Allow semihosting from user mode | expand |
On 8/15/22 14:03, Peter Maydell wrote: > The riscv target incorrectly enabled semihosting always, whether the > user asked for it or not. Call semihosting_enabled() passing the > correct value to the is_userspace argument, which fixes this and also > handles the userspace=on argument. > > Note that this is a behaviour change: we used to default to > semihosting being enabled, and now the user must pass > "-semihosting-config enable=on" if they want it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/riscv/cpu_helper.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 59b3680b1b2..49c4ea98ac9 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -24,6 +24,7 @@ > #include "exec/exec-all.h" > #include "tcg/tcg-op.h" > #include "trace.h" > +#include "semihosting/semihost.h" > #include "semihosting/common-semi.h" > > int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > @@ -1342,7 +1343,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(env->priv < PRV_S)) { > do_common_semihosting(cs); > env->pc += 4; > return; I think this should be done in translate. We should not have the overhead of checking the three insn sequence around ebreak unless semihosting is enabled. Note that ctx->mem_idx == env->priv, per cpu_mem_index(). r~
On Mon, Aug 15, 2022 at 1:26 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/15/22 14:03, Peter Maydell wrote: > > The riscv target incorrectly enabled semihosting always, whether the > > user asked for it or not. Call semihosting_enabled() passing the > > correct value to the is_userspace argument, which fixes this and also > > handles the userspace=on argument. > > > > Note that this is a behaviour change: we used to default to > > semihosting being enabled, and now the user must pass > > "-semihosting-config enable=on" if they want it. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > target/riscv/cpu_helper.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > index 59b3680b1b2..49c4ea98ac9 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -24,6 +24,7 @@ > > #include "exec/exec-all.h" > > #include "tcg/tcg-op.h" > > #include "trace.h" > > +#include "semihosting/semihost.h" > > #include "semihosting/common-semi.h" > > > > int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > @@ -1342,7 +1343,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(env->priv < PRV_S)) { > > do_common_semihosting(cs); > > env->pc += 4; > > return; > > I think this should be done in translate. We should not have the overhead of checking the > three insn sequence around ebreak unless semihosting is enabled. Note that ctx->mem_idx > == env->priv, per cpu_mem_index(). FWIW, the current series worked fine for my risc-v use case. Thanks, Peter! > > > r~ >
On Tue, Aug 16, 2022 at 5:11 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > The riscv target incorrectly enabled semihosting always, whether the > user asked for it or not. Call semihosting_enabled() passing the > correct value to the is_userspace argument, which fixes this and also > handles the userspace=on argument. > > Note that this is a behaviour change: we used to default to > semihosting being enabled, and now the user must pass > "-semihosting-config enable=on" if they want it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> I agree with Richard that a check in translate would be better, but this is also an improvement on the broken implementation we have now Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu_helper.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 59b3680b1b2..49c4ea98ac9 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -24,6 +24,7 @@ > #include "exec/exec-all.h" > #include "tcg/tcg-op.h" > #include "trace.h" > +#include "semihosting/semihost.h" > #include "semihosting/common-semi.h" > > int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > @@ -1342,7 +1343,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(env->priv < PRV_S)) { > do_common_semihosting(cs); > env->pc += 4; > return; > -- > 2.25.1 > >
On Thu, 18 Aug 2022 at 05:20, Alistair Francis <alistair23@gmail.com> wrote: > > On Tue, Aug 16, 2022 at 5:11 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > The riscv target incorrectly enabled semihosting always, whether the > > user asked for it or not. Call semihosting_enabled() passing the > > correct value to the is_userspace argument, which fixes this and also > > handles the userspace=on argument. > > > > Note that this is a behaviour change: we used to default to > > semihosting being enabled, and now the user must pass > > "-semihosting-config enable=on" if they want it. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > I agree with Richard that a check in translate would be better, but > this is also an improvement on the broken implementation we have now Do you have an opinion on whether there are likely to be many users who are using riscv semihosting without explicitly enabling it on the command line ? -- PMM
On Thu, Aug 18, 2022 at 11:58 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 18 Aug 2022 at 05:20, Alistair Francis <alistair23@gmail.com> wrote: > > > > On Tue, Aug 16, 2022 at 5:11 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > The riscv target incorrectly enabled semihosting always, whether the > > > user asked for it or not. Call semihosting_enabled() passing the > > > correct value to the is_userspace argument, which fixes this and also > > > handles the userspace=on argument. > > > > > > Note that this is a behaviour change: we used to default to > > > semihosting being enabled, and now the user must pass > > > "-semihosting-config enable=on" if they want it. > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > I agree with Richard that a check in translate would be better, but > > this is also an improvement on the broken implementation we have now > > Do you have an opinion on whether there are likely to be many > users who are using riscv semihosting without explicitly enabling it > on the command line ? I don't think there are many users. We have always stated that semihosting had to be enabled via the command line Alistair > > -- PMM
On Fri, 19 Aug 2022 at 01:55, Alistair Francis <alistair23@gmail.com> wrote: > > On Thu, Aug 18, 2022 at 11:58 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > Do you have an opinion on whether there are likely to be many > > users who are using riscv semihosting without explicitly enabling it > > on the command line ? > > I don't think there are many users. We have always stated that > semihosting had to be enabled via the command line Cool -- I'll do a v2 of this with the change RTH wants, and we won't go through the deprecate-and-warn process for fixing the "didn't pay attention to command line option" bug. -- PMM
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 59b3680b1b2..49c4ea98ac9 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -24,6 +24,7 @@ #include "exec/exec-all.h" #include "tcg/tcg-op.h" #include "trace.h" +#include "semihosting/semihost.h" #include "semihosting/common-semi.h" int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) @@ -1342,7 +1343,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(env->priv < PRV_S)) { do_common_semihosting(cs); env->pc += 4; return;
The riscv target incorrectly enabled semihosting always, whether the user asked for it or not. Call semihosting_enabled() passing the correct value to the is_userspace argument, which fixes this and also handles the userspace=on argument. Note that this is a behaviour change: we used to default to semihosting being enabled, and now the user must pass "-semihosting-config enable=on" if they want it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/riscv/cpu_helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)