hw/semihosting/console.c | 92 +++++++++++++++++++++++++++++++ include/hw/semihosting/console.h | 12 ++++ include/hw/semihosting/semihost.h | 1 + stubs/semihost.c | 4 ++ target/arm/arm-semi.c | 3 +- vl.c | 3 + 6 files changed, 113 insertions(+), 2 deletions(-)
Provides a blocking call to read a character from the console by hooking
into the console input chain. This happens *after* any uart has hooked in,
so specifying -semihost overrides input to any emulated uarts.
Signed-off-by: Keith Packard <keithp@keithp.com>
---
hw/semihosting/console.c | 92 +++++++++++++++++++++++++++++++
include/hw/semihosting/console.h | 12 ++++
include/hw/semihosting/semihost.h | 1 +
stubs/semihost.c | 4 ++
target/arm/arm-semi.c | 3 +-
vl.c | 3 +
6 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index b4b17c8afb..7345e2cce0 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -98,3 +98,95 @@ 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"
+
+#define FIFO_SIZE 1024
+
+typedef struct SemihostingFifo {
+ unsigned int insert, remove;
+
+ uint8_t fifo[FIFO_SIZE];
+} SemihostingFifo;
+
+#define fifo_insert(f, c) do { \
+ (f)->fifo[(f)->insert] = (c); \
+ (f)->insert = ((f)->insert + 1) & (FIFO_SIZE - 1); \
+} while (0)
+
+#define fifo_remove(f, c) do {\
+ c = (f)->fifo[(f)->remove]; \
+ (f)->remove = ((f)->remove + 1) & (FIFO_SIZE - 1); \
+} while (0)
+
+#define fifo_full(f) ((((f)->insert + 1) & (FIFO_SIZE - 1)) == \
+ (f)->remove)
+#define fifo_empty(f) ((f)->insert == (f)->remove)
+#define fifo_space(f) (((f)->remove - ((f)->insert + 1)) & \
+ (FIFO_SIZE - 1))
+
+typedef struct SemihostingConsole {
+ CharBackend backend;
+ pthread_mutex_t mutex;
+ pthread_cond_t cond;
+ bool got;
+ SemihostingFifo 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 = fifo_space(&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-- && !fifo_full(&c->fifo)) {
+ fifo_insert(&c->fifo, *buf++);
+ }
+ pthread_cond_broadcast(&c->cond);
+ pthread_mutex_unlock(&c->mutex);
+}
+
+target_ulong qemu_semihosting_console_inc(CPUArchState *env)
+{
+ (void) env;
+ SemihostingConsole *c = &console;
+ qemu_mutex_unlock_iothread();
+ pthread_mutex_lock(&c->mutex);
+ while (fifo_empty(&c->fifo)) {
+ pthread_cond_wait(&c->cond, &c->mutex);
+ }
+ uint8_t ch;
+ fifo_remove(&c->fifo, ch);
+ pthread_mutex_unlock(&c->mutex);
+ qemu_mutex_lock_iothread();
+ return (target_ulong) ch;
+}
+
+void qemu_semihosting_console_init(void)
+{
+ if (semihosting_enabled()) {
+ qemu_chr_fe_init(&console.backend, serial_hd(0), &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..56f3606a2a 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_outc:
+ * @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..a13d17c087 100644
--- a/include/hw/semihosting/semihost.h
+++ b/include/hw/semihosting/semihost.h
@@ -68,6 +68,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/stubs/semihost.c b/stubs/semihost.c
index f90589259c..1d8b37f7b2 100644
--- a/stubs/semihost.c
+++ b/stubs/semihost.c
@@ -69,3 +69,7 @@ void semihosting_arg_fallback(const char *file, const char *cmd)
void qemu_semihosting_connect_chardevs(void)
{
}
+
+void qemu_semihosting_console_init(void)
+{
+}
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);
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();
+
/* must be after terminal init, SDL library changes signal handlers */
os_setup_signal_handling();
--
2.23.0
On 22/10/19 05:13, Keith Packard wrote:
> Provides a blocking call to read a character from the console by hooking
> into the console input chain. This happens *after* any uart has hooked in,
> so specifying -semihost overrides input to any emulated uarts.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
I'm a bit confused, why is it not using semihosting_get_chardev? That
would be
-chardev stdio,id=semihost
-semihosting-config on,chardev=semihost
Paolo
> ---
> hw/semihosting/console.c | 92 +++++++++++++++++++++++++++++++
> include/hw/semihosting/console.h | 12 ++++
> include/hw/semihosting/semihost.h | 1 +
> stubs/semihost.c | 4 ++
> target/arm/arm-semi.c | 3 +-
> vl.c | 3 +
> 6 files changed, 113 insertions(+), 2 deletions(-)
>
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index b4b17c8afb..7345e2cce0 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -98,3 +98,95 @@ 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"
> +
> +#define FIFO_SIZE 1024
> +
> +typedef struct SemihostingFifo {
> + unsigned int insert, remove;
> +
> + uint8_t fifo[FIFO_SIZE];
> +} SemihostingFifo;
> +
> +#define fifo_insert(f, c) do { \
> + (f)->fifo[(f)->insert] = (c); \
> + (f)->insert = ((f)->insert + 1) & (FIFO_SIZE - 1); \
> +} while (0)
> +
> +#define fifo_remove(f, c) do {\
> + c = (f)->fifo[(f)->remove]; \
> + (f)->remove = ((f)->remove + 1) & (FIFO_SIZE - 1); \
> +} while (0)
> +
> +#define fifo_full(f) ((((f)->insert + 1) & (FIFO_SIZE - 1)) == \
> + (f)->remove)
> +#define fifo_empty(f) ((f)->insert == (f)->remove)
> +#define fifo_space(f) (((f)->remove - ((f)->insert + 1)) & \
> + (FIFO_SIZE - 1))
> +
> +typedef struct SemihostingConsole {
> + CharBackend backend;
> + pthread_mutex_t mutex;
> + pthread_cond_t cond;
> + bool got;
> + SemihostingFifo 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 = fifo_space(&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-- && !fifo_full(&c->fifo)) {
> + fifo_insert(&c->fifo, *buf++);
> + }
> + pthread_cond_broadcast(&c->cond);
> + pthread_mutex_unlock(&c->mutex);
> +}
> +
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> + (void) env;
> + SemihostingConsole *c = &console;
> + qemu_mutex_unlock_iothread();
> + pthread_mutex_lock(&c->mutex);
> + while (fifo_empty(&c->fifo)) {
> + pthread_cond_wait(&c->cond, &c->mutex);
> + }
> + uint8_t ch;
> + fifo_remove(&c->fifo, ch);
> + pthread_mutex_unlock(&c->mutex);
> + qemu_mutex_lock_iothread();
> + return (target_ulong) ch;
> +}
> +
> +void qemu_semihosting_console_init(void)
> +{
> + if (semihosting_enabled()) {
> + qemu_chr_fe_init(&console.backend, serial_hd(0), &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..56f3606a2a 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_outc:
> + * @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..a13d17c087 100644
> --- a/include/hw/semihosting/semihost.h
> +++ b/include/hw/semihosting/semihost.h
> @@ -68,6 +68,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/stubs/semihost.c b/stubs/semihost.c
> index f90589259c..1d8b37f7b2 100644
> --- a/stubs/semihost.c
> +++ b/stubs/semihost.c
> @@ -69,3 +69,7 @@ void semihosting_arg_fallback(const char *file, const char *cmd)
> void qemu_semihosting_connect_chardevs(void)
> {
> }
> +
> +void qemu_semihosting_console_init(void)
> +{
> +}
> 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);
> 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();
> +
> /* must be after terminal init, SDL library changes signal handlers */
> os_setup_signal_handling();
>
>
Paolo Bonzini <pbonzini@redhat.com> writes:
Thanks so much for looking at this patch.
> I'm a bit confused, why is it not using semihosting_get_chardev? That
> would be
>
> -chardev stdio,id=semihost
> -semihosting-config on,chardev=semihost
Because I didn't realize the semihosting code already had a Chardev
option. Thanks much for pointing it out. I've changed the code to use
the semihosting chardev instead of serial_hd(0). That change was quite
simple:
void qemu_semihosting_console_init(void)
{
- if (semihosting_enabled()) {
- qemu_chr_fe_init(&console.backend, serial_hd(0), &error_abort);
+ Chardev *chr = semihosting_get_chardev();
+
+ if (chr) {
+ qemu_chr_fe_init(&console.backend, chr, &error_abort);
qemu_chr_fe_set_handlers(&console.backend,
console_can_read,
console_read,
(I left the call to qemu_semihosting_console_init() late in the
initialization process so that the semihosting I/O ended up with the
stdio mux focus instead of the monitor)
Your example command line was really helpful in figuring out how to get
this to work. Here's the full command line I ended up using so that
semihost, serial and monitor are all muxed to stdio:
$ qemu-system-arm -chardev stdio,mux=on,id=stdio0 -serial chardev:stdio0 -semihosting-config enable=on,chardev=stdio0 -mon chardev=stdio0,mode=readline
It might be nice if this could be shortened, but it certainly provides
the necessary options to make it all work.
I'll post an updated version of the patch in a while, after waiting to
see if there are any additional comments.
--
-keith
On 22/10/19 20:12, Keith Packard wrote:
> $ qemu-system-arm -chardev stdio,mux=on,id=stdio0 -serial chardev:stdio0 -semihosting-config enable=on,chardev=stdio0 -mon chardev=stdio0,mode=readline
>
> It might be nice if this could be shortened, but it certainly provides
> the necessary options to make it all work.
Yeah, it's not easy to cram all of the possibilities in the command
line, so in general you need two options, one for the backend ("stdio")
and one for the front-end ("semihosting"). In some cases there are
shortcuts that refer the front-end name in the option name ("-serial"),
but semihosting isn't one of those.
There is no particular reason for this, except for the fact that the
older option "-semihosting" was added without an argument many many
years ago.
> I'll post an updated version of the patch in a while, after waiting to
> see if there are any additional comments.
Indeed I have a couple comments, mostly on coding style and duplication:
>
> +typedef struct SemihostingFifo {
> + unsigned int insert, remove;
> +
> + uint8_t fifo[FIFO_SIZE];
> +} SemihostingFifo;
> +
Please take a look at include/qemu/fifo8.h instead of rolling your own
ring buffer. Note that it is not thread-safe so you'll have to keep
that part.
>
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> + (void) env;
No need for this, and also...
> + SemihostingConsole *c = &console;
> + qemu_mutex_unlock_iothread();
> + pthread_mutex_lock(&c->mutex);
> + while (fifo_empty(&c->fifo)) {
> + pthread_cond_wait(&c->cond, &c->mutex);
> + }
> + uint8_t ch;
... we tend not to mix declarations and statements.
> + fifo_remove(&c->fifo, ch);
> + pthread_mutex_unlock(&c->mutex);
> + qemu_mutex_lock_iothread();
> + return (target_ulong) ch;
> +}
> +
Kudos for the unlock/lock_iothread; I am not really familiar with the
semihosting code and I would have naively assumed that it runs without
that lock taken. It is indeed better to have your own mutex, because we
do want to pull the unlock/lock up into do_arm_semihosting or even its
callers.
Thanks,
Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Please take a look at include/qemu/fifo8.h instead of rolling your own > ring buffer. Note that it is not thread-safe so you'll have to keep > that part. Sorry for not looking around sooner, and thanks for the pointer. I've also cleaned up the other issues. > Kudos for the unlock/lock_iothread; I am not really familiar with the > semihosting code and I would have naively assumed that it runs without > that lock taken. I discovered by testing that the semihosting functions were entered with the lock taken :-) I'll post an updated version of this patch to the list shortly. Thanks again for your review; I really appreciate the time you've taken to look at my patch. -- -keith
© 2016 - 2024 Red Hat, Inc.