| Message ID | 1432656234-9791-3-git-send-email-leon.alrae@imgtec.com |
|---|---|
| State | New |
| Headers | show |
On 26 May 2015 at 17:03, Leon Alrae <leon.alrae@imgtec.com> wrote: > Add new "arg" sub-argument to the --semihosting-config allowing the user > to pass multiple input arguments separately. It is required for example > by UHI semihosting to construct argc and argv. > > Also, update ARM semihosting to support new option (at the moment it is > the only target which cares about arguments). > > If the semihosting is enabled and no semihosting args have been specified, > then fall back to -kernel/-append. The -append string is split on whitespace > before initializing semihosting.argv[1..n]; this is different from what > QEMU MIPS machines' pseudo-bootloaders do (i.e. argv[1] contains the whole > -append), but is more intuitive from UHI user's point of view and Linux > kernel just does not care as it concatenates argv[1..n] into single cmdline > string anyway. > > Signed-off-by: Leon Alrae <leon.alrae@imgtec.com> > --- a/target-arm/arm-semi.c > +++ b/target-arm/arm-semi.c > @@ -27,6 +27,7 @@ > #include <time.h> > > #include "cpu.h" > +#include "exec/semihost.h" > #ifdef CONFIG_USER_ONLY > #include "qemu.h" > > @@ -440,10 +441,7 @@ uint32_t do_arm_semihosting(CPUARMState *env) > input_size = arg1; > /* Compute the size of the output string. */ > #if !defined(CONFIG_USER_ONLY) > - output_size = strlen(ts->boot_info->kernel_filename) > - + 1 /* Separating space. */ > - + strlen(ts->boot_info->kernel_cmdline) > - + 1; /* Terminating null byte. */ > + output_size = strlen(semihosting_get_cmdline()) + 1; It looks like semihosting_get_cmdline() can return NULL, in which case this will blow up, I think. Patch looks OK otherwise (haven't tested it yet). -- PMM
On 05/06/15 16:23, Peter Maydell wrote: > On 26 May 2015 at 17:03, Leon Alrae <leon.alrae@imgtec.com> wrote: >> --- a/target-arm/arm-semi.c >> +++ b/target-arm/arm-semi.c >> @@ -27,6 +27,7 @@ >> #include <time.h> >> >> #include "cpu.h" >> +#include "exec/semihost.h" >> #ifdef CONFIG_USER_ONLY >> #include "qemu.h" >> >> @@ -440,10 +441,7 @@ uint32_t do_arm_semihosting(CPUARMState *env) >> input_size = arg1; >> /* Compute the size of the output string. */ >> #if !defined(CONFIG_USER_ONLY) >> - output_size = strlen(ts->boot_info->kernel_filename) >> - + 1 /* Separating space. */ >> - + strlen(ts->boot_info->kernel_cmdline) >> - + 1; /* Terminating null byte. */ >> + output_size = strlen(semihosting_get_cmdline()) + 1; > > It looks like semihosting_get_cmdline() can return NULL, > in which case this will blow up, I think. semihosting_get_cmdline() returns NULL if neither semihosting args nor -kernel have been specified. As far as I can tell existing implementation may also blow up if kernel_filename is NULL, so we retain the same behaviour. Besides, it's not clear to me how the TARGET_SYS_GET_CMDLINE should behave if cmdline is not available, whether should return -1 or pass an empty string to the guest. For me this looks like a separate issue, not much related to this patch series. Thanks, Leon
> On 05 Jun 2015, at 23:09, Leon Alrae <leon.alrae@imgtec.com> wrote: > > ... As far as I can tell existing > implementation may also blow up if kernel_filename is NULL, not necessarily, in my branch it is perfectly legal to start qemu without an image, as long as the gdb server is started, since the application will be loaded by the gdb client. (this is how the Eclipse plug-in is configured to start qemu). > ... how the > TARGET_SYS_GET_CMDLINE should behave if cmdline is not available, > whether should return -1 or pass an empty string to the guest. for consistency I would suggest to return -1 for all cases that do not return a legal cmdline. regards, Liviu
On 5 June 2015 at 22:11, Liviu Ionescu <ilg@livius.net> wrote: > >> On 05 Jun 2015, at 23:09, Leon Alrae <leon.alrae@imgtec.com> wrote: >> ... how the >> TARGET_SYS_GET_CMDLINE should behave if cmdline is not available, >> whether should return -1 or pass an empty string to the guest. > > for consistency I would suggest to return -1 for all cases that do > not return a legal cmdline. The existing linux-user implementation of this semihosting call handles this case by returning the empty string, so consistency suggests following that in the equivalent softmmu case. -- PMM
> On 06 Jun 2015, at 01:54, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 5 June 2015 at 22:11, Liviu Ionescu <ilg@livius.net> wrote: >> >>> On 05 Jun 2015, at 23:09, Leon Alrae <leon.alrae@imgtec.com> wrote: >>> ... how the >>> TARGET_SYS_GET_CMDLINE should behave if cmdline is not available, >>> whether should return -1 or pass an empty string to the guest. >> >> for consistency I would suggest to return -1 for all cases that do >> not return a legal cmdline. > > The existing linux-user implementation of this semihosting > call handles this case by returning the empty string, so > consistency suggests following that in the equivalent > softmmu case. in this case it doesn't make any major difference, the application should accommodate both cases, but, as a general comment, if one case is broken/poorly implemented, for the sake of consistency I would not rush to make all others broken too, but I would first try to find a solution to fix/improve them all. regards, Liviu
would it be possible to have all the semihosting patches ready for 2.4? regards, Liviu
On 16 June 2015 at 13:32, Liviu Ionescu <ilg@livius.net> wrote:
> would it be possible to have all the semihosting patches ready for 2.4?
Yes, I agree that would be good. Is it just this 2 patch
series, or are there others too?
thanks
-- PMM
On 16/06/2015 15:03, Peter Maydell wrote: > On 16 June 2015 at 13:32, Liviu Ionescu <ilg@livius.net> wrote: >> would it be possible to have all the semihosting patches ready for 2.4? > > Yes, I agree that would be good. Is it just this 2 patch > series, or are there others too? Just to confirm -- the only thing required before applying this 2 patch series is testing in ARM context by you, right? Remaining semihosting patches I've got are MIPS specific, but they can go via target-mips tree separately. Leon
diff --git a/include/exec/semihost.h b/include/exec/semihost.h index c2f0bcb..5980939 100644 --- a/include/exec/semihost.h +++ b/include/exec/semihost.h @@ -36,9 +36,27 @@ static inline SemihostingTarget semihosting_get_target(void) { return SEMIHOSTING_TARGET_AUTO; } + +static inline const char *semihosting_get_arg(int i) +{ + return NULL; +} + +static inline int semihosting_get_argc(void) +{ + return 0; +} + +static inline const char *semihosting_get_cmdline(void) +{ + return NULL; +} #else bool semihosting_enabled(void); SemihostingTarget semihosting_get_target(void); +const char *semihosting_get_arg(int i); +int semihosting_get_argc(void); +const char *semihosting_get_cmdline(void); #endif #endif diff --git a/qemu-options.hx b/qemu-options.hx index ec356f6..90500ad 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3296,14 +3296,25 @@ STEXI Enable semihosting mode (ARM, M68K, Xtensa only). ETEXI DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config, - "-semihosting-config [enable=on|off,]target=native|gdb|auto semihosting configuration\n", + "-semihosting-config [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \ + " semihosting configuration\n", QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32) STEXI -@item -semihosting-config [enable=on|off,]target=native|gdb|auto +@item -semihosting-config [enable=on|off][,target=native|gdb|auto][,arg=str[,...]] @findex -semihosting-config -Enable semihosting and define where the semihosting calls will be addressed, -to QEMU (@code{native}) or to GDB (@code{gdb}). The default is @code{auto}, which means -@code{gdb} during debug sessions and @code{native} otherwise (ARM, M68K, Xtensa only). +Enable and configure semihosting (ARM, M68K, Xtensa only). +@table @option +@item target=@code{native|gdb|auto} +Defines where the semihosting calls will be addressed, to QEMU (@code{native}) +or to GDB (@code{gdb}). The default is @code{auto}, which means @code{gdb} +during debug sessions and @code{native} otherwise. +@item arg=@var{str1},arg=@var{str2},... +Allows the user to pass input arguments, and can be used multiple times to build +up a list. The old-style @code{-kernel}/@code{-append} method of passing a +command line is still supported for backward compatibility. If both the +@code{--semihosting-config arg} and the @code{-kernel}/@code{-append} are +specified, the former is passed to semihosting as it always takes precedence. +@end table ETEXI DEF("old-param", 0, QEMU_OPTION_old_param, "-old-param old param mode\n", QEMU_ARCH_ARM) diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c index a8b83e6..74a67e9 100644 --- a/target-arm/arm-semi.c +++ b/target-arm/arm-semi.c @@ -27,6 +27,7 @@ #include <time.h> #include "cpu.h" +#include "exec/semihost.h" #ifdef CONFIG_USER_ONLY #include "qemu.h" @@ -440,10 +441,7 @@ uint32_t do_arm_semihosting(CPUARMState *env) input_size = arg1; /* Compute the size of the output string. */ #if !defined(CONFIG_USER_ONLY) - output_size = strlen(ts->boot_info->kernel_filename) - + 1 /* Separating space. */ - + strlen(ts->boot_info->kernel_cmdline) - + 1; /* Terminating null byte. */ + output_size = strlen(semihosting_get_cmdline()) + 1; #else unsigned int i; @@ -474,9 +472,7 @@ uint32_t do_arm_semihosting(CPUARMState *env) /* Copy the command-line arguments. */ #if !defined(CONFIG_USER_ONLY) - pstrcpy(output_buffer, output_size, ts->boot_info->kernel_filename); - pstrcat(output_buffer, output_size, " "); - pstrcat(output_buffer, output_size, ts->boot_info->kernel_cmdline); + pstrcpy(output_buffer, output_size, semihosting_get_cmdline()); #else if (output_size == 1) { /* Empty command-line. */ diff --git a/vl.c b/vl.c index f3319a9..d599411 100644 --- a/vl.c +++ b/vl.c @@ -486,6 +486,9 @@ static QemuOptsList qemu_semihosting_config_opts = { }, { .name = "target", .type = QEMU_OPT_STRING, + }, { + .name = "arg", + .type = QEMU_OPT_STRING, }, { /* end of list */ } }, @@ -1230,6 +1233,9 @@ static void configure_msg(QemuOpts *opts) typedef struct SemihostingConfig { bool enabled; SemihostingTarget target; + const char **argv; + int argc; + const char *cmdline; /* concatenated argv */ } SemihostingConfig; static SemihostingConfig semihosting; @@ -1244,6 +1250,56 @@ SemihostingTarget semihosting_get_target(void) return semihosting.target; } +const char *semihosting_get_arg(int i) +{ + if (i >= semihosting.argc) { + return NULL; + } + return semihosting.argv[i]; +} + +int semihosting_get_argc(void) +{ + return semihosting.argc; +} + +const char *semihosting_get_cmdline(void) +{ + if (semihosting.cmdline == NULL && semihosting.argc > 0) { + semihosting.cmdline = g_strjoinv(" ", (gchar **)semihosting.argv); + } + return semihosting.cmdline; +} + +static int add_semihosting_arg(const char *name, const char *val, void *opaque) +{ + SemihostingConfig *s = opaque; + if (strcmp(name, "arg") == 0) { + s->argc++; + /* one extra element as g_strjoinv() expects NULL-terminated array */ + s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); + s->argv[s->argc - 1] = val; + s->argv[s->argc] = NULL; + } + return 0; +} + +/* Use strings passed via -kernel/-append to initialize semihosting.argv[] */ +static inline void semihosting_arg_fallback(const char *file, const char *cmd) +{ + char *cmd_token; + + /* argv[0] */ + add_semihosting_arg("arg", file, &semihosting); + + /* split -append and initialize argv[1..n] */ + cmd_token = strtok(g_strdup(cmd), " "); + while (cmd_token) { + add_semihosting_arg("arg", cmd_token, &semihosting); + cmd_token = strtok(NULL, " "); + } +} + /***********************************************************/ /* USB devices */ @@ -3592,6 +3648,9 @@ int main(int argc, char **argv, char **envp) } else { semihosting.target = SEMIHOSTING_TARGET_AUTO; } + /* Set semihosting argument count and vector */ + qemu_opt_foreach(opts, add_semihosting_arg, + &semihosting, 0); } else { fprintf(stderr, "Unsupported semihosting-config %s\n", optarg); @@ -4150,6 +4209,11 @@ int main(int argc, char **argv, char **envp) exit(1); } + if (semihosting_enabled() && !semihosting_get_argc() && kernel_filename) { + /* fall back to the -kernel/-append */ + semihosting_arg_fallback(kernel_filename, kernel_cmdline); + } + os_set_line_buffering(); #ifdef CONFIG_SPICE
Add new "arg" sub-argument to the --semihosting-config allowing the user to pass multiple input arguments separately. It is required for example by UHI semihosting to construct argc and argv. Also, update ARM semihosting to support new option (at the moment it is the only target which cares about arguments). If the semihosting is enabled and no semihosting args have been specified, then fall back to -kernel/-append. The -append string is split on whitespace before initializing semihosting.argv[1..n]; this is different from what QEMU MIPS machines' pseudo-bootloaders do (i.e. argv[1] contains the whole -append), but is more intuitive from UHI user's point of view and Linux kernel just does not care as it concatenates argv[1..n] into single cmdline string anyway. Signed-off-by: Leon Alrae <leon.alrae@imgtec.com> --- include/exec/semihost.h | 18 ++++++++++++++ qemu-options.hx | 21 ++++++++++++---- target-arm/arm-semi.c | 10 +++----- vl.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 12 deletions(-)