diff mbox series

[7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on

Message ID 20220815190303.2061559-8-peter.maydell@linaro.org
State Accepted
Headers show
Series Allow semihosting from user mode | expand

Commit Message

Peter Maydell Aug. 15, 2022, 7:03 p.m. UTC
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(-)

Comments

Richard Henderson Aug. 15, 2022, 8:26 p.m. UTC | #1
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~
Furquan Shaikh Aug. 16, 2022, 5:39 a.m. UTC | #2
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~
>
Alistair Francis Aug. 18, 2022, 4:19 a.m. UTC | #3
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
>
>
Peter Maydell Aug. 18, 2022, 1:57 p.m. UTC | #4
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
Alistair Francis Aug. 19, 2022, 12:55 a.m. UTC | #5
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
Peter Maydell Aug. 19, 2022, 9:09 a.m. UTC | #6
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 mbox series

Patch

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;