Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-11 Thread Alex Bennée 


Peter Maydell  writes:

> On Fri, 10 May 2019 at 17:59, Alex Bennée  wrote:
>>
>>
>> Peter Maydell  writes:
>>
>> > On Thu, 9 May 2019 at 17:59, Alex Bennée  wrote:
>> >>
>> >> For running system tests we want to be able to re-direct output to a
>> >> file like we do with serial output. This does the wiring to allow us
>> >> to treat semihosting like just another character output device.
>> >>
>> >> diff --git a/qemu-options.hx b/qemu-options.hx
>> >> index 51802cbb266..6aa3a08c2fb 100644
>> >> --- a/qemu-options.hx
>> >> +++ b/qemu-options.hx
>> >> @@ -3975,12 +3975,12 @@ STEXI
>> >>  Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only).
>> >>  ETEXI
>> >>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
>> >> -"-semihosting-config 
>> >> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
>> >> +"-semihosting-config 
>> >> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
>> >>  "semihosting configuration\n",
>> >>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 |
>> >>  QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2)
>> >
>> > As you can see in the docs here, semihosting is supported on
>> > five guest architectures, so we should implement this new
>> > feature for all of them, not just arm.
>>
>> As I've introduced this for testing I see no reason not to add support
>> for other architectures. However I was hoping this is something that
>> could be done organically as other system tests get enabled.
>
> IME transitions done "organically" really means "slowly, and
> nobody ever gets round to actually completing them".
> Semihosting is a user-facing feature, so if we want to add
> the user feature of allowing output to go to a chardev we
> should add it properly, I think.

So a quick review of the current semi output:

  - MIPS
This has a fairly generalised open/read/write support with special
handling for open/close on /dev/std[out/err/in]. There is also a
UHI_plog which currently just printf's to stdout

  - xtensa
This already has support for a sim_console char device as part of
the xtensa sim platform. Otherwise the TARGET_SYS_open can open
paths directly (which I assume could include stdio) which then
read/write.

  - m68k
This has the usual open/read/write/close support directly to the
FD's as well as support for integrating with the gdbstub via
gdb_do_syscall.

  - lm32
Although based on the m68k semithosting support it lacks the gdbstub
integration. It has the usual open/read/write/close stuff.

  - NIOS2
Again based on the m68k semihosting but looks like it was taken
later because it retains the gdbsub integration support.

Generally all the other semihosting stuff looks a lot cleaner - probably
an indication of being done later and avoiding some of the warts of the
early arm semihosting code.

One difference with ARM is it has specific calls aside from the
open/read/write/close (WRITEC/WRITE0) which are specifically aimed at
"console" type logging. They don't seem to require an explicit open at
the start and assume you can write to them from the get go.

One question that would need to be answered is should the chardev
support be generalised for all semihosts that can read/write to the
stdio outputs or should we restrict it to the "console" log operations
(xtensa sim, mips plog and ARM)?

--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-10 Thread Alex Bennée 


Peter Maydell  writes:

> On Thu, 9 May 2019 at 17:59, Alex Bennée  wrote:
>>
>> For running system tests we want to be able to re-direct output to a
>> file like we do with serial output. This does the wiring to allow us
>> to treat semihosting like just another character output device.
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 51802cbb266..6aa3a08c2fb 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3975,12 +3975,12 @@ STEXI
>>  Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only).
>>  ETEXI
>>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
>> -"-semihosting-config 
>> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
>> +"-semihosting-config 
>> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
>>  "semihosting configuration\n",
>>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 |
>>  QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2)
>
> As you can see in the docs here, semihosting is supported on
> five guest architectures, so we should implement this new
> feature for all of them, not just arm.

As I've introduced this for testing I see no reason not to add support
for other architectures. However I was hoping this is something that
could be done organically as other system tests get enabled.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-10 Thread Peter Maydell 
On Fri, 10 May 2019 at 17:59, Alex Bennée  wrote:
>
>
> Peter Maydell  writes:
>
> > On Thu, 9 May 2019 at 17:59, Alex Bennée  wrote:
> >>
> >> For running system tests we want to be able to re-direct output to a
> >> file like we do with serial output. This does the wiring to allow us
> >> to treat semihosting like just another character output device.
> >>
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 51802cbb266..6aa3a08c2fb 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -3975,12 +3975,12 @@ STEXI
> >>  Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only).
> >>  ETEXI
> >>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
> >> -"-semihosting-config 
> >> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
> >> +"-semihosting-config 
> >> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
> >>  "semihosting configuration\n",
> >>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 |
> >>  QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2)
> >
> > As you can see in the docs here, semihosting is supported on
> > five guest architectures, so we should implement this new
> > feature for all of them, not just arm.
>
> As I've introduced this for testing I see no reason not to add support
> for other architectures. However I was hoping this is something that
> could be done organically as other system tests get enabled.

IME transitions done "organically" really means "slowly, and
nobody ever gets round to actually completing them".
Semihosting is a user-facing feature, so if we want to add
the user feature of allowing output to go to a chardev we
should add it properly, I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-10 Thread Peter Maydell 
On Thu, 9 May 2019 at 17:59, Alex Bennée  wrote:
>
> For running system tests we want to be able to re-direct output to a
> file like we do with serial output. This does the wiring to allow us
> to treat semihosting like just another character output device.
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 51802cbb266..6aa3a08c2fb 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3975,12 +3975,12 @@ STEXI
>  Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only).
>  ETEXI
>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
> -"-semihosting-config 
> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
> +"-semihosting-config 
> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
>  "semihosting configuration\n",
>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 |
>  QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2)

As you can see in the docs here, semihosting is supported on
five guest architectures, so we should implement this new
feature for all of them, not just arm.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-10 Thread Peter Maydell 
On Fri, 10 May 2019 at 15:05, Alex Bennée  wrote:
> Only for the first one though.. that said I'm sure the write string is
> leaking when we do gdb output with whatever lock_user_string is trying
> to achieve.

Yes, there looks like there's a leak there. (The fix is
complicated because we need to check whether the string
buffer is required to hang around until the asynchronous
gdb operation is finished and the arm_semi_cb is invoked,
or whether we can free it as soon as arm_gdb_syscall() returns.)

lock_user_string is basically "give me a host pointer to the
string at this address in guest memory":
 * on softmmu, the 'lock' operation copies the contents of
   guest memory into a local buffer, and 'unlock' then frees
   the buffer (possibly copying the updated local buffer
   contents back to the guest)
 * on linux-user, 'lock' does the guest-addr-to-host-addr
   conversion, and if DEBUG_REMAP is defined then it will
   also copy it into a separate buffer (and unlock will
   copy it back)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-10 Thread Alex Bennée 


Richard Henderson  writes:

> On 5/9/19 11:55 PM, Alex Bennée wrote:
>>
>> Richard Henderson  writes:
>>
>>> On 5/9/19 9:58 AM, Alex Bennée wrote:
 @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void)
  {
  return NULL;
  }
 +
 +static inline Chardev *semihosting_get_chardev(void)
 +{
 +return NULL;
 +}
>>>
>>> Isn't the point of this function to avoid...
>>
>> Yes... but...
>>
>>>
 -return write(STDERR_FILENO, &c, 1);
 +#ifdef CONFIG_SOFTMMU
 +  Chardev *chardev = semihosting_get_chardev();
 +  if (chardev) {
 +  return qemu_chr_write_all(chardev, (uint8_t *) &c,
>>> 1);
>>
>> The qemu_chr device stuff doesn't have such stubs and is softmmu only as
>> well. *sigh*
>
> Ah, I see.  Yes that's a problem.
>
> Well at least you don't need the "else\n#endif\n{" ugliness.  You have the
> return out of the then block.

Only for the first one though.. that said I'm sure the write string is
leaking when we do gdb output with whatever lock_user_string is trying
to achieve.

>
>
> r~


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-10 Thread Richard Henderson 
On 5/9/19 11:55 PM, Alex Bennée wrote:
> 
> Richard Henderson  writes:
> 
>> On 5/9/19 9:58 AM, Alex Bennée wrote:
>>> @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void)
>>>  {
>>>  return NULL;
>>>  }
>>> +
>>> +static inline Chardev *semihosting_get_chardev(void)
>>> +{
>>> +return NULL;
>>> +}
>>
>> Isn't the point of this function to avoid...
> 
> Yes... but...
> 
>>
>>> -return write(STDERR_FILENO, &c, 1);
>>> +#ifdef CONFIG_SOFTMMU
>>> +  Chardev *chardev = semihosting_get_chardev();
>>> +  if (chardev) {
>>> +  return qemu_chr_write_all(chardev, (uint8_t *) &c,
>> 1);
> 
> The qemu_chr device stuff doesn't have such stubs and is softmmu only as
> well. *sigh*

Ah, I see.  Yes that's a problem.

Well at least you don't need the "else\n#endif\n{" ugliness.  You have the
return out of the then block.


r~

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-09 Thread Alex Bennée 


Richard Henderson  writes:

> On 5/9/19 9:58 AM, Alex Bennée wrote:
>> @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void)
>>  {
>>  return NULL;
>>  }
>> +
>> +static inline Chardev *semihosting_get_chardev(void)
>> +{
>> +return NULL;
>> +}
>
> Isn't the point of this function to avoid...

Yes... but...

>
>> -return write(STDERR_FILENO, &c, 1);
>> +#ifdef CONFIG_SOFTMMU
>> +  Chardev *chardev = semihosting_get_chardev();
>> +  if (chardev) {
>> +  return qemu_chr_write_all(chardev, (uint8_t *) &c,
>1);

The qemu_chr device stuff doesn't have such stubs and is softmmu only as
well. *sigh*

I guess stub it out in the header as well?

>> +  } else
>> +#endif
>> +  {
>> +  return write(STDERR_FILENO, &c, 1);
>> +  }
>
> ... this ifdef?
>
> Better to change
>
> - char c;
> + uint8_t c;
>
> above to avoid the cast in the call to qemu_chr_write_all?
> Or perhaps we should adjust qemu_chr_write_all to take void*?
>
>
> r~


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-09 Thread Richard Henderson 
On 5/9/19 9:58 AM, Alex Bennée wrote:
> @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void)
>  {
>  return NULL;
>  }
> +
> +static inline Chardev *semihosting_get_chardev(void)
> +{
> +return NULL;
> +}

Isn't the point of this function to avoid...

> -return write(STDERR_FILENO, &c, 1);
> +#ifdef CONFIG_SOFTMMU
> +  Chardev *chardev = semihosting_get_chardev();
> +  if (chardev) {
> +  return qemu_chr_write_all(chardev, (uint8_t *) &c, 1);
> +  } else
> +#endif
> +  {
> +  return write(STDERR_FILENO, &c, 1);
> +  }

... this ifdef?

Better to change

- char c;
+ uint8_t c;

above to avoid the cast in the call to qemu_chr_write_all?
Or perhaps we should adjust qemu_chr_write_all to take void*?


r~

[Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

2019-05-09 Thread Alex Bennée 
For running system tests we want to be able to re-direct output to a
file like we do with serial output. This does the wiring to allow us
to treat semihosting like just another character output device.

Signed-off-by: Alex Bennée 
---
 include/exec/semihost.h |  6 ++
 qemu-options.hx |  6 --
 target/arm/arm-semi.c   | 21 +++--
 vl.c| 23 +++
 4 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/include/exec/semihost.h b/include/exec/semihost.h
index 5980939c7b8..f5cc9ad2759 100644
--- a/include/exec/semihost.h
+++ b/include/exec/semihost.h
@@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void)
 {
 return NULL;
 }
+
+static inline Chardev *semihosting_get_chardev(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);
+Chardev *semihosting_get_chardev(void);
 #endif
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 51802cbb266..6aa3a08c2fb 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3975,12 +3975,12 @@ STEXI
 Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only).
 ETEXI
 DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
-"-semihosting-config 
[enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
+"-semihosting-config 
[enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
 "semihosting configuration\n",
 QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 |
 QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2)
 STEXI
-@item -semihosting-config 
[enable=on|off][,target=native|gdb|auto][,arg=str[,...]]
+@item -semihosting-config 
[enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]
 @findex -semihosting-config
 Enable and configure semihosting (ARM, M68K, Xtensa, MIPS, Nios II only).
 @table @option
@@ -3988,6 +3988,8 @@ Enable and configure semihosting (ARM, M68K, Xtensa, 
MIPS, Nios II only).
 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 chardev=@var{str1}
+Send the output to a chardev backend output for native or auto output when not 
in gdb
 @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
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 8b5fd7bc6e3..4c326fdc2fb 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -32,6 +32,7 @@
 #include "hw/arm/arm.h"
 #include "qemu/cutils.h"
 #endif
+#include "chardev/char.h"
 
 #define TARGET_SYS_OPEN0x01
 #define TARGET_SYS_CLOSE   0x02
@@ -310,7 +311,15 @@ target_ulong do_arm_semihosting(CPUARMState *env)
   if (use_gdb_syscalls()) {
 return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", args);
   } else {
-return write(STDERR_FILENO, &c, 1);
+#ifdef CONFIG_SOFTMMU
+  Chardev *chardev = semihosting_get_chardev();
+  if (chardev) {
+  return qemu_chr_write_all(chardev, (uint8_t *) &c, 1);
+  } else
+#endif
+  {
+  return write(STDERR_FILENO, &c, 1);
+  }
   }
 }
 case TARGET_SYS_WRITE0:
@@ -322,7 +331,15 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x",
args, len);
 } else {
-ret = write(STDERR_FILENO, s, len);
+#ifdef CONFIG_SOFTMMU
+Chardev *chardev = semihosting_get_chardev();
+if (chardev) {
+ret = qemu_chr_write_all(chardev, (uint8_t *) s, len);
+} else
+#endif
+{
+ret = write(STDERR_FILENO, s, len);
+}
 }
 unlock_user(s, args, 0);
 return ret;
diff --git a/vl.c b/vl.c
index b6709514c1b..34bbb4df865 100644
--- a/vl.c
+++ b/vl.c
@@ -511,6 +511,9 @@ static QemuOptsList qemu_semihosting_config_opts = {
 }, {
 .name = "target",
 .type = QEMU_OPT_STRING,
+}, {
+.name = "chardev",
+.type = QEMU_OPT_STRING,
 }, {
 .name = "arg",
 .type = QEMU_OPT_STRING,
@@ -1356,6 +1359,7 @@ static void configure_msg(QemuOpts *opts)
 typedef struct SemihostingConfig {
 bool enabled;
 SemihostingTarget target;
+Chardev *chardev;
 const char **argv;
 int argc;
 const char *cmdline; /* concatenated argv */
@@ -1386,6 +1390,11 @@ int semihosting_get_argc(void)
 return semihosting.argc;
 }
 
+Chardev *semihosting