[PATCH 0/7] Allow semihosting from user mode

[PATCH 0/7] Allow semihosting from user mode

[Peter Maydell]

Currently our semihosting implementations usually prohibit use of
semihosting calls in system emulation from the guest userspace.  This
is a very long standing behaviour justified originally "to provide
some semblance of security" (since code with access to the semihosting
ABI can do things like read and write arbitrary files on the host
system).  However, it is sometimes useful to be able to run trusted
guest code which performs semihosting calls from guest userspace,
notably for test code.

This patchset adds a command line suboption to the existing
semihosting-config option group so that you can explicitly opt in to
semihosting from guest userspace with "-semihosting-config
userspace=on".

It also brings all our target architectures into line about
how they handle semihosting. Currently these fall into three
different groups:

 * semihosting permitted only in privileged mode and only
   if enabled on the command line:
   - arm
   - m68k
 * semihosting permitted in any mode, if enabled on the command line:
   - mips
   - nios2
   - xtensa
 * semihosting permitted only in privileged mode, but fails
   to honour the existing "enable semihosting" option, instead
   enabling it all the time:
   - riscv

The effect of the new option for group 1 is:
 * user can now optionally also allow semihosting in usermode

For group 2 it is:
 * usermode semihosting used to be permitted, but now changes
   to default-disabled, needing explicit enablement

For group 3 it is:
 * semihosting overall used to be default-enabled and is
   now default-disabled, needing explicit enablement.
   Semihosting in usermode can also be enabled.

That means this is a "things that used to work no longer do
unless you change your commandline" change for groups 2 and 3
(so, mips, nios2. xtensa, riscv). In this patchset I've opted
to just make the change (with the intention of releasenoting
it) but I'm open to arguments that we ought to put it through
the deprecate-and-delete cycle. (I suspect this probably most
affects riscv.)

The patchset structure adds the option first and then updates
each target architecture in turn to honour it. It didn't seem
to me worth the extra patch-splitting to put the underlying
infrastructure in first, then the target changes and finally
exposing the option to the user only once it's honoured
everywhere.

NB: I haven't really tested this much, just 'make check'
and 'make check-avocado'; I wanted to get it out to the
mailing list for discussion, anyway.

thanks
-- PMM

Peter Maydell (7):
  semihosting: Allow optional use of semihosting from userspace
  target/arm: Honour -semihosting-config userspace=on
  target/m68k: Honour -semihosting-config userspace=on
  target/mips: Honour -semihosting-config userspace=on
  target/nios2: Honour -semihosting-config userspace=on
  target/xtensa: Honour -semihosting-config userspace=on
  target/riscv: Honour -semihosting-config userspace=on and enable=on

 include/semihosting/semihost.h            | 10 ++++++++--
 semihosting/config.c                      | 10 ++++++++--
 softmmu/vl.c                              |  2 +-
 stubs/semihost.c                          |  2 +-
 target/arm/translate-a64.c                | 12 +-----------
 target/arm/translate.c                    | 16 ++++------------
 target/m68k/op_helper.c                   |  3 +--
 target/mips/tcg/translate.c               |  9 +++++----
 target/nios2/translate.c                  |  3 ++-
 target/riscv/cpu_helper.c                 |  3 ++-
 target/xtensa/translate.c                 |  7 ++++---
 target/mips/tcg/micromips_translate.c.inc |  6 +++---
 target/mips/tcg/mips16e_translate.c.inc   |  2 +-
 target/mips/tcg/nanomips_translate.c.inc  |  4 ++--
 qemu-options.hx                           | 11 +++++++++--
 15 files changed, 52 insertions(+), 48 deletions(-)

-- 
2.25.1

[PATCH 1/7] semihosting: Allow optional use of semihosting from userspace

[Peter Maydell]

Currently our semihosting implementations generally prohibit use of
semihosting calls in system emulation from the guest userspace.  This
is a very long standing behaviour justified originally "to provide
some semblance of security" (since code with access to the
semihosting ABI can do things like read and write arbitrary files on
the host system).  However, it is sometimes useful to be able to run
trusted guest code which performs semihosting calls from guest
userspace, notably for test code.  Add a command line suboption to
the existing semihosting-config option group so that you can
explicitly opt in to semihosting from guest userspace with
 -semihosting-config userspace=on

(There is no equivalent option for the user-mode emulator, because
there by definition all code runs in userspace and has access to
semihosting already.)

This commit adds the infrastructure for the command line option and
adds a bool 'is_user' parameter to the function
semihosting_userspace_enabled() that target code can use to check
whether it should be permitting the semihosting call for userspace.
It mechanically makes all the callsites pass 'false', so they
continue checking "is semihosting enabled in general".  Subsequent
commits will make each target that implements semihosting honour the
userspace=on option by passing the correct value and removing
whatever "don't do this for userspace" checking they were doing by
hand.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/semihosting/semihost.h | 10 ++++++++--
 semihosting/config.c           | 10 ++++++++--
 softmmu/vl.c                   |  2 +-
 stubs/semihost.c               |  2 +-
 target/arm/translate-a64.c     |  2 +-
 target/arm/translate.c         |  6 +++---
 target/m68k/op_helper.c        |  2 +-
 target/nios2/translate.c       |  2 +-
 target/xtensa/translate.c      |  6 +++---
 qemu-options.hx                | 11 +++++++++--
 10 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/include/semihosting/semihost.h b/include/semihosting/semihost.h
index 93a3c21b44d..efd2efa25ae 100644
--- a/include/semihosting/semihost.h
+++ b/include/semihosting/semihost.h
@@ -27,7 +27,7 @@ typedef enum SemihostingTarget {
 } SemihostingTarget;
 
 #ifdef CONFIG_USER_ONLY
-static inline bool semihosting_enabled(void)
+static inline bool semihosting_enabled(bool is_user)
 {
     return true;
 }
@@ -52,7 +52,13 @@ static inline const char *semihosting_get_cmdline(void)
     return NULL;
 }
 #else /* !CONFIG_USER_ONLY */
-bool semihosting_enabled(void);
+/**
+ * semihosting_enabled:
+ * @is_user: true if guest code is in usermode (i.e. not privileged)
+ *
+ * Return true if guest code is allowed to make semihosting calls.
+ */
+bool semihosting_enabled(bool is_user);
 SemihostingTarget semihosting_get_target(void);
 const char *semihosting_get_arg(int i);
 int semihosting_get_argc(void);
diff --git a/semihosting/config.c b/semihosting/config.c
index e171d4d6bc3..89a17596879 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -34,6 +34,9 @@ QemuOptsList qemu_semihosting_config_opts = {
         {
             .name = "enable",
             .type = QEMU_OPT_BOOL,
+        }, {
+            .name = "userspace",
+            .type = QEMU_OPT_BOOL,
         }, {
             .name = "target",
             .type = QEMU_OPT_STRING,
@@ -50,6 +53,7 @@ QemuOptsList qemu_semihosting_config_opts = {
 
 typedef struct SemihostingConfig {
     bool enabled;
+    bool userspace_enabled;
     SemihostingTarget target;
     char **argv;
     int argc;
@@ -59,9 +63,9 @@ typedef struct SemihostingConfig {
 static SemihostingConfig semihosting;
 static const char *semihost_chardev;
 
-bool semihosting_enabled(void)
+bool semihosting_enabled(bool is_user)
 {
-    return semihosting.enabled;
+    return semihosting.enabled && (!is_user || semihosting.userspace_enabled);
 }
 
 SemihostingTarget semihosting_get_target(void)
@@ -137,6 +141,8 @@ int qemu_semihosting_config_options(const char *optarg)
     if (opts != NULL) {
         semihosting.enabled = qemu_opt_get_bool(opts, "enable",
                                                 true);
+        semihosting.userspace_enabled = qemu_opt_get_bool(opts, "userspace",
+                                                          false);
         const char *target = qemu_opt_get(opts, "target");
         /* setup of chardev is deferred until they are initialised */
         semihost_chardev = qemu_opt_get(opts, "chardev");
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 706bd7cff79..3593f1d7821 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1822,7 +1822,7 @@ static void qemu_apply_machine_options(QDict *qdict)
 {
     object_set_properties_from_keyval(OBJECT(current_machine), qdict, false, &error_fatal);
 
-    if (semihosting_enabled() && !semihosting_get_argc()) {
+    if (semihosting_enabled(false) && !semihosting_get_argc()) {
         /* fall back to the -kernel/-append */
         semihosting_arg_fallback(current_machine->kernel_filename, current_machine->kernel_cmdline);
     }
diff --git a/stubs/semihost.c b/stubs/semihost.c
index f486651afbb..d65c9fd5dcf 100644
--- a/stubs/semihost.c
+++ b/stubs/semihost.c
@@ -23,7 +23,7 @@ QemuOptsList qemu_semihosting_config_opts = {
 };
 
 /* Queries to config status default to off */
-bool semihosting_enabled(void)
+bool semihosting_enabled(bool is_user)
 {
     return false;
 }
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 163df8c6157..3decc8da573 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2219,7 +2219,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
          * it is required for halting debug disabled: it will UNDEF.
          * Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction.
          */
-        if (semihosting_enabled() && imm16 == 0xf000) {
+        if (semihosting_enabled(false) && imm16 == 0xf000) {
 #ifndef CONFIG_USER_ONLY
             /* In system mode, don't allow userspace access to semihosting,
              * to provide some semblance of security (and for consistency
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ad617b99481..b85be8a818d 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1169,7 +1169,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
      * semihosting, to provide some semblance of security
      * (and for consistency with our 32-bit semihosting).
      */
-    if (semihosting_enabled() &&
+    if (semihosting_enabled(false) &&
 #ifndef CONFIG_USER_ONLY
         s->current_el != 0 &&
 #endif
@@ -6556,7 +6556,7 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
     /* BKPT is OK with ECI set and leaves it untouched */
     s->eci_handled = true;
     if (arm_dc_feature(s, ARM_FEATURE_M) &&
-        semihosting_enabled() &&
+        semihosting_enabled(false) &&
 #ifndef CONFIG_USER_ONLY
         !IS_USER(s) &&
 #endif
@@ -8764,7 +8764,7 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a)
 {
     const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
 
-    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() &&
+    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled(false) &&
 #ifndef CONFIG_USER_ONLY
         !IS_USER(s) &&
 #endif
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index d9937ca8dc5..4b3dfec1306 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -203,7 +203,7 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw)
             cf_rte(env);
             return;
         case EXCP_HALT_INSN:
-            if (semihosting_enabled()
+            if (semihosting_enabled(false)
                     && (env->sr & SR_S) != 0
                     && (env->pc & 3) == 0
                     && cpu_lduw_code(env, env->pc - 4) == 0x4e71
diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 3a037a68cc4..2b556683422 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -818,7 +818,7 @@ static void gen_break(DisasContext *dc, uint32_t code, uint32_t flags)
 #ifndef CONFIG_USER_ONLY
     /* The semihosting instruction is "break 1".  */
     R_TYPE(instr, code);
-    if (semihosting_enabled() && instr.imm5 == 1) {
+    if (semihosting_enabled(false) && instr.imm5 == 1) {
         t_gen_helper_raise_exception(dc, EXCP_SEMIHOST);
         return;
     }
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 70e11eeb459..dc475a4274b 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -2364,9 +2364,9 @@ static uint32_t test_exceptions_simcall(DisasContext *dc,
     bool ill = true;
 #else
     /* Between RE.2 and RE.3 simcall opcode's become nop for the hardware. */
-    bool ill = dc->config->hw_version <= 250002 && !semihosting_enabled();
+    bool ill = dc->config->hw_version <= 250002 && !semihosting_enabled(false);
 #endif
-    if (ill || !semihosting_enabled()) {
+    if (ill || !semihosting_enabled(false)) {
         qemu_log_mask(LOG_GUEST_ERROR, "SIMCALL but semihosting is disabled\n");
     }
     return ill ? XTENSA_OP_ILL : 0;
@@ -2376,7 +2376,7 @@ static void translate_simcall(DisasContext *dc, const OpcodeArg arg[],
                               const uint32_t par[])
 {
 #ifndef CONFIG_USER_ONLY
-    if (semihosting_enabled()) {
+    if (semihosting_enabled(false)) {
         gen_helper_simcall(cpu_env);
     }
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 3f23a42fa87..4e7111abe3d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4614,12 +4614,12 @@ SRST
     information about the facilities this enables.
 ERST
 DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
-    "-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
+    "-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,userspace=on|off][,arg=str[,...]]\n" \
     "                semihosting configuration\n",
 QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA |
 QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2 | QEMU_ARCH_RISCV)
 SRST
-``-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]``
+``-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,userspace=on|off][,arg=str[,...]]``
     Enable and configure semihosting (ARM, M68K, Xtensa, MIPS, Nios II, RISC-V
     only).
 
@@ -4646,6 +4646,13 @@ SRST
         Send the output to a chardev backend output for native or auto
         output when not in gdb
 
+    ``userspace=on|off``
+        Allows code running in guest userspace to access the semihosting
+        interface. The default is that only privileged guest code can
+        make semihosting calls. Note that setting ``userspace=on`` should
+        only be used if all guest code is trusted (for example, in
+        bare-metal test case code).
+
     ``arg=str1,arg=str2,...``
         Allows the user to pass input arguments, and can be used
         multiple times to build up a list. The old-style
-- 
2.25.1

[PATCH 2/7] target/arm: Honour -semihosting-config userspace=on

[Peter Maydell]

Honour the commandline -semihosting-config userspace=on option,
instead of never permitting userspace semihosting calls in system
emulation mode, by passing the correct value to the is_userspace
argument of semihosting_enabled(), instead of manually checking and
always forbidding semihosting if the guest is in userspace and this
isn't the linux-user build.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 12 +-----------
 target/arm/translate.c     | 16 ++++------------
 2 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 3decc8da573..9bed336b47e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2219,17 +2219,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
          * it is required for halting debug disabled: it will UNDEF.
          * Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction.
          */
-        if (semihosting_enabled(false) && imm16 == 0xf000) {
-#ifndef CONFIG_USER_ONLY
-            /* In system mode, don't allow userspace access to semihosting,
-             * to provide some semblance of security (and for consistency
-             * with our 32-bit semihosting).
-             */
-            if (s->current_el == 0) {
-                unallocated_encoding(s);
-                break;
-            }
-#endif
+        if (semihosting_enabled(s->current_el == 0) && imm16 == 0xf000) {
             gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
         } else {
             unallocated_encoding(s);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index b85be8a818d..54543b7c2a8 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1169,10 +1169,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
      * semihosting, to provide some semblance of security
      * (and for consistency with our 32-bit semihosting).
      */
-    if (semihosting_enabled(false) &&
-#ifndef CONFIG_USER_ONLY
-        s->current_el != 0 &&
-#endif
+    if (semihosting_enabled(s->current_el != 0) &&
         (imm == (s->thumb ? 0x3c : 0xf000))) {
         gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
         return;
@@ -6556,10 +6553,7 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
     /* BKPT is OK with ECI set and leaves it untouched */
     s->eci_handled = true;
     if (arm_dc_feature(s, ARM_FEATURE_M) &&
-        semihosting_enabled(false) &&
-#ifndef CONFIG_USER_ONLY
-        !IS_USER(s) &&
-#endif
+        semihosting_enabled(s->current_el == 0) &&
         (a->imm == 0xab)) {
         gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
     } else {
@@ -8764,10 +8758,8 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a)
 {
     const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
 
-    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled(false) &&
-#ifndef CONFIG_USER_ONLY
-        !IS_USER(s) &&
-#endif
+    if (!arm_dc_feature(s, ARM_FEATURE_M) &&
+        semihosting_enabled(s->current_el == 0) &&
         (a->imm == semihost_imm)) {
         gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
     } else {
-- 
2.25.1

[PATCH 3/7] target/m68k: Honour -semihosting-config userspace=on

[Peter Maydell]

Honour the commandline -semihosting-config userspace=on option,
instead of never permitting userspace semihosting calls in system
emulation mode, by passing the correct value to the is_userspace
argument of semihosting_enabled(), instead of manually checking and
always forbidding semihosting if the guest is in userspace.

(Note that target/m68k doesn't support semihosting at all
in the linux-user build.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/m68k/op_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 4b3dfec1306..a96a0340506 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -203,8 +203,7 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw)
             cf_rte(env);
             return;
         case EXCP_HALT_INSN:
-            if (semihosting_enabled(false)
-                    && (env->sr & SR_S) != 0
+            if (semihosting_enabled((env->sr & SR_S) == 0)
                     && (env->pc & 3) == 0
                     && cpu_lduw_code(env, env->pc - 4) == 0x4e71
                     && cpu_ldl_code(env, env->pc) == 0x4e7bf000) {
-- 
2.25.1

[PATCH 4/7] target/mips: Honour -semihosting-config userspace=on

[Peter Maydell]

Honour the commandline -semihosting-config userspace=on option,
instead of always permitting userspace semihosting calls in system
emulation mode, by passing the correct value to the is_userspace
argument of semihosting_enabled().

Note that this is a behaviour change: if the user wants to
do semihosting calls from userspace they must now specifically
enable them on the command line.

MIPS semihosting is not implemented for linux-user builds.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/mips/tcg/translate.c               | 9 +++++----
 target/mips/tcg/micromips_translate.c.inc | 6 +++---
 target/mips/tcg/mips16e_translate.c.inc   | 2 +-
 target/mips/tcg/nanomips_translate.c.inc  | 4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index de1511baaf8..53886618ddd 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -12082,12 +12082,13 @@ static void gen_cache_operation(DisasContext *ctx, uint32_t op, int base,
     tcg_temp_free_i32(t0);
 }
 
-static inline bool is_uhi(int sdbbp_code)
+static inline bool is_uhi(DisasContext *ctx, int sdbbp_code)
 {
 #ifdef CONFIG_USER_ONLY
     return false;
 #else
-    return semihosting_enabled() && sdbbp_code == 1;
+    bool is_user = (ctx->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM;
+    return semihosting_enabled(is_user) && sdbbp_code == 1;
 #endif
 }
 
@@ -13898,7 +13899,7 @@ static void decode_opc_special_r6(CPUMIPSState *env, DisasContext *ctx)
         }
         break;
     case R6_OPC_SDBBP:
-        if (is_uhi(extract32(ctx->opcode, 6, 20))) {
+        if (is_uhi(ctx, extract32(ctx->opcode, 6, 20))) {
             ctx->base.is_jmp = DISAS_SEMIHOST;
         } else {
             if (ctx->hflags & MIPS_HFLAG_SBRI) {
@@ -14310,7 +14311,7 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
         gen_cl(ctx, op1, rd, rs);
         break;
     case OPC_SDBBP:
-        if (is_uhi(extract32(ctx->opcode, 6, 20))) {
+        if (is_uhi(ctx, extract32(ctx->opcode, 6, 20))) {
             ctx->base.is_jmp = DISAS_SEMIHOST;
         } else {
             /*
diff --git a/target/mips/tcg/micromips_translate.c.inc b/target/mips/tcg/micromips_translate.c.inc
index b2c696f8916..632895cc9ef 100644
--- a/target/mips/tcg/micromips_translate.c.inc
+++ b/target/mips/tcg/micromips_translate.c.inc
@@ -825,7 +825,7 @@ static void gen_pool16c_insn(DisasContext *ctx)
         generate_exception_break(ctx, extract32(ctx->opcode, 0, 4));
         break;
     case SDBBP16:
-        if (is_uhi(extract32(ctx->opcode, 0, 4))) {
+        if (is_uhi(ctx, extract32(ctx->opcode, 0, 4))) {
             ctx->base.is_jmp = DISAS_SEMIHOST;
         } else {
             /*
@@ -941,7 +941,7 @@ static void gen_pool16c_r6_insn(DisasContext *ctx)
             break;
         case R6_SDBBP16:
             /* SDBBP16 */
-            if (is_uhi(extract32(ctx->opcode, 6, 4))) {
+            if (is_uhi(ctx, extract32(ctx->opcode, 6, 4))) {
                 ctx->base.is_jmp = DISAS_SEMIHOST;
             } else {
                 if (ctx->hflags & MIPS_HFLAG_SBRI) {
@@ -1310,7 +1310,7 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
             generate_exception_end(ctx, EXCP_SYSCALL);
             break;
         case SDBBP:
-            if (is_uhi(extract32(ctx->opcode, 16, 10))) {
+            if (is_uhi(ctx, extract32(ctx->opcode, 16, 10))) {
                 ctx->base.is_jmp = DISAS_SEMIHOST;
             } else {
                 check_insn(ctx, ISA_MIPS_R1);
diff --git a/target/mips/tcg/mips16e_translate.c.inc b/target/mips/tcg/mips16e_translate.c.inc
index 7568933e234..918b15d55ce 100644
--- a/target/mips/tcg/mips16e_translate.c.inc
+++ b/target/mips/tcg/mips16e_translate.c.inc
@@ -951,7 +951,7 @@ static int decode_ase_mips16e(CPUMIPSState *env, DisasContext *ctx)
             }
             break;
         case RR_SDBBP:
-            if (is_uhi(extract32(ctx->opcode, 5, 6))) {
+            if (is_uhi(ctx, extract32(ctx->opcode, 5, 6))) {
                 ctx->base.is_jmp = DISAS_SEMIHOST;
             } else {
                 /*
diff --git a/target/mips/tcg/nanomips_translate.c.inc b/target/mips/tcg/nanomips_translate.c.inc
index b3aff22c189..812c111e3c3 100644
--- a/target/mips/tcg/nanomips_translate.c.inc
+++ b/target/mips/tcg/nanomips_translate.c.inc
@@ -3694,7 +3694,7 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                 generate_exception_end(ctx, EXCP_BREAK);
                 break;
             case NM_SDBBP:
-                if (is_uhi(extract32(ctx->opcode, 0, 19))) {
+                if (is_uhi(ctx, extract32(ctx->opcode, 0, 19))) {
                     ctx->base.is_jmp = DISAS_SEMIHOST;
                 } else {
                     if (ctx->hflags & MIPS_HFLAG_SBRI) {
@@ -4633,7 +4633,7 @@ static int decode_isa_nanomips(CPUMIPSState *env, DisasContext *ctx)
                 generate_exception_end(ctx, EXCP_BREAK);
                 break;
             case NM_SDBBP16:
-                if (is_uhi(extract32(ctx->opcode, 0, 3))) {
+                if (is_uhi(ctx, extract32(ctx->opcode, 0, 3))) {
                     ctx->base.is_jmp = DISAS_SEMIHOST;
                 } else {
                     if (ctx->hflags & MIPS_HFLAG_SBRI) {
-- 
2.25.1

[PATCH 5/7] target/nios2: Honour -semihosting-config userspace=on

[Peter Maydell]

Honour the commandline -semihosting-config userspace=on option,
instead of always permitting userspace semihosting calls in system
emulation mode, by passing the correct value to the is_userspace
argument of semihosting_enabled().

Note that this is a behaviour change: if the user wants to
do semihosting calls from userspace they must now specifically
enable them on the command line.

nios2 semihosting is not implemented for linux-user builds.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/nios2/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 2b556683422..d1786b43a69 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -817,8 +817,9 @@ static void gen_break(DisasContext *dc, uint32_t code, uint32_t flags)
 {
 #ifndef CONFIG_USER_ONLY
     /* The semihosting instruction is "break 1".  */
+    bool is_user = FIELD_EX32(dc->tb_flags, TBFLAGS, U);
     R_TYPE(instr, code);
-    if (semihosting_enabled(false) && instr.imm5 == 1) {
+    if (semihosting_enabled(is_user) && instr.imm5 == 1) {
         t_gen_helper_raise_exception(dc, EXCP_SEMIHOST);
         return;
     }
-- 
2.25.1

[PATCH 6/7] target/xtensa: Honour -semihosting-config userspace=on

[Peter Maydell]

Honour the commandline -semihosting-config userspace=on option,
instead of always permitting userspace semihosting calls in system
emulation mode, by passing the correct value to the is_userspace
argument of semihosting_enabled().

Note that this is a behaviour change: if the user wants to
do semihosting calls from userspace they must now specifically
enable them on the command line.

xtensa semihosting is not implemented for linux-user builds.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/xtensa/translate.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index dc475a4274b..43d55989349 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -2360,13 +2360,14 @@ static uint32_t test_exceptions_simcall(DisasContext *dc,
                                         const OpcodeArg arg[],
                                         const uint32_t par[])
 {
+    bool is_semi = semihosting_enabled(dc->cring != 0);
 #ifdef CONFIG_USER_ONLY
     bool ill = true;
 #else
     /* Between RE.2 and RE.3 simcall opcode's become nop for the hardware. */
-    bool ill = dc->config->hw_version <= 250002 && !semihosting_enabled(false);
+    bool ill = dc->config->hw_version <= 250002 && !is_semi;
 #endif
-    if (ill || !semihosting_enabled(false)) {
+    if (ill || !is_semi) {
         qemu_log_mask(LOG_GUEST_ERROR, "SIMCALL but semihosting is disabled\n");
     }
     return ill ? XTENSA_OP_ILL : 0;
@@ -2376,7 +2377,7 @@ static void translate_simcall(DisasContext *dc, const OpcodeArg arg[],
                               const uint32_t par[])
 {
 #ifndef CONFIG_USER_ONLY
-    if (semihosting_enabled(false)) {
+    if (semihosting_enabled(dc->cring != 0)) {
         gen_helper_simcall(cpu_env);
     }
 #endif
-- 
2.25.1

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

[Peter Maydell]

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

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

[Richard Henderson]

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~

Re: [PATCH 0/7] Allow semihosting from user mode

[Richard Henderson]

On 8/15/22 14:02, Peter Maydell wrote:
> Peter Maydell (7):
>    semihosting: Allow optional use of semihosting from userspace
>    target/arm: Honour -semihosting-config userspace=on
>    target/m68k: Honour -semihosting-config userspace=on
>    target/mips: Honour -semihosting-config userspace=on
>    target/nios2: Honour -semihosting-config userspace=on
>    target/xtensa: Honour -semihosting-config userspace=on
>    target/riscv: Honour -semihosting-config userspace=on and enable=on

Except riscv, for which I think needs changes,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 6/7] target/xtensa: Honour -semihosting-config userspace=on

[Max Filippov]

On Mon, Aug 15, 2022 at 12:03 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Honour the commandline -semihosting-config userspace=on option,
> instead of always permitting userspace semihosting calls in system
> emulation mode, by passing the correct value to the is_userspace
> argument of semihosting_enabled().
>
> Note that this is a behaviour change: if the user wants to
> do semihosting calls from userspace they must now specifically
> enable them on the command line.
>
> xtensa semihosting is not implemented for linux-user builds.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/xtensa/translate.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

Re: [PATCH 1/7] semihosting: Allow optional use of semihosting from userspace

[Philippe Mathieu-Daudé via]

On 15/8/22 21:02, Peter Maydell wrote:
> Currently our semihosting implementations generally prohibit use of
> semihosting calls in system emulation from the guest userspace.  This
> is a very long standing behaviour justified originally "to provide
> some semblance of security" (since code with access to the
> semihosting ABI can do things like read and write arbitrary files on
> the host system).  However, it is sometimes useful to be able to run
> trusted guest code which performs semihosting calls from guest
> userspace, notably for test code.  Add a command line suboption to
> the existing semihosting-config option group so that you can
> explicitly opt in to semihosting from guest userspace with
>   -semihosting-config userspace=on
> 
> (There is no equivalent option for the user-mode emulator, because
> there by definition all code runs in userspace and has access to
> semihosting already.)
> 
> This commit adds the infrastructure for the command line option and
> adds a bool 'is_user' parameter to the function
> semihosting_userspace_enabled() that target code can use to check
> whether it should be permitting the semihosting call for userspace.
> It mechanically makes all the callsites pass 'false', so they
> continue checking "is semihosting enabled in general".  Subsequent
> commits will make each target that implements semihosting honour the
> userspace=on option by passing the correct value and removing
> whatever "don't do this for userspace" checking they were doing by
> hand.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/semihosting/semihost.h | 10 ++++++++--
>   semihosting/config.c           | 10 ++++++++--
>   softmmu/vl.c                   |  2 +-
>   stubs/semihost.c               |  2 +-
>   target/arm/translate-a64.c     |  2 +-
>   target/arm/translate.c         |  6 +++---
>   target/m68k/op_helper.c        |  2 +-
>   target/nios2/translate.c       |  2 +-
>   target/xtensa/translate.c      |  6 +++---
>   qemu-options.hx                | 11 +++++++++--
>   10 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/include/semihosting/semihost.h b/include/semihosting/semihost.h
> index 93a3c21b44d..efd2efa25ae 100644
> --- a/include/semihosting/semihost.h
> +++ b/include/semihosting/semihost.h
> @@ -27,7 +27,7 @@ typedef enum SemihostingTarget {
>   } SemihostingTarget;
>   
>   #ifdef CONFIG_USER_ONLY
> -static inline bool semihosting_enabled(void)
> +static inline bool semihosting_enabled(bool is_user)
>   {
>       return true;
>   }
> @@ -52,7 +52,13 @@ static inline const char *semihosting_get_cmdline(void)
>       return NULL;
>   }
>   #else /* !CONFIG_USER_ONLY */
> -bool semihosting_enabled(void);
> +/**
> + * semihosting_enabled:
> + * @is_user: true if guest code is in usermode (i.e. not privileged)
> + *
> + * Return true if guest code is allowed to make semihosting calls.
> + */
> +bool semihosting_enabled(bool is_user);

Per the description, possibly rename as semihosting_allowed().

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

Re: [PATCH 1/7] semihosting: Allow optional use of semihosting from userspace

[Philippe Mathieu-Daudé]

On 15/8/22 21:02, Peter Maydell wrote:
> Currently our semihosting implementations generally prohibit use of
> semihosting calls in system emulation from the guest userspace.  This
> is a very long standing behaviour justified originally "to provide
> some semblance of security" (since code with access to the
> semihosting ABI can do things like read and write arbitrary files on
> the host system).  However, it is sometimes useful to be able to run
> trusted guest code which performs semihosting calls from guest
> userspace, notably for test code.  Add a command line suboption to
> the existing semihosting-config option group so that you can
> explicitly opt in to semihosting from guest userspace with
>   -semihosting-config userspace=on
> 
> (There is no equivalent option for the user-mode emulator, because
> there by definition all code runs in userspace and has access to
> semihosting already.)
> 
> This commit adds the infrastructure for the command line option and
> adds a bool 'is_user' parameter to the function
> semihosting_userspace_enabled() that target code can use to check
> whether it should be permitting the semihosting call for userspace.
> It mechanically makes all the callsites pass 'false', so they
> continue checking "is semihosting enabled in general".  Subsequent
> commits will make each target that implements semihosting honour the
> userspace=on option by passing the correct value and removing
> whatever "don't do this for userspace" checking they were doing by
> hand.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/semihosting/semihost.h | 10 ++++++++--
>   semihosting/config.c           | 10 ++++++++--
>   softmmu/vl.c                   |  2 +-
>   stubs/semihost.c               |  2 +-
>   target/arm/translate-a64.c     |  2 +-
>   target/arm/translate.c         |  6 +++---
>   target/m68k/op_helper.c        |  2 +-
>   target/nios2/translate.c       |  2 +-
>   target/xtensa/translate.c      |  6 +++---
>   qemu-options.hx                | 11 +++++++++--
>   10 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/include/semihosting/semihost.h b/include/semihosting/semihost.h
> index 93a3c21b44d..efd2efa25ae 100644
> --- a/include/semihosting/semihost.h
> +++ b/include/semihosting/semihost.h
> @@ -27,7 +27,7 @@ typedef enum SemihostingTarget {
>   } SemihostingTarget;
>   
>   #ifdef CONFIG_USER_ONLY
> -static inline bool semihosting_enabled(void)
> +static inline bool semihosting_enabled(bool is_user)
>   {
>       return true;
>   }
> @@ -52,7 +52,13 @@ static inline const char *semihosting_get_cmdline(void)
>       return NULL;
>   }
>   #else /* !CONFIG_USER_ONLY */
> -bool semihosting_enabled(void);
> +/**
> + * semihosting_enabled:
> + * @is_user: true if guest code is in usermode (i.e. not privileged)
> + *
> + * Return true if guest code is allowed to make semihosting calls.
> + */
> +bool semihosting_enabled(bool is_user);

Per the description, possibly rename as semihosting_allowed().

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

Re: [PATCH 4/7] target/mips: Honour -semihosting-config userspace=on

[Philippe Mathieu-Daudé via]

On 15/8/22 21:03, Peter Maydell wrote:
> Honour the commandline -semihosting-config userspace=on option,
> instead of always permitting userspace semihosting calls in system
> emulation mode, by passing the correct value to the is_userspace
> argument of semihosting_enabled().
> 
> Note that this is a behaviour change: if the user wants to
> do semihosting calls from userspace they must now specifically
> enable them on the command line.
> 
> MIPS semihosting is not implemented for linux-user builds.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/mips/tcg/translate.c               | 9 +++++----
>   target/mips/tcg/micromips_translate.c.inc | 6 +++---
>   target/mips/tcg/mips16e_translate.c.inc   | 2 +-
>   target/mips/tcg/nanomips_translate.c.inc  | 4 ++--
>   4 files changed, 11 insertions(+), 10 deletions(-)

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

Re: [PATCH 4/7] target/mips: Honour -semihosting-config userspace=on

[Philippe Mathieu-Daudé]

On 15/8/22 21:03, Peter Maydell wrote:
> Honour the commandline -semihosting-config userspace=on option,
> instead of always permitting userspace semihosting calls in system
> emulation mode, by passing the correct value to the is_userspace
> argument of semihosting_enabled().
> 
> Note that this is a behaviour change: if the user wants to
> do semihosting calls from userspace they must now specifically
> enable them on the command line.
> 
> MIPS semihosting is not implemented for linux-user builds.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/mips/tcg/translate.c               | 9 +++++----
>   target/mips/tcg/micromips_translate.c.inc | 6 +++---
>   target/mips/tcg/mips16e_translate.c.inc   | 2 +-
>   target/mips/tcg/nanomips_translate.c.inc  | 4 ++--
>   4 files changed, 11 insertions(+), 10 deletions(-)

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

Re: [PATCH 6/7] target/xtensa: Honour -semihosting-config userspace=on

[Philippe Mathieu-Daudé via]

On 15/8/22 21:03, Peter Maydell wrote:
> Honour the commandline -semihosting-config userspace=on option,
> instead of always permitting userspace semihosting calls in system
> emulation mode, by passing the correct value to the is_userspace
> argument of semihosting_enabled().
> 
> Note that this is a behaviour change: if the user wants to
> do semihosting calls from userspace they must now specifically
> enable them on the command line.
> 
> xtensa semihosting is not implemented for linux-user builds.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/xtensa/translate.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

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

Re: [PATCH 6/7] target/xtensa: Honour -semihosting-config userspace=on

[Philippe Mathieu-Daudé]

On 15/8/22 21:03, Peter Maydell wrote:
> Honour the commandline -semihosting-config userspace=on option,
> instead of always permitting userspace semihosting calls in system
> emulation mode, by passing the correct value to the is_userspace
> argument of semihosting_enabled().
> 
> Note that this is a behaviour change: if the user wants to
> do semihosting calls from userspace they must now specifically
> enable them on the command line.
> 
> xtensa semihosting is not implemented for linux-user builds.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/xtensa/translate.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

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

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

[Furquan Shaikh]

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

Re: [PATCH 1/7] semihosting: Allow optional use of semihosting from userspace

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Currently our semihosting implementations generally prohibit use of
> semihosting calls in system emulation from the guest userspace.  This
> is a very long standing behaviour justified originally "to provide
> some semblance of security" (since code with access to the
> semihosting ABI can do things like read and write arbitrary files on
> the host system).  However, it is sometimes useful to be able to run
> trusted guest code which performs semihosting calls from guest
> userspace, notably for test code.  Add a command line suboption to
> the existing semihosting-config option group so that you can
> explicitly opt in to semihosting from guest userspace with
>  -semihosting-config userspace=on
>
> (There is no equivalent option for the user-mode emulator, because
> there by definition all code runs in userspace and has access to
> semihosting already.)
>
> This commit adds the infrastructure for the command line option and
> adds a bool 'is_user' parameter to the function
> semihosting_userspace_enabled() that target code can use to check
> whether it should be permitting the semihosting call for userspace.
> It mechanically makes all the callsites pass 'false', so they
> continue checking "is semihosting enabled in general".  Subsequent
> commits will make each target that implements semihosting honour the
> userspace=on option by passing the correct value and removing
> whatever "don't do this for userspace" checking they were doing by
> hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 3/7] target/m68k: Honour -semihosting-config userspace=on

[Laurent Vivier]

Le 15/08/2022 à 21:02, Peter Maydell a écrit :
> Honour the commandline -semihosting-config userspace=on option,
> instead of never permitting userspace semihosting calls in system
> emulation mode, by passing the correct value to the is_userspace
> argument of semihosting_enabled(), instead of manually checking and
> always forbidding semihosting if the guest is in userspace.
> 
> (Note that target/m68k doesn't support semihosting at all
> in the linux-user build.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/op_helper.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 4b3dfec1306..a96a0340506 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -203,8 +203,7 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw)
>               cf_rte(env);
>               return;
>           case EXCP_HALT_INSN:
> -            if (semihosting_enabled(false)
> -                    && (env->sr & SR_S) != 0
> +            if (semihosting_enabled((env->sr & SR_S) == 0)
>                       && (env->pc & 3) == 0
>                       && cpu_lduw_code(env, env->pc - 4) == 0x4e71
>                       && cpu_ldl_code(env, env->pc) == 0x4e7bf000) {

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 1/7] semihosting: Allow optional use of semihosting from userspace

[Alistair Francis]

On Tue, Aug 16, 2022 at 5:03 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently our semihosting implementations generally prohibit use of
> semihosting calls in system emulation from the guest userspace.  This
> is a very long standing behaviour justified originally "to provide
> some semblance of security" (since code with access to the
> semihosting ABI can do things like read and write arbitrary files on
> the host system).  However, it is sometimes useful to be able to run
> trusted guest code which performs semihosting calls from guest
> userspace, notably for test code.  Add a command line suboption to
> the existing semihosting-config option group so that you can
> explicitly opt in to semihosting from guest userspace with
>  -semihosting-config userspace=on
>
> (There is no equivalent option for the user-mode emulator, because
> there by definition all code runs in userspace and has access to
> semihosting already.)
>
> This commit adds the infrastructure for the command line option and
> adds a bool 'is_user' parameter to the function
> semihosting_userspace_enabled() that target code can use to check
> whether it should be permitting the semihosting call for userspace.
> It mechanically makes all the callsites pass 'false', so they
> continue checking "is semihosting enabled in general".  Subsequent
> commits will make each target that implements semihosting honour the
> userspace=on option by passing the correct value and removing
> whatever "don't do this for userspace" checking they were doing by
> hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/semihosting/semihost.h | 10 ++++++++--
>  semihosting/config.c           | 10 ++++++++--
>  softmmu/vl.c                   |  2 +-
>  stubs/semihost.c               |  2 +-
>  target/arm/translate-a64.c     |  2 +-
>  target/arm/translate.c         |  6 +++---
>  target/m68k/op_helper.c        |  2 +-
>  target/nios2/translate.c       |  2 +-
>  target/xtensa/translate.c      |  6 +++---
>  qemu-options.hx                | 11 +++++++++--
>  10 files changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/include/semihosting/semihost.h b/include/semihosting/semihost.h
> index 93a3c21b44d..efd2efa25ae 100644
> --- a/include/semihosting/semihost.h
> +++ b/include/semihosting/semihost.h
> @@ -27,7 +27,7 @@ typedef enum SemihostingTarget {
>  } SemihostingTarget;
>
>  #ifdef CONFIG_USER_ONLY
> -static inline bool semihosting_enabled(void)
> +static inline bool semihosting_enabled(bool is_user)
>  {
>      return true;
>  }
> @@ -52,7 +52,13 @@ static inline const char *semihosting_get_cmdline(void)
>      return NULL;
>  }
>  #else /* !CONFIG_USER_ONLY */
> -bool semihosting_enabled(void);
> +/**
> + * semihosting_enabled:
> + * @is_user: true if guest code is in usermode (i.e. not privileged)
> + *
> + * Return true if guest code is allowed to make semihosting calls.
> + */
> +bool semihosting_enabled(bool is_user);
>  SemihostingTarget semihosting_get_target(void);
>  const char *semihosting_get_arg(int i);
>  int semihosting_get_argc(void);
> diff --git a/semihosting/config.c b/semihosting/config.c
> index e171d4d6bc3..89a17596879 100644
> --- a/semihosting/config.c
> +++ b/semihosting/config.c
> @@ -34,6 +34,9 @@ QemuOptsList qemu_semihosting_config_opts = {
>          {
>              .name = "enable",
>              .type = QEMU_OPT_BOOL,
> +        }, {
> +            .name = "userspace",
> +            .type = QEMU_OPT_BOOL,
>          }, {
>              .name = "target",
>              .type = QEMU_OPT_STRING,
> @@ -50,6 +53,7 @@ QemuOptsList qemu_semihosting_config_opts = {
>
>  typedef struct SemihostingConfig {
>      bool enabled;
> +    bool userspace_enabled;
>      SemihostingTarget target;
>      char **argv;
>      int argc;
> @@ -59,9 +63,9 @@ typedef struct SemihostingConfig {
>  static SemihostingConfig semihosting;
>  static const char *semihost_chardev;
>
> -bool semihosting_enabled(void)
> +bool semihosting_enabled(bool is_user)
>  {
> -    return semihosting.enabled;
> +    return semihosting.enabled && (!is_user || semihosting.userspace_enabled);
>  }
>
>  SemihostingTarget semihosting_get_target(void)
> @@ -137,6 +141,8 @@ int qemu_semihosting_config_options(const char *optarg)
>      if (opts != NULL) {
>          semihosting.enabled = qemu_opt_get_bool(opts, "enable",
>                                                  true);
> +        semihosting.userspace_enabled = qemu_opt_get_bool(opts, "userspace",
> +                                                          false);
>          const char *target = qemu_opt_get(opts, "target");
>          /* setup of chardev is deferred until they are initialised */
>          semihost_chardev = qemu_opt_get(opts, "chardev");
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 706bd7cff79..3593f1d7821 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1822,7 +1822,7 @@ static void qemu_apply_machine_options(QDict *qdict)
>  {
>      object_set_properties_from_keyval(OBJECT(current_machine), qdict, false, &error_fatal);
>
> -    if (semihosting_enabled() && !semihosting_get_argc()) {
> +    if (semihosting_enabled(false) && !semihosting_get_argc()) {
>          /* fall back to the -kernel/-append */
>          semihosting_arg_fallback(current_machine->kernel_filename, current_machine->kernel_cmdline);
>      }
> diff --git a/stubs/semihost.c b/stubs/semihost.c
> index f486651afbb..d65c9fd5dcf 100644
> --- a/stubs/semihost.c
> +++ b/stubs/semihost.c
> @@ -23,7 +23,7 @@ QemuOptsList qemu_semihosting_config_opts = {
>  };
>
>  /* Queries to config status default to off */
> -bool semihosting_enabled(void)
> +bool semihosting_enabled(bool is_user)
>  {
>      return false;
>  }
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 163df8c6157..3decc8da573 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -2219,7 +2219,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>           * it is required for halting debug disabled: it will UNDEF.
>           * Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction.
>           */
> -        if (semihosting_enabled() && imm16 == 0xf000) {
> +        if (semihosting_enabled(false) && imm16 == 0xf000) {
>  #ifndef CONFIG_USER_ONLY
>              /* In system mode, don't allow userspace access to semihosting,
>               * to provide some semblance of security (and for consistency
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ad617b99481..b85be8a818d 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1169,7 +1169,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
>       * semihosting, to provide some semblance of security
>       * (and for consistency with our 32-bit semihosting).
>       */
> -    if (semihosting_enabled() &&
> +    if (semihosting_enabled(false) &&
>  #ifndef CONFIG_USER_ONLY
>          s->current_el != 0 &&
>  #endif
> @@ -6556,7 +6556,7 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
>      /* BKPT is OK with ECI set and leaves it untouched */
>      s->eci_handled = true;
>      if (arm_dc_feature(s, ARM_FEATURE_M) &&
> -        semihosting_enabled() &&
> +        semihosting_enabled(false) &&
>  #ifndef CONFIG_USER_ONLY
>          !IS_USER(s) &&
>  #endif
> @@ -8764,7 +8764,7 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a)
>  {
>      const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
>
> -    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() &&
> +    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled(false) &&
>  #ifndef CONFIG_USER_ONLY
>          !IS_USER(s) &&
>  #endif
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index d9937ca8dc5..4b3dfec1306 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -203,7 +203,7 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw)
>              cf_rte(env);
>              return;
>          case EXCP_HALT_INSN:
> -            if (semihosting_enabled()
> +            if (semihosting_enabled(false)
>                      && (env->sr & SR_S) != 0
>                      && (env->pc & 3) == 0
>                      && cpu_lduw_code(env, env->pc - 4) == 0x4e71
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 3a037a68cc4..2b556683422 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -818,7 +818,7 @@ static void gen_break(DisasContext *dc, uint32_t code, uint32_t flags)
>  #ifndef CONFIG_USER_ONLY
>      /* The semihosting instruction is "break 1".  */
>      R_TYPE(instr, code);
> -    if (semihosting_enabled() && instr.imm5 == 1) {
> +    if (semihosting_enabled(false) && instr.imm5 == 1) {
>          t_gen_helper_raise_exception(dc, EXCP_SEMIHOST);
>          return;
>      }
> diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> index 70e11eeb459..dc475a4274b 100644
> --- a/target/xtensa/translate.c
> +++ b/target/xtensa/translate.c
> @@ -2364,9 +2364,9 @@ static uint32_t test_exceptions_simcall(DisasContext *dc,
>      bool ill = true;
>  #else
>      /* Between RE.2 and RE.3 simcall opcode's become nop for the hardware. */
> -    bool ill = dc->config->hw_version <= 250002 && !semihosting_enabled();
> +    bool ill = dc->config->hw_version <= 250002 && !semihosting_enabled(false);
>  #endif
> -    if (ill || !semihosting_enabled()) {
> +    if (ill || !semihosting_enabled(false)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "SIMCALL but semihosting is disabled\n");
>      }
>      return ill ? XTENSA_OP_ILL : 0;
> @@ -2376,7 +2376,7 @@ static void translate_simcall(DisasContext *dc, const OpcodeArg arg[],
>                                const uint32_t par[])
>  {
>  #ifndef CONFIG_USER_ONLY
> -    if (semihosting_enabled()) {
> +    if (semihosting_enabled(false)) {
>          gen_helper_simcall(cpu_env);
>      }
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3f23a42fa87..4e7111abe3d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4614,12 +4614,12 @@ SRST
>      information about the facilities this enables.
>  ERST
>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
> -    "-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
> +    "-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,userspace=on|off][,arg=str[,...]]\n" \
>      "                semihosting configuration\n",
>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA |
>  QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2 | QEMU_ARCH_RISCV)
>  SRST
> -``-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]``
> +``-semihosting-config [enable=on|off][,target=native|gdb|auto][,chardev=id][,userspace=on|off][,arg=str[,...]]``
>      Enable and configure semihosting (ARM, M68K, Xtensa, MIPS, Nios II, RISC-V
>      only).
>
> @@ -4646,6 +4646,13 @@ SRST
>          Send the output to a chardev backend output for native or auto
>          output when not in gdb
>
> +    ``userspace=on|off``
> +        Allows code running in guest userspace to access the semihosting
> +        interface. The default is that only privileged guest code can
> +        make semihosting calls. Note that setting ``userspace=on`` should
> +        only be used if all guest code is trusted (for example, in
> +        bare-metal test case code).
> +
>      ``arg=str1,arg=str2,...``
>          Allows the user to pass input arguments, and can be used
>          multiple times to build up a list. The old-style
> --
> 2.25.1
>
>

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

[Alistair Francis]

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

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

[Peter Maydell]

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

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

[Alistair Francis]

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

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

[Peter Maydell]

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