diff mbox series

[v1,05/23] semihosting: enable chardev backed output

Message ID 20190509165912.10512-6-alex.bennee@linaro.org
State New
Headers show
Series current testing/next queue (docker/system & io tests) | expand

Commit Message

Alex Bennée May 9, 2019, 4:58 p.m. UTC 
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 <alex.bennee@linaro.org>

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

-- 
2.20.1

Comments

Richard Henderson May 9, 2019, 10:48 p.m. UTC | #1 
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~
Alex Bennée May 10, 2019, 6:55 a.m. UTC | #2 
Richard Henderson <richard.henderson@linaro.org> 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
Richard Henderson May 10, 2019, 1:52 p.m. UTC | #3 
On 5/9/19 11:55 PM, Alex Bennée wrote:
> 

> Richard Henderson <richard.henderson@linaro.org> 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~
Alex Bennée May 10, 2019, 2:05 p.m. UTC | #4 
Richard Henderson <richard.henderson@linaro.org> writes:

> On 5/9/19 11:55 PM, Alex Bennée wrote:

>>

>> Richard Henderson <richard.henderson@linaro.org> 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
Peter Maydell May 10, 2019, 2:21 p.m. UTC | #5 
On Fri, 10 May 2019 at 15:05, Alex Bennée <alex.bennee@linaro.org> 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
Peter Maydell May 10, 2019, 2:22 p.m. UTC | #6 
On Thu, 9 May 2019 at 17:59, Alex Bennée <alex.bennee@linaro.org> 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
Alex Bennée May 10, 2019, 4:59 p.m. UTC | #7 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 9 May 2019 at 17:59, Alex Bennée <alex.bennee@linaro.org> 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
Peter Maydell May 10, 2019, 5:02 p.m. UTC | #8 
On Fri, 10 May 2019 at 17:59, Alex Bennée <alex.bennee@linaro.org> wrote:
>

>

> Peter Maydell <peter.maydell@linaro.org> writes:

>

> > On Thu, 9 May 2019 at 17:59, Alex Bennée <alex.bennee@linaro.org> 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
Alex Bennée May 11, 2019, 6:04 p.m. UTC | #9 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 10 May 2019 at 17:59, Alex Bennée <alex.bennee@linaro.org> wrote:

>>

>>

>> Peter Maydell <peter.maydell@linaro.org> writes:

>>

>> > On Thu, 9 May 2019 at 17:59, Alex Bennée <alex.bennee@linaro.org> 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
diff mbox series

Patch

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_OPEN        0x01
 #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_get_chardev(void)
+{
+    return semihosting.chardev;
+}
+
 const char *semihosting_get_cmdline(void)
 {
     if (semihosting.cmdline == NULL && semihosting.argc > 0) {
@@ -3027,6 +3036,7 @@  int main(int argc, char **argv, char **envp)
     int display_remote = 0;
     const char *log_mask = NULL;
     const char *log_file = NULL;
+    const char *semihost_chardev = NULL;
     char *trace_file = NULL;
     ram_addr_t maxram_size;
     uint64_t ram_slots = 0;
@@ -3744,6 +3754,8 @@  int main(int argc, char **argv, char **envp)
                     semihosting.enabled = qemu_opt_get_bool(opts, "enable",
                                                             true);
                     const char *target = qemu_opt_get(opts, "target");
+                    /* setup of chardev is deferred until they are initialised */
+                    semihost_chardev = qemu_opt_get(opts, "chardev");
                     if (target != NULL) {
                         if (strcmp("native", target) == 0) {
                             semihosting.target = SEMIHOSTING_TARGET_NATIVE;
@@ -4277,6 +4289,17 @@  int main(int argc, char **argv, char **envp)
     qemu_opts_foreach(qemu_find_opts("chardev"),
                       chardev_init_func, NULL, &error_fatal);
 
+    /* We had to defer this until chardevs were created */
+    if (semihost_chardev) {
+        Chardev *chr = qemu_chr_find(semihost_chardev);
+        if (chr == NULL) {
+            error_report("semihosting chardev '%s' not found",
+                         semihost_chardev);
+            exit(1);
+        }
+        semihosting.chardev = chr;
+    }
+
 #ifdef CONFIG_VIRTFS
     qemu_opts_foreach(qemu_find_opts("fsdev"),
                       fsdev_init_func, NULL, &error_fatal);