Keith Packard <kei...@keithp.com> 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 <pbonz...@redhat.com>)
> 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 <kei...@keithp.com>
> ---
> 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
