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

[Qemu-devel] [PATCH v3 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

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   | 56 +++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h   |  1 -
 qemu-options.hx           | 19 +++++++++----
 target-arm/helper.c       |  7 +++--
 target-lm32/helper.c      |  3 +-
 target-m68k/op_helper.c   |  5 ++--
 target-xtensa/translate.c |  3 +-
 vl.c                      | 71 +++++++++++++++++++++++++++++++++++++++++------
 10 files changed, 146 insertions(+), 33 deletions(-)
 create mode 100644 include/exec/semihost.h

[Qemu-devel] [PATCH v3 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 f8f8d76..f5a9897 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,
@@ -4401,7 +4402,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) {
@@ -4713,7 +4714,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)
@@ -4740,7 +4741,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 v3 2/2] semihosting: add --semihosting-config arg sub-argument

[Leon Alrae]

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

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 include/exec/semihost.h | 12 ++++++++++++
 qemu-options.hx         | 19 ++++++++++++++-----
 vl.c                    | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/include/exec/semihost.h b/include/exec/semihost.h
index c2f0bcb..6e4e8c0 100644
--- a/include/exec/semihost.h
+++ b/include/exec/semihost.h
@@ -36,9 +36,21 @@ 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;
+}
 #else
 bool semihosting_enabled(void);
 SemihostingTarget semihosting_get_target(void);
+const char *semihosting_get_arg(int i);
+int semihosting_get_argc(void);
 #endif
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index ec356f6..84ae6c2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3296,14 +3296,23 @@ 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, can be used multiple times to build up
+a list. This is a replacement for the old-style -kernel/-append method of
+passing a command line to semihosting.
+@end table
 ETEXI
 DEF("old-param", 0, QEMU_OPTION_old_param,
     "-old-param      old param mode\n", QEMU_ARCH_ARM)
diff --git a/vl.c b/vl.c
index f3319a9..c8ac1e6 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,8 @@ static void configure_msg(QemuOpts *opts)
 typedef struct SemihostingConfig {
     bool enabled;
     SemihostingTarget target;
+    const char **argv;
+    int argc;
 } SemihostingConfig;
 
 static SemihostingConfig semihosting;
@@ -1244,6 +1249,31 @@ 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;
+}
+
+static int add_semihosting_arg(const char *name, const char *val, void *opaque)
+{
+    SemihostingConfig *s = opaque;
+    if (strcmp(name, "arg") == 0) {
+        s->argc++;
+        s->argv = g_realloc(s->argv, s->argc * sizeof(void *));
+        s->argv[s->argc - 1] = val;
+    }
+    return 0;
+}
+
 /***********************************************************/
 /* USB devices */
 
@@ -3592,6 +3622,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);

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

[Peter Maydell]

On 8 May 2015 at 12:41, Leon Alrae <leon.alrae@imgtec.com> wrote:
> Add new "arg" sub-argument to the --semihosting-config allowing to pass
> multiple input argument separately. It is required for example by UHI
> semihosting to construct argc and argv.
>
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> ---
>  include/exec/semihost.h | 12 ++++++++++++
>  qemu-options.hx         | 19 ++++++++++++++-----
>  vl.c                    | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/include/exec/semihost.h b/include/exec/semihost.h
> index c2f0bcb..6e4e8c0 100644
> --- a/include/exec/semihost.h
> +++ b/include/exec/semihost.h
> @@ -36,9 +36,21 @@ 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;
> +}
>  #else
>  bool semihosting_enabled(void);
>  SemihostingTarget semihosting_get_target(void);
> +const char *semihosting_get_arg(int i);
> +int semihosting_get_argc(void);
>  #endif
>
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ec356f6..84ae6c2 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3296,14 +3296,23 @@ 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, can be used multiple times to build up
> +a list. This is a replacement for the old-style -kernel/-append method of
> +passing a command line to semihosting.
> +@end table

You need to say how this interacts with the -kernel/-append option
(ie what happens if you specify both). Also, you haven't actually
changed anything so at the moment -arg doesn't do anything.

The only semihosting target in-tree which cares about arguments
at the moment is ARM.

Ideally semihost.h's "get me the command line" function(s) should
handle "use -arg if present, fall back to -kernel + -append if not"
so the target specific semihosting code doesn't need to care.

thanks
-- PMM

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

[Leon Alrae]

On 18/05/2015 17:18, Peter Maydell wrote:
> On 8 May 2015 at 12:41, Leon Alrae <leon.alrae@imgtec.com> wrote:
>> Add new "arg" sub-argument to the --semihosting-config allowing to pass
>> multiple input argument separately. It is required for example by UHI
>> semihosting to construct argc and argv.
>>
>> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
>> ---
>>  include/exec/semihost.h | 12 ++++++++++++
>>  qemu-options.hx         | 19 ++++++++++++++-----
>>  vl.c                    | 33 +++++++++++++++++++++++++++++++++
>>  3 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/exec/semihost.h b/include/exec/semihost.h
>> index c2f0bcb..6e4e8c0 100644
>> --- a/include/exec/semihost.h
>> +++ b/include/exec/semihost.h
>> @@ -36,9 +36,21 @@ 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;
>> +}
>>  #else
>>  bool semihosting_enabled(void);
>>  SemihostingTarget semihosting_get_target(void);
>> +const char *semihosting_get_arg(int i);
>> +int semihosting_get_argc(void);
>>  #endif
>>
>>  #endif
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index ec356f6..84ae6c2 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3296,14 +3296,23 @@ 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, can be used multiple times to build up
>> +a list. This is a replacement for the old-style -kernel/-append method of
>> +passing a command line to semihosting.
>> +@end table
> 
> You need to say how this interacts with the -kernel/-append option
> (ie what happens if you specify both).

I don't see any correlation between semihosting options and "-append"
which is described as "kernel command line". I know that ARM semihosting
uses it as cmdline, therefore I'd wanted to leave the meaning of
"-append" option specific to a target semihosting implementation. But
perhaps specifying it globally would be better here, I can add something
like "If both are specified, -kernel/-append are ignored (-kernel is
used to load an image, but the path won't be passed to semihosting)" if
we are happy with that.

> Also, you haven't actually changed anything so at the moment -arg doesn't do
> anything.

Yes, but is that an issue in this case? Commit message explains why -arg
is required and UHI patches are already on the mailing list.

> The only semihosting target in-tree which cares about arguments
> at the moment is ARM.

I left it untouched as I'm aware that Liviu has some more ARM
semihosting stuff pending.

> Ideally semihost.h's "get me the command line" function(s) should
> handle "use -arg if present, fall back to -kernel + -append if not"
> so the target specific semihosting code doesn't need to care.

Agreed. We could initialize semihosting.argv[0] with -kernel and argv[1]
with -append string if in semihosting mode and no semihosting args have
been specified. I'll update it in v4.

Thanks,
Leon

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

[Liviu Ionescu]

> On 20 May 2015, at 11:11, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> like "If both are specified, -kernel/-append are ignored (-kernel is
> used to load an image, but the path won't be passed to semihosting)" 
...
> We could initialize semihosting.argv[0] with -kernel


here you have a small contradiction, both in the current ARM implementation, and in your proposal, the full kernel path is passed as argv[0].

perhaps you could rephrase the text for the manual. the idea to transmit is that, if specified, --semihosting-config always takes precedence; -kernel/-append have a different purpose and although, for compatibility reasons, they can still be used to pass semihosting args, their use is deprecated in favour of --semihosting-config.

regards,

Liviu

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

[Leon Alrae]

On 20/05/2015 09:30, Liviu Ionescu wrote:
> 
>> On 20 May 2015, at 11:11, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>
>> like "If both are specified, -kernel/-append are ignored (-kernel is
>> used to load an image, but the path won't be passed to semihosting)" 
> ...
>> We could initialize semihosting.argv[0] with -kernel
> 
> 
> here you have a small contradiction, both in the current ARM implementation, and in your proposal, the full kernel path is passed as argv[0].

These are two different cases:
1) If both "--semihosting-config arg" and -kernel/-append are specified
in QEMU command line, then -kernel/-append is ignored and
semihosting.argv[0] is set with first arg string instead of -kernel
path, arg[1] with second arg, and so on.
2) If we are in semihosting mode but no "--semihosting-config arg" were
specified then argv[0] is set with full path from -kernel, and argv[1]
is set with -append. This is compatible with what ARM is currently doing
I believe.

The following example would support above two cases arm-semi.c:

-            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);
+            if (semihosting_get_argc()) {
+                pstrcat(output_buffer, output_size,
semihosting_get_arg(0));
+                for (i = 1; i < semihosting_get_argc(); i++) {
+                    pstrcat(output_buffer, output_size, " ");
+                    pstrcat(output_buffer, output_size,
semihosting_get_arg(i));
+                }
+            }

Leon

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

[Liviu Ionescu]

> On 20 May 2015, at 11:51, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> On 20/05/2015 09:30, Liviu Ionescu wrote:
>> 
>>> On 20 May 2015, at 11:11, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>> 
>>> like "If both are specified, -kernel/-append are ignored (-kernel is
>>> used to load an image, but the path won't be passed to semihosting)" 
>> ...
>>> We could initialize semihosting.argv[0] with -kernel
>> 
>> 
>> here you have a small contradiction, both in the current ARM implementation, and in your proposal, the full kernel path is passed as argv[0].
> 
> These are two different cases: ...

yes, that's correct, my comment was related to the above quotes from your message, in the first part you mention "the path won't be passed to semihosting", which is not consistent with the actual behaviour, since the entire kernel path is passed unchanged.

if you update the manual text, please rephrase to make the text easier to understand and remove this notice related to no-path kernel.

regards,

Liviu

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

[Leon Alrae]

On 20/05/2015 12:12, Liviu Ionescu wrote:
> yes, that's correct, my comment was related to the above quotes from your message, in the first part you mention "the path won't be passed to semihosting", which is not consistent with the actual behaviour, since the entire kernel path is passed unchanged.

Ah, I see. Yes, that's a good point; I'm going to address this in v4.

> 
> if you update the manual text, please rephrase to make the text easier to understand and remove this notice related to no-path kernel.

I rephrased it a bit (but please remember that "easier to understand" is
very subjective):

"Allows the user to pass input arguments, and can be used multiple times
to build up a list. The old-style -kernel/-append method of passing a
command line is still supported for backward compatibility. If both the
--semihosting-config arg and the -kernel/-append are specified, the
former is passed to semihosting as it always takes precedence."

Thanks,
Leon

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

[Peter Maydell]

On 20 May 2015 at 09:11, Leon Alrae <leon.alrae@imgtec.com> wrote:
> On 18/05/2015 17:18, Peter Maydell wrote:
>> You need to say how this interacts with the -kernel/-append option
>> (ie what happens if you specify both).
>
> I don't see any correlation between semihosting options and "-append"
> which is described as "kernel command line". I know that ARM semihosting
> uses it as cmdline, therefore I'd wanted to leave the meaning of
> "-append" option specific to a target semihosting implementation.

I really don't want to introduce arguments which differ in semantics
depending on the target CPU type. At the moment 100% of our CPUs
which have a semihosting API that includes a command line argument
honour -kernel/-append.

> But
> perhaps specifying it globally would be better here, I can add something
> like "If both are specified, -kernel/-append are ignored (-kernel is
> used to load an image, but the path won't be passed to semihosting)" if
> we are happy with that.

Yes, that's the behaviour we want.

>> Also, you haven't actually changed anything so at the moment -arg doesn't do
>> anything.
>
> Yes, but is that an issue in this case? Commit message explains why -arg
> is required and UHI patches are already on the mailing list.

Well, with these patches alone we end up with a documented but broken
option. For reasons of consistency (again) I don't want the new
option in the tree without it being supported by ARM, even if the
MIPS code that uses it goes in.

>> Ideally semihost.h's "get me the command line" function(s) should
>> handle "use -arg if present, fall back to -kernel + -append if not"
>> so the target specific semihosting code doesn't need to care.
>
> Agreed. We could initialize semihosting.argv[0] with -kernel and argv[1]
> with -append string if in semihosting mode and no semihosting args have
> been specified. I'll update it in v4.

I'm tempted to suggest splitting -append on whitespace to initialize
argv[1..n]. ARM will just re-concatenate it, but it seems more
useful for the non-ARM cases.

-- PMM

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

[Leon Alrae]

On 20/05/2015 09:54, Peter Maydell wrote:
> Well, with these patches alone we end up with a documented but broken
> option. For reasons of consistency (again) I don't want the new
> option in the tree without it being supported by ARM, even if the
> MIPS code that uses it goes in.

Fair enough. I’ll update arm-semi.c as well to concatenate all args.

Leon

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

[Liviu Ionescu]

> On 20 May 2015, at 12:31, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> ... I’ll update arm-semi.c as well to concatenate all args.

you can use parts of my initial patch for the arm semihosting related changes.

---

personally I would add a pointer (cmdline?) in the semihosting structure and a getter (semihosting_get_cmdline()?). at first call, the getter would concatenate all args and store the result in the structure (the concatenate function is there), so that future calls will get it faster.

this is necessary since in my code I need the cmdline string in at least two different locations.


regards,

Liviu

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

[Peter Maydell]

On 20 May 2015 at 14:49, Liviu Ionescu <ilg@livius.net> wrote:
> personally I would add a pointer (cmdline?) in the semihosting
> structure and a getter (semihosting_get_cmdline()?). at first call,
> the getter would concatenate all args and store the result in the
> structure (the concatenate function is there), so that future calls
> will get it faster.

Seems worth having the core code provide the "give me commandline
as a single string" functionality, certainly.

PS: the glib function g_strjoinv("", null_terminated_string_array)
is probably better than reimplementing the string-concatenation
wheel.

-- PMM

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

[Liviu Ionescu]

> On 20 May 2015, at 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> PS: the glib function g_strjoinv("", null_terminated_string_array)
> is probably better than reimplementing the string-concatenation
> wheel.

I'm not familiar with g_strjoinv(), but the args concatenate logic should protect args containing spaces with quotes or apostrophes. my concatenate_semihosting_cmdline() does this and I guess it can be used as a starting point if no glib functions providing this functionality are available.


regards,

Liviu

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

[Peter Maydell]

On 20 May 2015 at 15:31, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 20 May 2015, at 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> PS: the glib function g_strjoinv("", null_terminated_string_array)
>> is probably better than reimplementing the string-concatenation
>> wheel.
>
> I'm not familiar with g_strjoinv(), but the args concatenate logic
> should protect args containing spaces with quotes or apostrophes.

It should just concatenate the arguments, putting spaces between them.
The ARM semihosting API doesn't mandate any kind of quoting, and we
can't know what the guest application expects, so the user is going
to have to supply any necessary quotes.

-- PMM

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

[Liviu Ionescu]

> On 20 May 2015, at 17:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> ... The ARM semihosting API doesn't mandate any kind of quoting, and we
> can't know what the guest application expects,

ok (btw, my embedded semihosting code is happy with both quotes or apostrophes)
 
> so the user is going
> to have to supply any necessary quotes.

could you exemplify how the qemu syntax would look like for passing something functionally equivalent to the shell syntax [program --opt1 "path 1" --opt2 "path 2"]? Is the syntax identical for both UHI and ARM?


regards,

Liviu

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

[Peter Maydell]

On 20 May 2015 at 15:59, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 20 May 2015, at 17:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> ... The ARM semihosting API doesn't mandate any kind of quoting, and we
>> can't know what the guest application expects,
>
> ok (btw, my embedded semihosting code is happy with both quotes or apostrophes)
>
>> so the user is going
>> to have to supply any necessary quotes.
>
> could you exemplify how the qemu syntax would look like for passing
> something functionally equivalent to the shell syntax
> [program --opt1 "path 1" --opt2 "path 2"]?

That depends entirely on how the guest program chooses to parse
its command line string. Semihosting doesn't have any mandated
quoting. So you just want to do something like

  --semihosting-config,arg=commandline

where commandline is the command line you want the app to
receive, with:
 * any escaping that the shell you ran QEMU via requires
 * the comma-escaping that QEMU's --arggroup,opt=value,opt=value
   syntax requires for values

In the example above, assuming a POSIX-shell style quoting
and that the guest splits its command line string honouring
double-quotes:
 --semihosting-config,arg='program --opt1 "path 1" --opt2 "path 2"'

UHI's API apparently passes arguments individually to the
guest binary, so you would want to use multiple arg=.

-- PMM

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

[Liviu Ionescu]

> On 20 May 2015, at 18:11, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> --semihosting-config,arg='program --opt1 "path 1" --opt2 "path 2"'

ok, thank you, this seems manageable.

Liviu

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

[Leon Alrae]

On 20/05/2015 09:54, Peter Maydell wrote:
> On 20 May 2015 at 09:11, Leon Alrae <leon.alrae@imgtec.com> wrote:
>> Agreed. We could initialize semihosting.argv[0] with -kernel and argv[1]
>> with -append string if in semihosting mode and no semihosting args have
>> been specified. I'll update it in v4.
> 
> I'm tempted to suggest splitting -append on whitespace to initialize
> argv[1..n]. ARM will just re-concatenate it, but it seems more
> useful for the non-ARM cases.

I've been considering that, and I think we are better off without
splitting -append in semihosting mode because this is at least
consistent with MIPS machines in QEMU (in non-semihosting mode) --
pseudo-bootloaders in MALTA and Fulong2e have hardcoded argc to 2
(-kernel and -append). Therefore I would leave it as it is unless we
really need to change it.

Leon

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

[Peter Maydell]

On 21 May 2015 at 14:57, Leon Alrae <leon.alrae@imgtec.com> wrote:
> I've been considering that, and I think we are better off without
> splitting -append in semihosting mode because this is at least
> consistent with MIPS machines in QEMU (in non-semihosting mode) --
> pseudo-bootloaders in MALTA and Fulong2e have hardcoded argc to 2
> (-kernel and -append). Therefore I would leave it as it is unless we
> really need to change it.

Well, it will presumably mean that on a MIPS semihosting case
if the user tries -append "some command line" it won't do what
they expect...

-- PMM

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

[Leon Alrae]

On 21/05/2015 15:01, Peter Maydell wrote:
> Well, it will presumably mean that on a MIPS semihosting case
> if the user tries -append "some command line" it won't do what
> they expect...

Yes, but I think the user should use semihosting-config arg, which is
more natural in semihosting context, rather than -append (which is
described as "kernel command line" in qemu manual).

Leon

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

[Liviu Ionescu]

> On 21 May 2015, at 16:57, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> ... pseudo-bootloaders in MALTA and Fulong2e have hardcoded argc to 2
> (-kernel and -append). 

I don't know the specifics of other emulated platforms, but to me the whole issue looks messy, and the details of using the newly added arg option just seem to increase the entropy.

from what I understood, for MIPS the emulator must be called with a sequence of ,arg=xxx, while for ARM it needs to be called with a single long string including the entire command line.

I understand that different platforms have different specifics, but I have difficulties to understand why they cannot be implemented consistently from the user point of view, not the qemu developer point of view.

as such, in my opinion there should be either a single string, split internally by the parser when needed, or an array of substrings, concatenated internally when needed.

unless these things get clarified, for GNU ARM Eclipse QEMU, I'm inclined to keep my "--semihosting-cmdline arg0 arg1 ... argn" as it is easier to use and understand.



regards,

Liviu

p.s. and I think there are major differences between using -append to pass arguments to the kernel and passing arguments to semihosting, this being one of the reasons for suggesting a new command.

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

[Peter Maydell]

On 21 May 2015 at 15:28, Liviu Ionescu <ilg@livius.net> wrote:
> from what I understood, for MIPS the emulator must be called with a
> sequence of ,arg=xxx, while for ARM it needs to be called with a single
> long string including the entire command line.

You can use a sequence of ,arg=xxx on ARM too. It's just that it might
be easier not to bother, because the guest can't tell the difference
between:
 (1) the user specified three arguments "foo", "bar" and "baz", which
QEMU concatenated together with spaces in order to give the guest
the semihosting commandling string "foo bar baz"
 (2) the user specified a single argument "foo bar baz" with spaces in it

> I understand that different platforms have different specifics,
> but I have difficulties to understand why they cannot be implemented
> consistently from the user point of view, not the qemu developer point
> of view.

I am definitely strongly in favour of making this as consistent
for the user as we can manage.

> as such, in my opinion there should be either a single string,
> split internally by the parser when needed, or an array of
> substrings, concatenated internally when needed.

I agree that we should not require the target code to do the
splitting and concatenation. I think we're heading towards
the array-of-substrings approach.

> unless these things get clarified, for GNU ARM Eclipse QEMU, I'm inclined to keep my "--semihosting-cmdline arg0 arg1 ... argn" as it is easier to use and understand.

This won't work for MIPS, because there's no way to
specify different arguments properly.

-- PMM

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

[Liviu Ionescu]

> On 21 May 2015, at 17:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 21 May 2015 at 15:28, Liviu Ionescu <ilg@livius.net> wrote:
>> from what I understood, for MIPS the emulator must be called with a
>> sequence of ,arg=xxx, while for ARM it needs to be called with a single
>> long string including the entire command line.
> 
> You can use a sequence of ,arg=xxx on ARM too. It's just that it might
> be easier not to bother, because the guest can't tell the difference
> between:
> (1) the user specified three arguments "foo", "bar" and "baz", which
> QEMU concatenated together with spaces in order to give the guest
> the semihosting commandling string "foo bar baz"
> (2) the user specified a single argument "foo bar baz" with spaces in it
> 
>> as such, in my opinion there should be either a single string,
>> split internally by the parser when needed, or an array of
>> substrings, concatenated internally when needed.
> 
> I agree that we should not require the target code to do the
> splitting and concatenation. I think we're heading towards
> the array-of-substrings approach.

a few lines before you presented two examples. I guess the users will choose most of the time the second one, which is not an array of substrings approach.

regarding splitting on the target, for ARM semihosting you cannot avoid it, but this is implemented anyway in the startup code, not in user code.

> 
>> unless these things get clarified, for GNU ARM Eclipse QEMU, I'm inclined to keep my "--semihosting-cmdline arg0 arg1 ... argn" as it is easier to use and understand.
> 
> This won't work for MIPS, because there's no way to
> specify different arguments properly.

I'm afraid you are missing something. since I already implemented this, I can tell you that this was the cleanest method, there was no intermediate parsing on the way, I just copied the pointers from the tail of the qemu argv[] to an internal array, and this array was passed unchanged to semihosting, the emulated process receiving *exactly* what the qemu receives, without any processing. for ARM, the substrings were concatenated, using quotes or apostrophes for args containing spaces.


regards,

Liviu

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

[Peter Maydell]

On 21 May 2015 at 15:57, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 21 May 2015, at 17:33, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 21 May 2015 at 15:28, Liviu Ionescu <ilg@livius.net> wrote:
>>> from what I understood, for MIPS the emulator must be called with a
>>> sequence of ,arg=xxx, while for ARM it needs to be called with a single
>>> long string including the entire command line.
>>
>> You can use a sequence of ,arg=xxx on ARM too. It's just that it might
>> be easier not to bother, because the guest can't tell the difference
>> between:
>> (1) the user specified three arguments "foo", "bar" and "baz", which
>> QEMU concatenated together with spaces in order to give the guest
>> the semihosting commandling string "foo bar baz"
>> (2) the user specified a single argument "foo bar baz" with spaces in it
>>
>>> as such, in my opinion there should be either a single string,
>>> split internally by the parser when needed, or an array of
>>> substrings, concatenated internally when needed.
>>
>> I agree that we should not require the target code to do the
>> splitting and concatenation. I think we're heading towards
>> the array-of-substrings approach.
>
> a few lines before you presented two examples. I guess the users
> will choose most of the time the second one, which is not an array
> of substrings approach.

That's their choice... It is still an array of substrings,
but if the user only provides one string then we have an
array of length 1.

> regarding splitting on the target, for ARM semihosting you cannot avoid
> it, but this is implemented anyway in the startup code, not in user code.

This is the guest code. QEMU has no say in what the guest does,
so it's not relevant to the discussion.

>>> unless these things get clarified, for GNU ARM Eclipse QEMU, I'm inclined to keep my "--semihosting-cmdline arg0 arg1 ... argn" as it is easier to use and understand.
>>
>> This won't work for MIPS, because there's no way to
>> specify different arguments properly.
>
> I'm afraid you are missing something.

Sorry, I misinterpreted those quotes. Anyway, this
is just different syntax, it's the same effectively
as arg=x,arg=y,arg=z.

> implemented this, I can tell you that this was the
> cleanest method, there was no intermediate parsing on
> the way, I just copied the pointers from the tail of
> the qemu argv[] to an internal array, and this array
> was passed unchanged to semihosting, the emulated process
> receiving *exactly* what the qemu receives, without any
> processing. for ARM, the substrings were concatenated,
> using quotes or apostrophes for args containing spaces.

Adding quotes and apostrophes seems wrong to me,
because it's making assumptions about the guest's
handling of the semihosting string.

-- PMM

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

[Liviu Ionescu]

> On 21 May 2015, at 18:06, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> ...
> That's their choice... It is still an array of substrings,
> but if the user only provides one string then we have an
> array of length 1.

this was my initial point, as long as you offer the choice, you no longer have consistent usage between ARM and MIPS, the ARM users will prefer the array of length 1.

> ...
> Adding quotes and apostrophes seems wrong to me,
> because it's making assumptions about the guest's
> handling of the semihosting string.

yes, it is making the assumption that the guest code is able to process args with embedded spaces. once this accepted, properly parsing the quotes and apostrophes is required anyway, so there shouldn't be a big problem.


regards,

Liviu

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

[Peter Maydell]

On 21 May 2015 at 16:24, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 21 May 2015, at 18:06, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> ...
>> That's their choice... It is still an array of substrings,
>> but if the user only provides one string then we have an
>> array of length 1.
>
> this was my initial point, as long as you offer the choice,
> you no longer have consistent usage between ARM and MIPS,
> the ARM users will prefer the array of length 1.

There's no way to not offer the choice. In your syntax
this would be
 --semihosting-cmdline "foo bar baz"

>> ...
>> Adding quotes and apostrophes seems wrong to me,
>> because it's making assumptions about the guest's
>> handling of the semihosting string.
>
> yes, it is making the assumption that the guest code is able to
> process args with embedded spaces. once this accepted, properly
> parsing the quotes and apostrophes is required anyway

Required by who? It's not in the semihosting API. You
could perfectly well have a guest which required arguments
with spaces to be quoted via <> characters, or which
just treated the whole string as a single element, or
which didn't have any quoting support...

-- PMM

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

[Liviu Ionescu]

> On 21 May 2015, at 18:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> 
> There's no way to not offer the choice. In your syntax
> this would be
> --semihosting-cmdline "foo bar baz"

not exactly. in my implementation this would arrive in the guest code as argc=1, argv[0]="foo bar baz", which will be understood as a weird program name. 

>> ...
>> yes, it is making the assumption that the guest code is able to
>> process args with embedded spaces. once this accepted, properly
>> parsing the quotes and apostrophes is required anyway
> 
> Required by who? It's not in the semihosting API. You
> could perfectly well have a guest which required arguments
> with spaces to be quoted via <> characters, or which
> just treated the whole string as a single element, or
> which didn't have any quoting support...

I doubt implementations quoting with <> are really in use and have enough value to be a problem.
 

regards,

Liviu

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

[Peter Maydell]

On 21 May 2015 at 16:47, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 21 May 2015, at 18:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>
>> There's no way to not offer the choice. In your syntax
>> this would be
>> --semihosting-cmdline "foo bar baz"
>
> not exactly. in my implementation this would arrive in the guest
> code as argc=1, argv[0]="foo bar baz", which will be understood as
> a weird program name.

Well, it's the same as --semihosting-options,arg="foo bar baz".
That will arrive in the guest code as "foo bar baz", which
the guest code may or may not choose to interpret as a
program name foo and two arguments.

At the semihosting API level all the guest sees is a single
string and its length.

-- PMM

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

[Liviu Ionescu]

> On 21 May 2015, at 18:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 21 May 2015 at 16:47, Liviu Ionescu <ilg@livius.net> wrote:
>> 
>>> On 21 May 2015, at 18:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 
>>> 
>>> There's no way to not offer the choice. In your syntax
>>> this would be
>>> --semihosting-cmdline "foo bar baz"
>> 
>> not exactly. in my implementation this would arrive in the guest
>> code as argc=1, argv[0]="foo bar baz", which will be understood as
>> a weird program name.
> 
> Well, it's the same as --semihosting-options,arg="foo bar baz".
> That will arrive in the guest code as "foo bar baz", which
> the guest code may or may not choose to interpret as a
> program name foo and two arguments.

negative. 

your example: 

	... --semihosting-options,arg="foo bar baz" ...

will arrive in the guest code as: 

	['f' 'o' 'o' ' ' 'b' 'a' 'r' ' ' 'b' 'a' 'z']

which any non-brain damaged parser should understand as: 

	argc=3, argv[0]="foo", argv[1]="bar", argv[2]="baz"

while in my implementation a command line like: 

	... --semihosting-cmdline "foo bar baz" CR 

will arrive as: 

	['"' 'f' 'o' 'o' ' ' 'b' 'a' 'r' ' ' 'b' 'a' 'z' '"'] 

which a reasonably smart parser (like the one I use) will understand as: 

	argc=1, argv[0]="foo bar baz"

it is true that a dumb parser would return: 

	argc=3, argv[0]="\"foo", argv[1]="bar", argv[2]="baz\""

but from this it is not very difficult to understand that args with spaces should be avoided.


regards,

Liviu

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

[Peter Maydell]

On 21 May 2015 at 17:36, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 21 May 2015, at 18:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 21 May 2015 at 16:47, Liviu Ionescu <ilg@livius.net> wrote:
>>>
>>>> On 21 May 2015, at 18:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>>
>>>> There's no way to not offer the choice. In your syntax
>>>> this would be
>>>> --semihosting-cmdline "foo bar baz"
>>>
>>> not exactly. in my implementation this would arrive in the guest
>>> code as argc=1, argv[0]="foo bar baz", which will be understood as
>>> a weird program name.
>>
>> Well, it's the same as --semihosting-options,arg="foo bar baz".
>> That will arrive in the guest code as "foo bar baz", which
>> the guest code may or may not choose to interpret as a
>> program name foo and two arguments.
>
> negative.
>
> your example:
>
>         ... --semihosting-options,arg="foo bar baz" ...
>
> will arrive in the guest code as:
>
>         ['f' 'o' 'o' ' ' 'b' 'a' 'r' ' ' 'b' 'a' 'z']
>
> which any non-brain damaged parser should understand as:
>
>         argc=3, argv[0]="foo", argv[1]="bar", argv[2]="baz"
>
> while in my implementation a command line like:
>
>         ... --semihosting-cmdline "foo bar baz" CR
>
> will arrive as:
>
>         ['"' 'f' 'o' 'o' ' ' 'b' 'a' 'r' ' ' 'b' 'a' 'z' '"']

That's because your code is adding quotes, which I've already
said I think is incorrect. If you took out the addition of
quotes the two would be the same.

-- PMM

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

[Liviu Ionescu]

> On 21 May 2015, at 19:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> ... That's because your code is adding quotes, which I've already
> said I think is incorrect. If you took out the addition of
> quotes the two would be the same.

it might be incorrect for some obscure/broken guest implementations, but my semihosting code is fully functional since last year, while yours is not only more difficult to use, it allows inconsistent use cases and it is still unknown when will be functional for arm and available in the public repository.


regards,

Liviu

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

[Maciej W. Rozycki]

On Thu, 21 May 2015, Liviu Ionescu wrote:

> p.s. and I think there are major differences between using -append to 
> pass arguments to the kernel and passing arguments to semihosting, this 
> being one of the reasons for suggesting a new command.

 Out of curiosity, why do you think there are differences between passing 
arguments to `vmlinux' and any other bare-metal executable, or maybe more 
specifically what differences do you have in mind?

  Maciej

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

[Liviu Ionescu]

> On 21 May 2015, at 21:35, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> 
> On Thu, 21 May 2015, Liviu Ionescu wrote:
> 
>> p.s. and I think there are major differences between using -append to 
>> pass arguments to the kernel and passing arguments to semihosting, this 
>> being one of the reasons for suggesting a new command.
> 
> Out of curiosity, why do you think there are differences between passing 
> arguments to `vmlinux' and any other bare-metal executable, or maybe more 
> specifically what differences do you have in mind?

because, as per ARM specs, the semihosting buffer inside guest applications should be at least 80 bytes, which, in practical terms, means... exactly 80 bytes.

when you use -kernel & -append, argv[0] is the full path of the kernel, which might be longer than 80 bytes itself.


another reason is that -kernel and subsequently -append are optional in some use cases. for example I start qemu as a gdb server, without any -kernel, and I load the image via the gdb client, as for any debug session. however I need to pass semihosting args to the application, and a separate --semihosting-cmdline makes more sense for this.


regards,

Liviu