Re: [PATCH] Semihost SYS_READC implementation (v4)

[Alex Bennée]

Keith Packard <address@hidden> writes:

> Provides a blocking call to read a character from the console using
> semihosting.chardev, if specified. This takes some careful command
> line options to use stdio successfully as the serial ports, monitor
> and semihost all want to use stdio. Here's a sample set of command
> line options which share stdio betwen semihost, monitor and serial
> ports:
>
>       qemu \
>       -chardev stdio,mux=on,id=stdio0 \
>       -serial chardev:stdio0 \
>       -semihosting-config enable=on,chardev=stdio0 \
>       -mon chardev=stdio0,mode=readline

I can see the use for this but I'd like to know what you are testing
with. We only have very basic smoketests in check-tcg but I've tested
with the latest arm-semihosting tests and they are all fine so no
regressions there.

>
> This creates a chardev hooked to stdio and then connects all of the
> subsystems to it. A shorter mechanism would be good to hear about.

Please keep version history bellow --- so they get dropped when the
patch is applied.

>
> v2:
>       Add implementation in linux-user/arm/semihost.c
>
> v3:  (thanks to Paolo Bonzini <address@hidden>)
>       Replace hand-rolled fifo with fifo8
>       Avoid mixing code and declarations
>       Remove spurious (void) cast of function parameters
>       Define qemu_semihosting_console_init when CONFIG_USER_ONLY
>
> v4:
>       Add qemu_semihosting_console_init to stubs/semihost.c for
>       hosts that don't support semihosting

This doesn't appear to be in the diff which is why I'm seeing a compile
failure for non-CONFIG_SEMIHOST machines. However...

>
> Signed-off-by: Keith Packard <address@hidden>
> ---
>  hw/semihosting/console.c          | 73 +++++++++++++++++++++++++++++++
>  include/hw/semihosting/console.h  | 12 +++++
>  include/hw/semihosting/semihost.h |  4 ++
>  linux-user/arm/semihost.c         | 24 ++++++++++
>  target/arm/arm-semi.c             |  3 +-
>  vl.c                              |  3 ++
>  6 files changed, 117 insertions(+), 2 deletions(-)
>
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index b4b17c8afb..197bff079b 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -98,3 +98,76 @@ void qemu_semihosting_console_outc(CPUArchState *env, 
> target_ulong addr)
>                        __func__, addr);
>      }
>  }
> +
> +#include <pthread.h>
> +#include "chardev/char-fe.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/main-loop.h"
> +#include "qapi/error.h"
> +#include "qemu/fifo8.h"
> +
> +#define FIFO_SIZE   1024
> +
> +typedef struct SemihostingConsole {
> +    CharBackend         backend;
> +    pthread_mutex_t     mutex;
> +    pthread_cond_t      cond;
> +    bool                got;
> +    Fifo8               fifo;
> +} SemihostingConsole;
> +
> +static SemihostingConsole console = {
> +    .mutex = PTHREAD_MUTEX_INITIALIZER,
> +    .cond = PTHREAD_COND_INITIALIZER
> +};
> +
> +static int console_can_read(void *opaque)
> +{
> +    SemihostingConsole *c = opaque;
> +    int ret;
> +    pthread_mutex_lock(&c->mutex);
> +    ret = (int) fifo8_num_free(&c->fifo);
> +    pthread_mutex_unlock(&c->mutex);
> +    return ret;
> +}
> +
> +static void console_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    SemihostingConsole *c = opaque;
> +    pthread_mutex_lock(&c->mutex);
> +    while (size-- && !fifo8_is_full(&c->fifo)) {
> +        fifo8_push(&c->fifo, *buf++);
> +    }
> +    pthread_cond_broadcast(&c->cond);
> +    pthread_mutex_unlock(&c->mutex);
> +}
> +
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> +    uint8_t ch;
> +    SemihostingConsole *c = &console;
> +    qemu_mutex_unlock_iothread();
> +    pthread_mutex_lock(&c->mutex);
> +    while (fifo8_is_empty(&c->fifo)) {
> +        pthread_cond_wait(&c->cond, &c->mutex);
> +    }
> +    ch = fifo8_pop(&c->fifo);
> +    pthread_mutex_unlock(&c->mutex);
> +    qemu_mutex_lock_iothread();
> +    return (target_ulong) ch;
> +}
> +
> +void qemu_semihosting_console_init(void)
> +{
> +    Chardev *chr = semihosting_get_chardev();
> +
> +    if  (chr) {
> +        fifo8_create(&console.fifo, FIFO_SIZE);
> +        qemu_chr_fe_init(&console.backend, chr, &error_abort);
> +        qemu_chr_fe_set_handlers(&console.backend,
> +                                 console_can_read,
> +                                 console_read,
> +                                 NULL, NULL, &console,
> +                                 NULL, true);
> +    }
> +}
> diff --git a/include/hw/semihosting/console.h 
> b/include/hw/semihosting/console.h
> index 9be9754bcd..f7d5905b41 100644
> --- a/include/hw/semihosting/console.h
> +++ b/include/hw/semihosting/console.h
> @@ -37,6 +37,18 @@ int qemu_semihosting_console_outs(CPUArchState *env, 
> target_ulong s);
>   */
>  void qemu_semihosting_console_outc(CPUArchState *env, target_ulong c);
>
> +/**
> + * qemu_semihosting_console_inc:
> + * @env: CPUArchState
> + *
> + * Receive single character from debug console. This
> + * may be the remote gdb session if a softmmu guest is currently being
> + * debugged.
> + *
> + * Returns: character read or -1 on error
> + */
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env);
> +
>  /**
>   * qemu_semihosting_log_out:
>   * @s: pointer to string
> diff --git a/include/hw/semihosting/semihost.h 
> b/include/hw/semihosting/semihost.h
> index 60fc42d851..b8ce5117ae 100644
> --- a/include/hw/semihosting/semihost.h
> +++ b/include/hw/semihosting/semihost.h
> @@ -56,6 +56,9 @@ static inline Chardev *semihosting_get_chardev(void)
>  {
>      return NULL;
>  }
> +static inline void qemu_semihosting_console_init(void)
> +{
> +}
>  #else /* !CONFIG_USER_ONLY */
>  bool semihosting_enabled(void);
>  SemihostingTarget semihosting_get_target(void);
> @@ -68,6 +71,7 @@ Chardev *semihosting_get_chardev(void);
>  void qemu_semihosting_enable(void);
>  int qemu_semihosting_config_options(const char *opt);
>  void qemu_semihosting_connect_chardevs(void);
> +void qemu_semihosting_console_init(void);
>  #endif /* CONFIG_USER_ONLY */
>
>  #endif /* SEMIHOST_H */
> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
> index a16b525eec..13a097515b 100644
> --- a/linux-user/arm/semihost.c
> +++ b/linux-user/arm/semihost.c
> @@ -47,3 +47,27 @@ void qemu_semihosting_console_outc(CPUArchState *env, 
> target_ulong addr)
>          }
>      }
>  }
> +
> +#include <poll.h>

Headers should go at the top...I was about to discuss the usage of
poll() but I realise we are in linux-user here so non-POSIX portability
isn't an issue.

> +
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> +    uint8_t c;
> +    struct pollfd pollfd = {
> +        .fd = STDIN_FILENO,
> +        .events = POLLIN
> +    };
> +
> +    if (poll(&pollfd, 1, -1) != 1) {
> +        qemu_log_mask(LOG_UNIMP, "%s: unexpected read from stdin failure",
> +                      __func__);
> +        return (target_ulong) -1;
> +    }
> +
> +    if (read(STDIN_FILENO, &c, 1) != 1) {
> +        qemu_log_mask(LOG_UNIMP, "%s: unexpected read from stdin failure",
> +                      __func__);
> +        return (target_ulong) -1;
> +    }
> +    return (target_ulong) c;
> +}
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 6f7b6d801b..47d61f6fe1 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -802,8 +802,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>
>          return guestfd_fns[gf->type].readfn(cpu, gf, arg1, len);
>      case TARGET_SYS_READC:
> -        qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
> -        return 0;
> +        return qemu_semihosting_console_inc(env);

I'm not sure this would be correct if there was no character available.
The docs imply it blocks although don't say so explicitly AFAICT.

>      case TARGET_SYS_ISTTY:
>          GET_ARG(0);
>
> diff --git a/vl.c b/vl.c
> index 4489cfb2bb..ac584d97ea 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4381,6 +4381,9 @@ int main(int argc, char **argv, char **envp)
>      ds = init_displaystate();
>      qemu_display_init(ds, &dpy);
>
> +    /* connect semihosting console input if requested */
> +    qemu_semihosting_console_init();
> +

I'd rather rename qemu_semihosting_connect_chardevs to
qemu_semihosting_init and keep all our bits of semihosting setup in
there rather than having multiple calls out of vl.c

>      /* must be after terminal init, SDL library changes signal handlers */
>      os_setup_signal_handling();


--
Alex Bennée