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

[Qemu-devel] [PATCH 0/4] 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

Leon Alrae (4):
  semihosting: remove semihosting_enabled declaration from sysemu.h
  semihosting: remove semihosting_target declaration from gdbstub.h
  semihosting: create SemihostingConfig struct
  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           |  8 ++++--
 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, 137 insertions(+), 31 deletions(-)
 create mode 100644 include/exec/semihost.h

[Qemu-devel] [PATCH 1/4] semihosting: remove semihosting_enabled declaration from sysemu.h

[Leon Alrae]

Remove extern int semihosting_enabled declaration from sysemu.h and
create equivalent semihosting_enabled() function. Also introduce
semihost.h containing things related to semihosting config.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 include/exec/semihost.h   | 32 ++++++++++++++++++++++++++++++++
 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                      | 18 ++++++++++++++----
 7 files changed, 56 insertions(+), 13 deletions(-)
 create mode 100644 include/exec/semihost.h

diff --git a/include/exec/semihost.h b/include/exec/semihost.h
new file mode 100644
index 0000000..5d9a916
--- /dev/null
+++ b/include/exec/semihost.h
@@ -0,0 +1,32 @@
+/*
+ * 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
+
+#ifdef CONFIG_USER_ONLY
+static inline bool semihosting_enabled(void)
+{
+    return true;
+}
+#else
+bool semihosting_enabled(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..5a8040d 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,16 @@ static void configure_msg(QemuOpts *opts)
 }
 
 /***********************************************************/
+/* Semihosting */
+
+static bool semihosting_allowed;
+
+bool semihosting_enabled(void)
+{
+    return semihosting_allowed;
+}
+
+/***********************************************************/
 /* USB devices */
 
 static int usb_device_add(const char *devname)
@@ -3545,15 +3555,15 @@ int main(int argc, char **argv, char **envp)
                 nb_option_roms++;
                 break;
             case QEMU_OPTION_semihosting:
-                semihosting_enabled = 1;
+                semihosting_allowed = true;
                 semihosting_target = SEMIHOSTING_TARGET_AUTO;
                 break;
             case QEMU_OPTION_semihosting_config:
-                semihosting_enabled = 1;
+                semihosting_allowed = true;
                 opts = qemu_opts_parse(qemu_find_opts("semihosting-config"),
                                            optarg, 0);
                 if (opts != NULL) {
-                    semihosting_enabled = qemu_opt_get_bool(opts, "enable",
+                    semihosting_allowed = qemu_opt_get_bool(opts, "enable",
                                                             true);
                     const char *target = qemu_opt_get(opts, "target");
                     if (target != NULL) {

[Qemu-devel] [PATCH 2/4] semihosting: remove semihosting_target declaration from gdbstub.h

[Leon Alrae]

Follow the semihosting_enabled() approach and move semihosting_target
related stuff to semihost.h.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 gdbstub.c               |  8 ++++----
 include/exec/gdbstub.h  |  6 ------
 include/exec/semihost.h | 12 ++++++++++++
 vl.c                    |  6 ++++++
 4 files changed, 22 insertions(+), 10 deletions(-)

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
index 5d9a916..c2f0bcb 100644
--- a/include/exec/semihost.h
+++ b/include/exec/semihost.h
@@ -20,13 +20,25 @@
 #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/vl.c b/vl.c
index 5a8040d..a42f127 100644
--- a/vl.c
+++ b/vl.c
@@ -1228,12 +1228,18 @@ static void configure_msg(QemuOpts *opts)
 /* Semihosting */
 
 static bool semihosting_allowed;
+static SemihostingTarget semihosting_target;
 
 bool semihosting_enabled(void)
 {
     return semihosting_allowed;
 }
 
+SemihostingTarget semihosting_get_target(void)
+{
+    return semihosting_target;
+}
+
 /***********************************************************/
 /* USB devices */
 

[Qemu-devel] [PATCH 3/4] semihosting: create SemihostingConfig struct

[Leon Alrae]

Group semihosting config related variables in a single structure.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 vl.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/vl.c b/vl.c
index a42f127..82586c7 100644
--- a/vl.c
+++ b/vl.c
@@ -1227,17 +1227,21 @@ static void configure_msg(QemuOpts *opts)
 /***********************************************************/
 /* Semihosting */
 
-static bool semihosting_allowed;
-static SemihostingTarget semihosting_target;
+typedef struct SemihostingConfig {
+    bool allowed;
+    SemihostingTarget target;
+} SemihostingConfig;
+
+static SemihostingConfig semihosting;
 
 bool semihosting_enabled(void)
 {
-    return semihosting_allowed;
+    return semihosting.allowed;
 }
 
 SemihostingTarget semihosting_get_target(void)
 {
-    return semihosting_target;
+    return semihosting.target;
 }
 
 /***********************************************************/
@@ -3561,24 +3565,24 @@ int main(int argc, char **argv, char **envp)
                 nb_option_roms++;
                 break;
             case QEMU_OPTION_semihosting:
-                semihosting_allowed = true;
-                semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                semihosting.allowed = true;
+                semihosting.target = SEMIHOSTING_TARGET_AUTO;
                 break;
             case QEMU_OPTION_semihosting_config:
-                semihosting_allowed = true;
+                semihosting.allowed = true;
                 opts = qemu_opts_parse(qemu_find_opts("semihosting-config"),
                                            optarg, 0);
                 if (opts != NULL) {
-                    semihosting_allowed = qemu_opt_get_bool(opts, "enable",
+                    semihosting.allowed = 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",
@@ -3586,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 4/4] 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         |  8 +++++---
 vl.c                    | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 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..ed2a7e8 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3296,14 +3296,16 @@ 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).
+@code{gdb} during debug sessions and @code{native} otherwise. The @var{arg} allows to
+pass input arguments, can be used multiple times to build up a list (ARM, M68K, Xtensa only)
 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 82586c7..203ac86 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 allowed;
     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 4/4] semihosting: add --semihosting-config arg sub-argument

[Liviu Ionescu]

> On 06 May 2015, at 17:57, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> +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;
> +}

being done at init time probably it has no impact, but, as a matter of style, I would avoid iterating realloc when the buffer size is actually known.

is it that difficult to count the "arg"s and correctly alloc the array?


regards,

Liviu

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

[Leon Alrae]

On 07/05/2015 07:51, Liviu Ionescu wrote:
> 
>> On 06 May 2015, at 17:57, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>
>> +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;
>> +}
> 
> being done at init time probably it has no impact, but, as a matter of style, I would avoid iterating realloc when the buffer size is actually known.

At this point QEMU doesn't know the buffer size, thus we need to
traverse the list of sub-arguments to determine the number of args and
also to get the value (i.e. string) of each arg.

> 
> is it that difficult to count the "arg"s and correctly alloc the array?

This probably would require going through the list twice which wouldn't
be better in my opinion.

Leon

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

[Liviu Ionescu]

> On 07 May 2015, at 12:52, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
>> is it that difficult to count the "arg"s and correctly alloc the array?
> 
> This probably would require going through the list twice 

the code to iterate is already there, for other options, you just need to add a branch to the existing case.


regards,

Liviu

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

[Leon Alrae]

On 07/05/2015 12:50, Liviu Ionescu wrote:
> 
>> On 07 May 2015, at 12:52, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>
>>> is it that difficult to count the "arg"s and correctly alloc the array?
>>
>> This probably would require going through the list twice 
> 
> the code to iterate is already there, for other options, you just need to add a branch to the existing case.

Do you mean modifying QEMU option parser? But I don't see anything wrong
in using qemu_opt_foreach().

Leon

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

[Liviu Ionescu]

> On 07 May 2015, at 15:21, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> On 07/05/2015 12:50, Liviu Ionescu wrote:
>> 
>>> On 07 May 2015, at 12:52, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>> 
>>>> is it that difficult to count the "arg"s and correctly alloc the array?
>>> 
>>> This probably would require going through the list twice 
>> 
>> the code to iterate is already there, for other options, you just need to add a branch to the existing case.
> 
> Do you mean modifying QEMU option parser?

not at all.

option parsing is done anyway in two passes. the first pass is a short loop, used to identify nodeconfig & nouserconfig. the second pass is the big for(;;) loop. you can use this first pass to count the number of args, similarly as I did in my patch.


regards,

Liviu

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

[Leon Alrae]

On 07/05/2015 13:35, Liviu Ionescu wrote:
> 
>> On 07 May 2015, at 15:21, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>
>> On 07/05/2015 12:50, Liviu Ionescu wrote:
>>>
>>>> On 07 May 2015, at 12:52, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>>>
>>>>> is it that difficult to count the "arg"s and correctly alloc the array?
>>>>
>>>> This probably would require going through the list twice 
>>>
>>> the code to iterate is already there, for other options, you just need to add a branch to the existing case.
>>
>> Do you mean modifying QEMU option parser?
> 
> not at all.
> 
> option parsing is done anyway in two passes. the first pass is a short loop, used to identify nodeconfig & nouserconfig. the second pass is the big for(;;) loop. you can use this first pass to count the number of args, similarly as I did in my patch.
> 

I think parsing -semihosting-config sub-options should happen inside
"case QEMU_OPTION_semihosting_config", just to be consistent with other
options.

Leon

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

[Liviu Ionescu]

apparently your patch does not fix the arm semihosting problems. do you plan a separate patch for this?


> bool semihosting_enabled(void)
> {
>     return semihosting.allowed;
> }

any particular reason for naming the structure member ".allowed" and the getter function "_enabled()"?


regards,

Liviu

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

[Leon Alrae]

On 06/05/2015 16:22, Liviu Ionescu wrote:
> 
> apparently your patch does not fix the arm semihosting problems. do you plan a separate patch for this?

This patchset doesn't contain any target semihosting specific changes
(just a clean up and new arg option). I'm going to follow up with UHI
patch series only. I don't have anything set up to test other
semihosting interfaces, thus I don't plan to do any changes in these
areas in near future. Therefore feel free to send your ARM semihosting
improvements.

> 
> 
>> bool semihosting_enabled(void)
>> {
>>     return semihosting.allowed;
>> }
> 
> any particular reason for naming the structure member ".allowed" and the getter function "_enabled()"?

Generally no, but patch #1 introduced semihosting_enabled() function,
hence this change. However, thanks for pointing this out as I just
realized that with the patch #3 this rename was not required at all.
I'll correct that.

Thanks,
Leon

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

[Liviu Ionescu]

> On 06 May 2015, at 19:12, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> ... I'm going to follow up with UHI
> patch series only. I don't have anything set up to test other
> semihosting interfaces, thus I don't plan to do any changes in these
> areas in near future. Therefore feel free to send your ARM semihosting
> improvements.

are these patches finally in? if so, what repo/branch should I use to get them?


regards,

Liviu

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

[Leon Alrae]

On 13/05/2015 06:48, Liviu Ionescu wrote:
> are these patches finally in? if so, what repo/branch should I use to get them?

Note that v3 was sent quite recently and there should be some time to
give everyone a chance to comment. I was planning to send it out later,
together with UHI via target-mips tree. However, given there were no
more issues pointed out and there are more semihosting patches incoming
which depend on these changes, maybe it would be worth to apply them
now? If there are no objections, I'll prepare a separate pullreq
containing just the clean-up and -semihosting-config arg.

Regards,
Leon

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

[Peter Maydell]

On 13 May 2015 at 10:29, Leon Alrae <leon.alrae@imgtec.com> wrote:
> On 13/05/2015 06:48, Liviu Ionescu wrote:
>> are these patches finally in? if so, what repo/branch should I use to get them?
>
> Note that v3 was sent quite recently and there should be some time to
> give everyone a chance to comment.

It's still on my to-review list, yes. I hope to get to it this week
sometime.

thanks
-- PMM