[Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Liviu Ionescu]

A new sub-option was added to -semihosting-config to define the entire
semihosting command line (cmdline=string).

This string is passed down to armv7m.c; if not defined, for
compatibility reasons, the -kernel -append values are used.

The armv7m_init() and stellaris_init() interfaces were streamlined,
to use the MachineState structure instead of separate strings.

The semihosting_cmdline was added to the structures MachineState and
arm_boot_info.


Signed-off-by: Liviu Ionescu <ilg@livius.net>
---
 hw/arm/armv7m.c       | 15 ++++++++++++++-
 hw/arm/stellaris.c    | 12 ++++--------
 include/hw/arm/arm.h  |  3 ++-
 include/hw/boards.h   |  1 +
 qemu-options.hx       | 10 +++++++---
 target-arm/arm-semi.c | 19 +++++++++++++++----
 vl.c                  |  9 +++++++++
 7 files changed, 52 insertions(+), 17 deletions(-)
 mode change 100644 => 100755 qemu-options.hx

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index ef24ca4..696de12 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -13,6 +13,9 @@
 #include "elf.h"
 #include "sysemu/qtest.h"
 #include "qemu/error-report.h"
+#include "hw/boards.h"
+
+static struct arm_boot_info armv7m_binfo;
 
 /* Bitbanded IO.  Each word corresponds to a single bit.  */
 
@@ -168,7 +171,7 @@ static void armv7m_reset(void *opaque)
 
 qemu_irq *armv7m_init(MemoryRegion *system_memory,
                       int flash_size, int sram_size,
-                      const char *kernel_filename, const char *cpu_model)
+                      MachineState *machine)
 {
     ARMCPU *cpu;
     CPUARMState *env;
@@ -184,6 +187,9 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
     MemoryRegion *flash = g_new(MemoryRegion, 1);
     MemoryRegion *hack = g_new(MemoryRegion, 1);
 
+    const char *kernel_filename = machine->kernel_filename;
+    const char *cpu_model = machine->cpu_model;
+
     flash_size *= 1024;
     sram_size *= 1024;
 
@@ -240,6 +246,13 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
         exit(1);
     }
 
+    /* Fill-in a minimalistic boot info, required for semihosting */
+    armv7m_binfo.kernel_filename = kernel_filename;
+    armv7m_binfo.kernel_cmdline = machine->kernel_cmdline;
+    armv7m_binfo.semihosting_cmdline = machine->semihosting_cmdline;
+
+    env->boot_info = &armv7m_binfo;
+
     if (kernel_filename) {
         image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
                               NULL, big_endian, ELF_MACHINE, 1);
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 64bd4b4..f93326a 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1198,7 +1198,7 @@ static stellaris_board_info stellaris_boards[] = {
   }
 };
 
-static void stellaris_init(const char *kernel_filename, const char *cpu_model,
+static void stellaris_init(MachineState *machine,
                            stellaris_board_info *board)
 {
     static const int uart_irq[] = {5, 6, 33, 34};
@@ -1223,7 +1223,7 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
     flash_size = ((board->dc0 & 0xffff) + 1) << 1;
     sram_size = (board->dc0 >> 18) + 1;
     pic = armv7m_init(get_system_memory(),
-                      flash_size, sram_size, kernel_filename, cpu_model);
+                      flash_size, sram_size, machine);
 
     if (board->dc1 & (1 << 16)) {
         dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
@@ -1335,16 +1335,12 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
 /* FIXME: Figure out how to generate these from stellaris_boards.  */
 static void lm3s811evb_init(MachineState *machine)
 {
-    const char *cpu_model = machine->cpu_model;
-    const char *kernel_filename = machine->kernel_filename;
-    stellaris_init(kernel_filename, cpu_model, &stellaris_boards[0]);
+    stellaris_init(machine, &stellaris_boards[0]);
 }
 
 static void lm3s6965evb_init(MachineState *machine)
 {
-    const char *cpu_model = machine->cpu_model;
-    const char *kernel_filename = machine->kernel_filename;
-    stellaris_init(kernel_filename, cpu_model, &stellaris_boards[1]);
+    stellaris_init(machine, &stellaris_boards[1]);
 }
 
 static QEMUMachine lm3s811evb_machine = {
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index cefc9e6..bae853c 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -17,7 +17,7 @@
 /* armv7m.c */
 qemu_irq *armv7m_init(MemoryRegion *system_memory,
                       int flash_size, int sram_size,
-                      const char *kernel_filename, const char *cpu_model);
+                      MachineState *machine);
 
 /* arm_boot.c */
 struct arm_boot_info {
@@ -26,6 +26,7 @@ struct arm_boot_info {
     const char *kernel_cmdline;
     const char *initrd_filename;
     const char *dtb_filename;
+    const char *semihosting_cmdline;
     hwaddr loader_start;
     /* multicore boards that use the default secondary core boot functions
      * need to put the address of the secondary boot code, the boot reg,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index e0a6790..a739d74 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -142,6 +142,7 @@ struct MachineState {
     char *kernel_cmdline;
     char *initrd_filename;
     const char *cpu_model;
+    char *semihosting_cmdline;
     AccelState *accelerator;
 };
 
diff --git a/qemu-options.hx b/qemu-options.hx
old mode 100644
new mode 100755
index 3e81222..c5db4d1
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3219,14 +3219,18 @@ 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[,cmdline=string]   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[,cmdline=string]
 @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 optional
+@code{cmdline} defines the entire command line passed to the application via the
+semihosting SYS_GET_CMDLINE call, including the program name that will be
+passed as argv[0].
+(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/target-arm/arm-semi.c b/target-arm/arm-semi.c
index ebb5235..b5b9c76 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -436,10 +436,14 @@ 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)
+            if (ts->boot_info->semihosting_cmdline) {
+                output_size = strlen(ts->boot_info->semihosting_cmdline) + 1;
+            } else {
+                output_size = strlen(ts->boot_info->kernel_filename)
                         + 1  /* Separating space.  */
                         + strlen(ts->boot_info->kernel_cmdline)
                         + 1; /* Terminating null byte.  */
+            }
 #else
             unsigned int i;
 
@@ -470,9 +474,16 @@ 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);
+            if (ts->boot_info->semihosting_cmdline) {
+                pstrcpy(output_buffer, output_size,
+                        ts->boot_info->semihosting_cmdline);
+            } else {
+                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);
+            }
 #else
             if (output_size == 1) {
                 /* Empty command-line.  */
diff --git a/vl.c b/vl.c
index 8844983..a88e5da 100644
--- a/vl.c
+++ b/vl.c
@@ -566,6 +566,9 @@ static QemuOptsList qemu_semihosting_config_opts = {
         }, {
             .name = "target",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "cmdline",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -2758,6 +2761,7 @@ int main(int argc, char **argv, char **envp)
     int snapshot, linux_boot;
     const char *initrd_filename;
     const char *kernel_filename, *kernel_cmdline;
+    const char *semihosting_cmdline = NULL;
     const char *boot_order;
     DisplayState *ds;
     int cyls, heads, secs, translation;
@@ -3662,6 +3666,9 @@ int main(int argc, char **argv, char **envp)
                     } else {
                         semihosting_target = SEMIHOSTING_TARGET_AUTO;
                     }
+
+                    semihosting_cmdline = qemu_opt_get(opts, "cmdline");
+
                 } else {
                     fprintf(stderr, "Unsupported semihosting-config %s\n",
                             optarg);
@@ -4169,6 +4176,8 @@ int main(int argc, char **argv, char **envp)
         current_machine->kernel_cmdline = (char *)kernel_cmdline;
     }
 
+    current_machine->semihosting_cmdline = (char *)semihosting_cmdline;
+
     linux_boot = (kernel_filename != NULL);
 
     if (!linux_boot && *kernel_cmdline != '\0') {
-- 
1.9.3 (Apple Git-50)

Re: [Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Peter Maydell]

On 19 November 2014 22:05, Liviu Ionescu <ilg@livius.net> wrote:
> A new sub-option was added to -semihosting-config to define the entire
> semihosting command line (cmdline=string).
>
> This string is passed down to armv7m.c; if not defined, for
> compatibility reasons, the -kernel -append values are used.
>
> The armv7m_init() and stellaris_init() interfaces were streamlined,
> to use the MachineState structure instead of separate strings.
>
> The semihosting_cmdline was added to the structures MachineState and
> arm_boot_info.

I think you can avoid having to plumb the command line
string into the MachineState and arm_boot_info structures,
because you can just have the semihosting code look the
option up by name:

     QemuOpts *opts =
qemu_opts_find(qemu_find_opts("semihosting-config"), NULL);
     cmdline = qemu_opt_get(opts, "cmdline");
     if (cmdline) {
         ...
     } else {
         fall back to constructing from kernel/append args;
     }

That will also automatically make the command line option
work for A profile CPUs as well.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Liviu Ionescu]

On 20 Nov 2014, at 15:31, Peter Maydell <peter.maydell@linaro.org> wrote:

> I think you can avoid having to plumb the command line
> string into the MachineState and arm_boot_info structures,
> because you can just have the semihosting code look the
> option up by name:
> 
>     QemuOpts *opts =
> qemu_opts_find(qemu_find_opts("semihosting-config"), NULL);

something is missing with the way options are handled, this call fails with BAD_ACCESS.

I tried to debug the problem, but with little success so far.

> That will also automatically make the command line option
> work for A profile CPUs as well.

where is the A profile code?


regards,

Liviu

Re: [Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Peter Maydell]

On 20 November 2014 15:03, Liviu Ionescu <ilg@livius.net> wrote:
>
> On 20 Nov 2014, at 15:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> I think you can avoid having to plumb the command line
>> string into the MachineState and arm_boot_info structures,
>> because you can just have the semihosting code look the
>> option up by name:
>>
>>     QemuOpts *opts =
>> qemu_opts_find(qemu_find_opts("semihosting-config"), NULL);
>
> something is missing with the way options are handled, this call fails with BAD_ACCESS.

What's this? It's not an error code in QEMU...

> I tried to debug the problem, but with little success so far.

I'll have a look.

>> That will also automatically make the command line option
>> work for A profile CPUs as well.
>
> where is the A profile code?

A profile CPUs generally boot via the code in hw/arm/boot.c.

-- PMM

Re: [Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Liviu Ionescu]

On 20 Nov 2014, at 17:10, Peter Maydell <peter.maydell@linaro.org> wrote:

>> something is missing with the way options are handled, this call fails with BAD_ACCESS.
> 
> What's this? It's not an error code in QEMU...

no, it is a system condition, EXC_BAD_ACCESS, generally caused by a bad pointer. (on linux probably you get a signal).

I also tried with ".merge_lists = true," but no difference.


Liviu

Re: [Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Peter Maydell]

On 20 November 2014 15:17, Liviu Ionescu <ilg@livius.net> wrote:
>
> On 20 Nov 2014, at 17:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>>> something is missing with the way options are handled, this call fails with BAD_ACCESS.
>>
>> What's this? It's not an error code in QEMU...
>
> no, it is a system condition, EXC_BAD_ACCESS, generally caused by a bad pointer. (on linux probably you get a signal).

Ah, right, SIGSEGV. Anyway, I had a quick play
and it seems to work correctly for me: with this diff:


diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
index a8b83e6..d822641 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -36,6 +36,8 @@
 #include "exec/gdbstub.h"
 #include "hw/arm/arm.h"
 #endif
+#include "qemu/option.h"
+#include "qemu/config-file.h"

 #define TARGET_SYS_OPEN        0x01
 #define TARGET_SYS_CLOSE       0x02
@@ -199,6 +201,11 @@ uint32_t do_arm_semihosting(CPUARMState *env)
 #else
     CPUARMState *ts = env;
 #endif
+    QemuOpts *opts;
+    const char *cmdline;
+    opts = qemu_opts_find(qemu_find_opts("semihosting-config"), NULL);
+    cmdline = qemu_opt_get(opts, "cmdline");
+    printf("got cmdline %s\n", cmdline ? cmdline : "(null)");

     nr = env->regs[0];
     args = env->regs[1];
diff --git a/vl.c b/vl.c
index 9b69dc3..97eac57 100644
--- a/vl.c
+++ b/vl.c
@@ -565,6 +565,9 @@ static QemuOptsList qemu_semihosting_config_opts = {
         }, {
             .name = "target",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "cmdline",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },


it prints the command line string at every semihosting call,
if there is one. (I just stuck it at the top of the function
for testing since the test binary I happened to have to hand
didn't do the 'get commandline' call.)

NB: you'll want to stick this inside the !CONFIG_USER_ONLY
ifdef, because on linux-user it'll compile but the
qemu_find_opts() call will fail and then we'll segfault
because we pass a NULL to qemu_opts_find().

-- PMM

Re: [Qemu-devel] [PATCH] Add -semihosting-config ....cmdline=string.

[Liviu Ionescu]

On 20 Nov 2014, at 17:39, Peter Maydell <peter.maydell@linaro.org> wrote:

> NB: you'll want to stick this inside the !CONFIG_USER_ONLY

did this.

now it works for me, but please check if it compiles properly for other targets.

regards,

Liviu