[Qemu-devel] [PATCH v4 0/2] semihosting: clean up and add --semihosting-config arg

[Qemu-devel] [PATCH v4 0/2] semihosting: clean up and add --semihosting-config arg

[Leon Alrae]

Hi,

This patch series adds "arg=" sub-option to --semihosting-config group. It
allows building up a list of input arguments as it can appear multiple
times in the command line. This is a flexible solution for creating
argc/argv for the guest program (needed by UHI semihosting for example).
RFC patch and related discussion was here:
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00115.html

It also contains some generic code clean up -- all semihosting related
things were moved to vl.c (where they are actually set) and grouped in
the SemihostingConfig structure. They can be accessed via
include/exec/semihost.h introduced in this patch series.

Since this touches generic semihosting code I'm sending it as a separate
patchset from MIPS-specific UHI semihosting patches.

Regards,
Leon

v4:
* add semihosting_get_cmdline() and update arm-semi.c to support new option
* for backward compatibility use -kernel/-append to initialize semihosting.argv
* update qemu doc to describe the interaction between arg and -kernel/-append

v3:
* improved documentation (rephrased and used @table so that generated
  doc looks nicer)

v2:
* squash clean-up related patches so renaming is not required (these
  modifications are relatively simple anyway).

Leon Alrae (2):
  semihosting: create SemihostingConfig structure and semihost.h
  semihosting: add --semihosting-config arg sub-argument

 gdbstub.c                 |   8 ++--
 include/exec/gdbstub.h    |   6 ---
 include/exec/semihost.h   |  62 ++++++++++++++++++++++++++++
 include/sysemu/sysemu.h   |   1 -
 qemu-options.hx           |  21 +++++++---
 target-arm/arm-semi.c     |  10 ++---
 target-arm/helper.c       |   7 ++--
 target-lm32/helper.c      |   3 +-
 target-m68k/op_helper.c   |   5 +--
 target-xtensa/translate.c |   3 +-
 vl.c                      | 102 ++++++++++++++++++++++++++++++++++++++++++----
 11 files changed, 188 insertions(+), 40 deletions(-)
 create mode 100644 include/exec/semihost.h

[Qemu-devel] [PATCH v4 1/2] semihosting: create SemihostingConfig structure and semihost.h

[Leon Alrae]

Remove semihosting_enabled and semihosting_target and replace them with
SemihostingConfig structure containing equivalent fields. The structure
is defined in vl.c where it is actually set.

Also introduce separate header file include/exec/semihost.h allowing to
access semihosting config related stuff from target specific semihosting
code.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 gdbstub.c                 |  8 ++++----
 include/exec/gdbstub.h    |  6 ------
 include/exec/semihost.h   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h   |  1 -
 target-arm/helper.c       |  7 ++++---
 target-lm32/helper.c      |  3 ++-
 target-m68k/op_helper.c   |  5 ++---
 target-xtensa/translate.c |  3 ++-
 vl.c                      | 38 +++++++++++++++++++++++++++++---------
 9 files changed, 87 insertions(+), 28 deletions(-)
 create mode 100644 include/exec/semihost.h

diff --git a/gdbstub.c b/gdbstub.c
index 8abcb8a..9931d81 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -40,6 +40,7 @@
 #include "cpu.h"
 #include "qemu/sockets.h"
 #include "sysemu/kvm.h"
+#include "exec/semihost.h"
 
 static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
                                          uint8_t *buf, int len, bool is_write)
@@ -317,8 +318,6 @@ static GDBState *gdbserver_state;
 
 bool gdb_has_xml;
 
-int semihosting_target = SEMIHOSTING_TARGET_AUTO;
-
 #ifdef CONFIG_USER_ONLY
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
@@ -356,10 +355,11 @@ static enum {
 /* Decide if either remote gdb syscalls or native file IO should be used. */
 int use_gdb_syscalls(void)
 {
-    if (semihosting_target == SEMIHOSTING_TARGET_NATIVE) {
+    SemihostingTarget target = semihosting_get_target();
+    if (target == SEMIHOSTING_TARGET_NATIVE) {
         /* -semihosting-config target=native */
         return false;
-    } else if (semihosting_target == SEMIHOSTING_TARGET_GDB) {
+    } else if (target == SEMIHOSTING_TARGET_GDB) {
         /* -semihosting-config target=gdb */
         return true;
     }
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index c633248..a608a26 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -95,10 +95,4 @@ extern bool gdb_has_xml;
 /* in gdbstub-xml.c, generated by scripts/feature_to_c.sh */
 extern const char *const xml_builtin[][2];
 
-/* Command line option defining whether semihosting should go via gdb or not */
-extern int semihosting_target;
-#define SEMIHOSTING_TARGET_AUTO     0
-#define SEMIHOSTING_TARGET_NATIVE   1
-#define SEMIHOSTING_TARGET_GDB      2
-
 #endif
diff --git a/include/exec/semihost.h b/include/exec/semihost.h
new file mode 100644
index 0000000..c2f0bcb
--- /dev/null
+++ b/include/exec/semihost.h
@@ -0,0 +1,44 @@
+/*
+ * Semihosting support
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef SEMIHOST_H
+#define SEMIHOST_H
+
+typedef enum SemihostingTarget {
+    SEMIHOSTING_TARGET_AUTO = 0,
+    SEMIHOSTING_TARGET_NATIVE,
+    SEMIHOSTING_TARGET_GDB
+} SemihostingTarget;
+
+#ifdef CONFIG_USER_ONLY
+static inline bool semihosting_enabled(void)
+{
+    return true;
+}
+
+static inline SemihostingTarget semihosting_get_target(void)
+{
+    return SEMIHOSTING_TARGET_AUTO;
+}
+#else
+bool semihosting_enabled(void);
+SemihostingTarget semihosting_get_target(void);
+#endif
+
+#endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8a52934..bf552a7 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -125,7 +125,6 @@ extern int cursor_hide;
 extern int graphic_rotate;
 extern int no_quit;
 extern int no_shutdown;
-extern int semihosting_enabled;
 extern int old_param;
 extern int boot_menu;
 extern bool boot_strict;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5d0f011..188a356 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -10,6 +10,7 @@
 #include "exec/cpu_ldst.h"
 #include "arm_ldst.h"
 #include <zlib.h> /* For crc32 */
+#include "exec/semihost.h"
 
 #ifndef CONFIG_USER_ONLY
 static inline int get_phys_addr(CPUARMState *env, target_ulong address,
@@ -4403,7 +4404,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
         return;
     case EXCP_BKPT:
-        if (semihosting_enabled) {
+        if (semihosting_enabled()) {
             int nr;
             nr = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
             if (nr == 0xab) {
@@ -4715,7 +4716,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
             offset = 4;
         break;
     case EXCP_SWI:
-        if (semihosting_enabled) {
+        if (semihosting_enabled()) {
             /* Check for semihosting interrupt.  */
             if (env->thumb) {
                 mask = arm_lduw_code(env, env->regs[15] - 2, env->bswap_code)
@@ -4742,7 +4743,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
         break;
     case EXCP_BKPT:
         /* See if this is a semihosting syscall.  */
-        if (env->thumb && semihosting_enabled) {
+        if (env->thumb && semihosting_enabled()) {
             mask = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
             if (mask == 0xab
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index 7a41f29..a88aa5a 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -20,6 +20,7 @@
 #include "cpu.h"
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
+#include "exec/semihost.h"
 
 int lm32_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
                               int mmu_idx)
@@ -162,7 +163,7 @@ void lm32_cpu_do_interrupt(CPUState *cs)
 
     switch (cs->exception_index) {
     case EXCP_SYSTEMCALL:
-        if (unlikely(semihosting_enabled)) {
+        if (unlikely(semihosting_enabled())) {
             /* do_semicall() returns true if call was handled. Otherwise
              * do the normal exception handling. */
             if (lm32_cpu_do_semihosting(cs)) {
diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
index 06661f5..4f8fabb 100644
--- a/target-m68k/op_helper.c
+++ b/target-m68k/op_helper.c
@@ -19,6 +19,7 @@
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/cpu_ldst.h"
+#include "exec/semihost.h"
 
 #if defined(CONFIG_USER_ONLY)
 
@@ -33,8 +34,6 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
 
 #else
 
-extern int semihosting_enabled;
-
 /* Try to fill the TLB and return an exception if error. If retaddr is
    NULL, it means that the function was called in C code (i.e. not
    from generated code or from helper.c) */
@@ -85,7 +84,7 @@ static void do_interrupt_all(CPUM68KState *env, int is_hw)
             do_rte(env);
             return;
         case EXCP_HALT_INSN:
-            if (semihosting_enabled
+            if (semihosting_enabled()
                     && (env->sr & SR_S) != 0
                     && (env->pc & 3) == 0
                     && cpu_lduw_code(env, env->pc - 4) == 0x4e71
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index 6e5096c..3d52079 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -37,6 +37,7 @@
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
 #include "exec/cpu_ldst.h"
+#include "exec/semihost.h"
 
 #include "exec/helper-proto.h"
 #include "exec/helper-gen.h"
@@ -1216,7 +1217,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
                         break;
 
                     case 1: /*SIMCALL*/
-                        if (semihosting_enabled) {
+                        if (semihosting_enabled()) {
                             if (gen_check_privilege(dc)) {
                                 gen_helper_simcall(cpu_env);
                             }
diff --git a/vl.c b/vl.c
index 15bccc4..f3319a9 100644
--- a/vl.c
+++ b/vl.c
@@ -119,6 +119,7 @@ int main(int argc, char **argv)
 #include "qapi/opts-visitor.h"
 #include "qom/object_interfaces.h"
 #include "qapi-event.h"
+#include "exec/semihost.h"
 
 #define DEFAULT_RAM_SIZE 128
 
@@ -171,7 +172,6 @@ int graphic_rotate = 0;
 const char *watchdog;
 QEMUOptionRom option_rom[MAX_OPTION_ROMS];
 int nb_option_roms;
-int semihosting_enabled = 0;
 int old_param = 0;
 const char *qemu_name;
 int alt_grab = 0;
@@ -1225,6 +1225,26 @@ static void configure_msg(QemuOpts *opts)
 }
 
 /***********************************************************/
+/* Semihosting */
+
+typedef struct SemihostingConfig {
+    bool enabled;
+    SemihostingTarget target;
+} SemihostingConfig;
+
+static SemihostingConfig semihosting;
+
+bool semihosting_enabled(void)
+{
+    return semihosting.enabled;
+}
+
+SemihostingTarget semihosting_get_target(void)
+{
+    return semihosting.target;
+}
+
+/***********************************************************/
 /* USB devices */
 
 static int usb_device_add(const char *devname)
@@ -3545,24 +3565,24 @@ int main(int argc, char **argv, char **envp)
                 nb_option_roms++;
                 break;
             case QEMU_OPTION_semihosting:
-                semihosting_enabled = 1;
-                semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                semihosting.enabled = true;
+                semihosting.target = SEMIHOSTING_TARGET_AUTO;
                 break;
             case QEMU_OPTION_semihosting_config:
-                semihosting_enabled = 1;
+                semihosting.enabled = true;
                 opts = qemu_opts_parse(qemu_find_opts("semihosting-config"),
                                            optarg, 0);
                 if (opts != NULL) {
-                    semihosting_enabled = qemu_opt_get_bool(opts, "enable",
+                    semihosting.enabled = qemu_opt_get_bool(opts, "enable",
                                                             true);
                     const char *target = qemu_opt_get(opts, "target");
                     if (target != NULL) {
                         if (strcmp("native", target) == 0) {
-                            semihosting_target = SEMIHOSTING_TARGET_NATIVE;
+                            semihosting.target = SEMIHOSTING_TARGET_NATIVE;
                         } else if (strcmp("gdb", target) == 0) {
-                            semihosting_target = SEMIHOSTING_TARGET_GDB;
+                            semihosting.target = SEMIHOSTING_TARGET_GDB;
                         } else  if (strcmp("auto", target) == 0) {
-                            semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                            semihosting.target = SEMIHOSTING_TARGET_AUTO;
                         } else {
                             fprintf(stderr, "Unsupported semihosting-config"
                                     " %s\n",
@@ -3570,7 +3590,7 @@ int main(int argc, char **argv, char **envp)
                             exit(1);
                         }
                     } else {
-                        semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                        semihosting.target = SEMIHOSTING_TARGET_AUTO;
                     }
                 } else {
                     fprintf(stderr, "Unsupported semihosting-config %s\n",

[Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Leon Alrae]

Add new "arg" sub-argument to the --semihosting-config allowing the user
to pass multiple input arguments separately. It is required for example
by UHI semihosting to construct argc and argv.

Also, update ARM semihosting to support new option (at the moment it is
the only target which cares about arguments).

If the semihosting is enabled and no semihosting args have been specified,
then fall back to -kernel/-append. The -append string is split on whitespace
before initializing semihosting.argv[1..n]; this is different from what
QEMU MIPS machines' pseudo-bootloaders do (i.e. argv[1] contains the whole
-append), but is more intuitive from UHI user's point of view and Linux
kernel just does not care as it concatenates argv[1..n] into single cmdline
string anyway.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 include/exec/semihost.h | 18 ++++++++++++++
 qemu-options.hx         | 21 ++++++++++++----
 target-arm/arm-semi.c   | 10 +++-----
 vl.c                    | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 12 deletions(-)

diff --git a/include/exec/semihost.h b/include/exec/semihost.h
index c2f0bcb..5980939 100644
--- a/include/exec/semihost.h
+++ b/include/exec/semihost.h
@@ -36,9 +36,27 @@ static inline SemihostingTarget semihosting_get_target(void)
 {
     return SEMIHOSTING_TARGET_AUTO;
 }
+
+static inline const char *semihosting_get_arg(int i)
+{
+    return NULL;
+}
+
+static inline int semihosting_get_argc(void)
+{
+    return 0;
+}
+
+static inline const char *semihosting_get_cmdline(void)
+{
+    return NULL;
+}
 #else
 bool semihosting_enabled(void);
 SemihostingTarget semihosting_get_target(void);
+const char *semihosting_get_arg(int i);
+int semihosting_get_argc(void);
+const char *semihosting_get_cmdline(void);
 #endif
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index ec356f6..90500ad 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3296,14 +3296,25 @@ STEXI
 Enable semihosting mode (ARM, M68K, Xtensa only).
 ETEXI
 DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
-    "-semihosting-config [enable=on|off,]target=native|gdb|auto   semihosting configuration\n",
+    "-semihosting-config [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
+    "                semihosting configuration\n",
 QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32)
 STEXI
-@item -semihosting-config [enable=on|off,]target=native|gdb|auto
+@item -semihosting-config [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]
 @findex -semihosting-config
-Enable semihosting and define where the semihosting calls will be addressed,
-to QEMU (@code{native}) or to GDB (@code{gdb}). The default is @code{auto}, which means
-@code{gdb} during debug sessions and @code{native} otherwise (ARM, M68K, Xtensa only).
+Enable and configure semihosting (ARM, M68K, Xtensa only).
+@table @option
+@item target=@code{native|gdb|auto}
+Defines where the semihosting calls will be addressed, to QEMU (@code{native})
+or to GDB (@code{gdb}). The default is @code{auto}, which means @code{gdb}
+during debug sessions and @code{native} otherwise.
+@item arg=@var{str1},arg=@var{str2},...
+Allows the user to pass input arguments, and can be used multiple times to build
+up a list. The old-style @code{-kernel}/@code{-append} method of passing a
+command line is still supported for backward compatibility. If both the
+@code{--semihosting-config arg} and the @code{-kernel}/@code{-append} are
+specified, the former is passed to semihosting as it always takes precedence.
+@end table
 ETEXI
 DEF("old-param", 0, QEMU_OPTION_old_param,
     "-old-param      old param mode\n", QEMU_ARCH_ARM)
diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
index a8b83e6..74a67e9 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -27,6 +27,7 @@
 #include <time.h>
 
 #include "cpu.h"
+#include "exec/semihost.h"
 #ifdef CONFIG_USER_ONLY
 #include "qemu.h"
 
@@ -440,10 +441,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
             input_size = arg1;
             /* Compute the size of the output string.  */
 #if !defined(CONFIG_USER_ONLY)
-            output_size = strlen(ts->boot_info->kernel_filename)
-                        + 1  /* Separating space.  */
-                        + strlen(ts->boot_info->kernel_cmdline)
-                        + 1; /* Terminating null byte.  */
+            output_size = strlen(semihosting_get_cmdline()) + 1;
 #else
             unsigned int i;
 
@@ -474,9 +472,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
 
             /* Copy the command-line arguments.  */
 #if !defined(CONFIG_USER_ONLY)
-            pstrcpy(output_buffer, output_size, ts->boot_info->kernel_filename);
-            pstrcat(output_buffer, output_size, " ");
-            pstrcat(output_buffer, output_size, ts->boot_info->kernel_cmdline);
+            pstrcpy(output_buffer, output_size, semihosting_get_cmdline());
 #else
             if (output_size == 1) {
                 /* Empty command-line.  */
diff --git a/vl.c b/vl.c
index f3319a9..d599411 100644
--- a/vl.c
+++ b/vl.c
@@ -486,6 +486,9 @@ static QemuOptsList qemu_semihosting_config_opts = {
         }, {
             .name = "target",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "arg",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -1230,6 +1233,9 @@ static void configure_msg(QemuOpts *opts)
 typedef struct SemihostingConfig {
     bool enabled;
     SemihostingTarget target;
+    const char **argv;
+    int argc;
+    const char *cmdline; /* concatenated argv */
 } SemihostingConfig;
 
 static SemihostingConfig semihosting;
@@ -1244,6 +1250,56 @@ SemihostingTarget semihosting_get_target(void)
     return semihosting.target;
 }
 
+const char *semihosting_get_arg(int i)
+{
+    if (i >= semihosting.argc) {
+        return NULL;
+    }
+    return semihosting.argv[i];
+}
+
+int semihosting_get_argc(void)
+{
+    return semihosting.argc;
+}
+
+const char *semihosting_get_cmdline(void)
+{
+    if (semihosting.cmdline == NULL && semihosting.argc > 0) {
+        semihosting.cmdline = g_strjoinv(" ", (gchar **)semihosting.argv);
+    }
+    return semihosting.cmdline;
+}
+
+static int add_semihosting_arg(const char *name, const char *val, void *opaque)
+{
+    SemihostingConfig *s = opaque;
+    if (strcmp(name, "arg") == 0) {
+        s->argc++;
+        /* one extra element as g_strjoinv() expects NULL-terminated array */
+        s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
+        s->argv[s->argc - 1] = val;
+        s->argv[s->argc] = NULL;
+    }
+    return 0;
+}
+
+/* Use strings passed via -kernel/-append to initialize semihosting.argv[] */
+static inline void semihosting_arg_fallback(const char *file, const char *cmd)
+{
+    char *cmd_token;
+
+    /* argv[0] */
+    add_semihosting_arg("arg", file, &semihosting);
+
+    /* split -append and initialize argv[1..n] */
+    cmd_token = strtok(g_strdup(cmd), " ");
+    while (cmd_token) {
+        add_semihosting_arg("arg", cmd_token, &semihosting);
+        cmd_token = strtok(NULL, " ");
+    }
+}
+
 /***********************************************************/
 /* USB devices */
 
@@ -3592,6 +3648,9 @@ int main(int argc, char **argv, char **envp)
                     } else {
                         semihosting.target = SEMIHOSTING_TARGET_AUTO;
                     }
+                    /* Set semihosting argument count and vector */
+                    qemu_opt_foreach(opts, add_semihosting_arg,
+                                     &semihosting, 0);
                 } else {
                     fprintf(stderr, "Unsupported semihosting-config %s\n",
                             optarg);
@@ -4150,6 +4209,11 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    if (semihosting_enabled() && !semihosting_get_argc() && kernel_filename) {
+        /* fall back to the -kernel/-append */
+        semihosting_arg_fallback(kernel_filename, kernel_cmdline);
+    }
+
     os_set_line_buffering();
 
 #ifdef CONFIG_SPICE

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Peter Maydell]

On 26 May 2015 at 17:03, Leon Alrae <leon.alrae@imgtec.com> wrote:
> Add new "arg" sub-argument to the --semihosting-config allowing the user
> to pass multiple input arguments separately. It is required for example
> by UHI semihosting to construct argc and argv.
>
> Also, update ARM semihosting to support new option (at the moment it is
> the only target which cares about arguments).
>
> If the semihosting is enabled and no semihosting args have been specified,
> then fall back to -kernel/-append. The -append string is split on whitespace
> before initializing semihosting.argv[1..n]; this is different from what
> QEMU MIPS machines' pseudo-bootloaders do (i.e. argv[1] contains the whole
> -append), but is more intuitive from UHI user's point of view and Linux
> kernel just does not care as it concatenates argv[1..n] into single cmdline
> string anyway.
>
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>

> --- a/target-arm/arm-semi.c
> +++ b/target-arm/arm-semi.c
> @@ -27,6 +27,7 @@
>  #include <time.h>
>
>  #include "cpu.h"
> +#include "exec/semihost.h"
>  #ifdef CONFIG_USER_ONLY
>  #include "qemu.h"
>
> @@ -440,10 +441,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
>              input_size = arg1;
>              /* Compute the size of the output string.  */
>  #if !defined(CONFIG_USER_ONLY)
> -            output_size = strlen(ts->boot_info->kernel_filename)
> -                        + 1  /* Separating space.  */
> -                        + strlen(ts->boot_info->kernel_cmdline)
> -                        + 1; /* Terminating null byte.  */
> +            output_size = strlen(semihosting_get_cmdline()) + 1;

It looks like semihosting_get_cmdline() can return NULL,
in which case this will blow up, I think.

Patch looks OK otherwise (haven't tested it yet).

-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Leon Alrae]

On 05/06/15 16:23, Peter Maydell wrote:
> On 26 May 2015 at 17:03, Leon Alrae <leon.alrae@imgtec.com> wrote:
>> --- a/target-arm/arm-semi.c
>> +++ b/target-arm/arm-semi.c
>> @@ -27,6 +27,7 @@
>>  #include <time.h>
>>
>>  #include "cpu.h"
>> +#include "exec/semihost.h"
>>  #ifdef CONFIG_USER_ONLY
>>  #include "qemu.h"
>>
>> @@ -440,10 +441,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
>>              input_size = arg1;
>>              /* Compute the size of the output string.  */
>>  #if !defined(CONFIG_USER_ONLY)
>> -            output_size = strlen(ts->boot_info->kernel_filename)
>> -                        + 1  /* Separating space.  */
>> -                        + strlen(ts->boot_info->kernel_cmdline)
>> -                        + 1; /* Terminating null byte.  */
>> +            output_size = strlen(semihosting_get_cmdline()) + 1;
> 
> It looks like semihosting_get_cmdline() can return NULL,
> in which case this will blow up, I think.

semihosting_get_cmdline() returns NULL if neither semihosting args nor
-kernel have been specified. As far as I can tell existing
implementation may also blow up if kernel_filename is NULL, so we retain
the same behaviour. Besides, it's not clear to me how the
TARGET_SYS_GET_CMDLINE should behave if cmdline is not available,
whether should return -1 or pass an empty string to the guest. For me
this looks like a separate issue, not much related to this patch series.

Thanks,
Leon

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Liviu Ionescu]

> On 05 Jun 2015, at 23:09, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> ... As far as I can tell existing
> implementation may also blow up if kernel_filename is NULL,

not necessarily, in my branch it is perfectly legal to start qemu without an image, as long as the gdb server is started, since the application will be loaded by the gdb client. (this is how the Eclipse plug-in is configured to start qemu).

> ... how the
> TARGET_SYS_GET_CMDLINE should behave if cmdline is not available,
> whether should return -1 or pass an empty string to the guest. 

for consistency I would suggest to return -1 for all cases that do not return a legal cmdline.

regards,

Liviu

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Peter Maydell]

On 5 June 2015 at 22:11, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 05 Jun 2015, at 23:09, Leon Alrae <leon.alrae@imgtec.com> wrote:
>> ... how the
>> TARGET_SYS_GET_CMDLINE should behave if cmdline is not available,
>> whether should return -1 or pass an empty string to the guest.
>
> for consistency I would suggest to return -1 for all cases that do
> not return a legal cmdline.

The existing linux-user implementation of this semihosting
call handles this case by returning the empty string, so
consistency suggests following that in the equivalent
softmmu case.

-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Liviu Ionescu]

> On 06 Jun 2015, at 01:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 5 June 2015 at 22:11, Liviu Ionescu <ilg@livius.net> wrote:
>> 
>>> On 05 Jun 2015, at 23:09, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>> ... how the
>>> TARGET_SYS_GET_CMDLINE should behave if cmdline is not available,
>>> whether should return -1 or pass an empty string to the guest.
>> 
>> for consistency I would suggest to return -1 for all cases that do
>> not return a legal cmdline.
> 
> The existing linux-user implementation of this semihosting
> call handles this case by returning the empty string, so
> consistency suggests following that in the equivalent
> softmmu case.

in this case it doesn't make any major difference, the application should accommodate both cases, but, as a general comment, if one case is broken/poorly implemented, for the sake of consistency I would not rush to make all others broken too, but I would first try to find a solution to fix/improve them all.

regards,

Liviu

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Liviu Ionescu]

would it be possible to have all the semihosting patches ready for 2.4?

regards,

Liviu

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Peter Maydell]

On 16 June 2015 at 13:32, Liviu Ionescu <ilg@livius.net> wrote:
> would it be possible to have all the semihosting patches ready for 2.4?

Yes, I agree that would be good. Is it just this 2 patch
series, or are there others too?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Leon Alrae]

On 16/06/2015 15:03, Peter Maydell wrote:
> On 16 June 2015 at 13:32, Liviu Ionescu <ilg@livius.net> wrote:
>> would it be possible to have all the semihosting patches ready for 2.4?
> 
> Yes, I agree that would be good. Is it just this 2 patch
> series, or are there others too?

Just to confirm -- the only thing required before applying this 2 patch series
is testing in ARM context by you, right?

Remaining semihosting patches I've got are MIPS specific, but they can go via
target-mips tree separately.

Leon

Re: [Qemu-devel] [PATCH v4 2/2] semihosting: add --semihosting-config arg sub-argument

[Peter Maydell]

On 16 June 2015 at 15:20, Leon Alrae <leon.alrae@imgtec.com> wrote:
> On 16/06/2015 15:03, Peter Maydell wrote:
>> On 16 June 2015 at 13:32, Liviu Ionescu <ilg@livius.net> wrote:
>>> would it be possible to have all the semihosting patches ready for 2.4?
>>
>> Yes, I agree that would be good. Is it just this 2 patch
>> series, or are there others too?
>
> Just to confirm -- the only thing required before applying this 2 patch series
> is testing in ARM context by you, right?

That, and you need to respin it, because the qemu_opt_foreach API
has changed in current master.

Something like this is probably sufficient but you should check:

index 5edfa00..278cd6a 100644
--- a/vl.c
+++ b/vl.c
@@ -1292,7 +1292,9 @@ const char *semihosting_get_cmdline(void)
     return semihosting.cmdline;
 }

-static int add_semihosting_arg(const char *name, const char *val, void *opaque)
+static int add_semihosting_arg(void *opaque,
+                               const char *name, const char *val,
+                               Error **errp)
 {
     SemihostingConfig *s = opaque;
     if (strcmp(name, "arg") == 0) {
@@ -1311,12 +1313,12 @@ static inline void
semihosting_arg_fallback(const char *file, const char *cmd)
     char *cmd_token;

     /* argv[0] */
-    add_semihosting_arg("arg", file, &semihosting);
+    add_semihosting_arg(&semihosting, "arg", file, NULL);

     /* split -append and initialize argv[1..n] */
     cmd_token = strtok(g_strdup(cmd), " ");
     while (cmd_token) {
-        add_semihosting_arg("arg", cmd_token, &semihosting);
+        add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
         cmd_token = strtok(NULL, " ");
     }
 }
@@ -3727,7 +3729,7 @@ int main(int argc, char **argv, char **envp)
                     }
                     /* Set semihosting argument count and vector */
                     qemu_opt_foreach(opts, add_semihosting_arg,
-                                     &semihosting, 0);
+                                     &semihosting, NULL);
                 } else {
                     fprintf(stderr, "Unsupported semihosting-config %s\n",
                             optarg);


thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/2] semihosting: clean up and add --semihosting-config arg

[Leon Alrae]

On 26/05/2015 17:03, Leon Alrae wrote:
> v4:
> * add semihosting_get_cmdline() and update arm-semi.c to support new option
> * for backward compatibility use -kernel/-append to initialize semihosting.argv
> * update qemu doc to describe the interaction between arg and -kernel/-append

I believe this patchset addressed all the issues raised so far concerning the
--semihosting-config arg option.

As far as semihosting_get_cmdline() goes, at the moment it's used only by ARM
semihosting. I'm not familiar with this API, but if it doesn't mandate any
specific quoting, then I think we should keep the implementation in QEMU free
from any assumptions about guest code and it should be up to the user to pass
an understandable by the guest string. Therefore semihosting_get_cmdline()
returns the same string (i.e. a.out foo "bar baz") for the following inputs:

qemu-system-arm ... -kernel a.out -append 'foo "bar baz"'
qemu-system-arm ... -semihosting-config arg='a.out foo "bar baz"'
qemu-system-arm ... -semihosting-config arg=a.out,arg=foo,arg='"bar baz"'

Do you think there is anything missing or requiring further clarification that
is stopping us from applying these changes? It would be great to sort this out
as MIPS UHI patches rely on these changes.

Thanks,
Leon