Re: [PATCH 7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on
On Fri, 19 Aug 2022 at 01:55, Alistair Francis wrote: > > On Thu, Aug 18, 2022 at 11:58 PM Peter Maydell > 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
Re: [PATCH 7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on
On Thu, Aug 18, 2022 at 11:58 PM Peter Maydell wrote: > > On Thu, 18 Aug 2022 at 05:20, Alistair Francis wrote: > > > > On Tue, Aug 16, 2022 at 5:11 AM 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 > > > > 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
Re: [PATCH 7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on
On Thu, 18 Aug 2022 at 05:20, Alistair Francis wrote: > > On Tue, Aug 16, 2022 at 5:11 AM 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 > > 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
Re: [PATCH 7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on
On Tue, Aug 16, 2022 at 5:11 AM 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
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
> ---
> 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
>
>
Re: [PATCH 7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on
On Mon, Aug 15, 2022 at 1:26 PM Richard Henderson
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
> > ---
> > 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~
>
Re: [PATCH 7/7] target/riscv: Honour -semihosting-config userspace=on and enable=on
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
---
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~
