diff mbox

[v3,2/2] semihosting: add --semihosting-config arg sub-argument

Message ID 1431085311-24617-3-git-send-email-leon.alrae@imgtec.com
State New
Headers show

Commit Message

Leon Alrae May 8, 2015, 11:41 a.m. UTC
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(-)

Comments

Peter Maydell May 18, 2015, 4:18 p.m. UTC | #1
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
Leon Alrae May 20, 2015, 8:11 a.m. UTC | #2
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
Liviu Ionescu May 20, 2015, 8:30 a.m. UTC | #3
> 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
Leon Alrae May 20, 2015, 8:51 a.m. UTC | #4
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
Peter Maydell May 20, 2015, 8:54 a.m. UTC | #5
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
Leon Alrae May 20, 2015, 9:31 a.m. UTC | #6
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
Liviu Ionescu May 20, 2015, 11:12 a.m. UTC | #7
> 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
Leon Alrae May 20, 2015, 1:10 p.m. UTC | #8
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
Liviu Ionescu May 20, 2015, 1:49 p.m. UTC | #9
> 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
Peter Maydell May 20, 2015, 2:18 p.m. UTC | #10
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
Liviu Ionescu May 20, 2015, 2:31 p.m. UTC | #11
> 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
Peter Maydell May 20, 2015, 2:40 p.m. UTC | #12
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
Liviu Ionescu May 20, 2015, 2:59 p.m. UTC | #13
> 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
Peter Maydell May 20, 2015, 3:11 p.m. UTC | #14
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
Liviu Ionescu May 20, 2015, 3:47 p.m. UTC | #15
> 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
Leon Alrae May 21, 2015, 1:57 p.m. UTC | #16
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
Peter Maydell May 21, 2015, 2:01 p.m. UTC | #17
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
Leon Alrae May 21, 2015, 2:26 p.m. UTC | #18
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
Liviu Ionescu May 21, 2015, 2:28 p.m. UTC | #19
> 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.
Peter Maydell May 21, 2015, 2:33 p.m. UTC | #20
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
Liviu Ionescu May 21, 2015, 2:57 p.m. UTC | #21
> 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
Peter Maydell May 21, 2015, 3:06 p.m. UTC | #22
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
Liviu Ionescu May 21, 2015, 3:24 p.m. UTC | #23
> 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
Peter Maydell May 21, 2015, 3:29 p.m. UTC | #24
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
Liviu Ionescu May 21, 2015, 3:47 p.m. UTC | #25
> 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
Peter Maydell May 21, 2015, 3:54 p.m. UTC | #26
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
Liviu Ionescu May 21, 2015, 4:36 p.m. UTC | #27
> 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
Peter Maydell May 21, 2015, 4:56 p.m. UTC | #28
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
Liviu Ionescu May 21, 2015, 5:11 p.m. UTC | #29
> 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
Maciej W. Rozycki May 21, 2015, 6:35 p.m. UTC | #30
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
Liviu Ionescu May 21, 2015, 6:58 p.m. UTC | #31
> 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
diff mbox

Patch

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);