hw/semihosting/console.c | 72 +++++++++++++++++++++++++++++++ include/hw/semihosting/console.h | 12 ++++++ include/hw/semihosting/semihost.h | 4 ++ linux-user/arm/semihost.c | 23 ++++++++++ stubs/semihost.c | 4 ++ target/arm/arm-semi.c | 3 +- vl.c | 3 ++ 7 files changed, 119 insertions(+), 2 deletions(-)
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
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.
Signed-off-by: Keith Packard <keithp@keithp.com>
---
v2:
Add implementation in linux-user/arm/semihost.c
v3: (thanks to Paolo Bonzini <pbonzini@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
v5:
Move #include statements to the top of the file.
Actually include the stubs/semihost.c patch that was
supposed to be in v4
v6:
Move call to qemu_semihosting_console_init earlier in
main so that the mux starts connected to the serial device
---
hw/semihosting/console.c | 72 +++++++++++++++++++++++++++++++
include/hw/semihosting/console.h | 12 ++++++
include/hw/semihosting/semihost.h | 4 ++
linux-user/arm/semihost.c | 23 ++++++++++
stubs/semihost.c | 4 ++
target/arm/arm-semi.c | 3 +-
vl.c | 3 ++
7 files changed, 119 insertions(+), 2 deletions(-)
diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index b4b17c8afb..4db68d6227 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -22,6 +22,12 @@
#include "exec/gdbstub.h"
#include "qemu/log.h"
#include "chardev/char.h"
+#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"
int qemu_semihosting_log_out(const char *s, int len)
{
@@ -98,3 +104,69 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
__func__, addr);
}
}
+
+#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..4f998d6220 100644
--- a/linux-user/arm/semihost.c
+++ b/linux-user/arm/semihost.c
@@ -14,6 +14,7 @@
#include "cpu.h"
#include "hw/semihosting/console.h"
#include "qemu.h"
+#include <poll.h>
int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
{
@@ -47,3 +48,25 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
}
}
}
+
+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/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..7ea8a907fd 100644
--- a/vl.c
+++ b/vl.c
@@ -4284,6 +4284,9 @@ int main(int argc, char **argv, char **envp)
qemu_opts_foreach(qemu_find_opts("mon"),
mon_init_func, NULL, &error_fatal);
+ /* connect semihosting console input if requested */
+ qemu_semihosting_console_init();
+
if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
exit(1);
if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
--
2.24.0.rc2
Keith Packard <keithp@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
>
> 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.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
>
> ---
>
> v2:
> Add implementation in linux-user/arm/semihost.c
>
> v3: (thanks to Paolo Bonzini <pbonzini@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
>
> v5:
> Move #include statements to the top of the file.
> Actually include the stubs/semihost.c patch that was
> supposed to be in v4
> v6:
> Move call to qemu_semihosting_console_init earlier in
> main so that the mux starts connected to the serial device
> ---
> hw/semihosting/console.c | 72 +++++++++++++++++++++++++++++++
> include/hw/semihosting/console.h | 12 ++++++
> include/hw/semihosting/semihost.h | 4 ++
> linux-user/arm/semihost.c | 23 ++++++++++
> stubs/semihost.c | 4 ++
> target/arm/arm-semi.c | 3 +-
> vl.c | 3 ++
> 7 files changed, 119 insertions(+), 2 deletions(-)
>
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index b4b17c8afb..4db68d6227 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -22,6 +22,12 @@
> #include "exec/gdbstub.h"
> #include "qemu/log.h"
> #include "chardev/char.h"
> +#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"
>
> int qemu_semihosting_log_out(const char *s, int len)
> {
> @@ -98,3 +104,69 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
> __func__, addr);
> }
> }
> +
> +#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;
> +}
I've been trying to exercise this code with a new test case:
https://github.com/stsquad/semihosting-tests/tree/readc-test
But I end up deadlocked. Even worse when I issue quit via the mmio we
end up hanging on something that will never complete:
(gdb) thread apply all bt
Thread 3 (Thread 0x7f8b1959e700 (LWP 14017)):
#0 0x00007f8b2ada900c in futex_wait_cancelable (private=0, expected=0, futex_word=0x56213f5482e8 <console+136>) at ../sysdeps/unix/sysv/linux/futex-internal.h:88
#1 0x00007f8b2ada900c in __pthread_cond_wait_common (abstime=0x0, mutex=0x56213f548298 <console+56>, cond=0x56213f5482c0 <console+96>) at pthread_cond_wait.c:502
#2 0x00007f8b2ada900c in __pthread_cond_wait (cond=cond@entry=0x56213f5482c0 <console+96>, mutex=mutex@entry=0x56213f548298 <console+56>) at pthread_cond_wait.c:655
#3 0x000056213ea31a40 in qemu_semihosting_console_inc (env=env@entry=0x56214138a680) at /home/alex/lsrc/qemu.git/hw/semihosting/console.c:151
#4 0x000056213eab96e8 in do_arm_semihosting (env=env@entry=0x56214138a680) at /home/alex/lsrc/qemu.git/target/arm/arm-semi.c:805
#5 0x000056213eacd521 in handle_semihosting (cs=<optimized out>) at /home/alex/lsrc/qemu.git/target/arm/helper.c:8476
#6 0x000056213eacd521 in arm_cpu_do_interrupt (cs=<optimized out>) at /home/alex/lsrc/qemu.git/target/arm/helper.c:8522
#7 0x000056213e9e53d0 in cpu_handle_exception (ret=<synthetic pointer>, cpu=0x5621411fe2f0) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:503
#8 0x000056213e9e53d0 in cpu_exec (cpu=cpu@entry=0x562141381550) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:711
#9 0x000056213e9b4f1f in tcg_cpu_exec (cpu=0x562141381550) at /home/alex/lsrc/qemu.git/cpus.c:1473
#10 0x000056213e9b715b in qemu_tcg_cpu_thread_fn (arg=arg@entry=0x562141381550) at /home/alex/lsrc/qemu.git/cpus.c:1781
#11 0x000056213ef026fa in qemu_thread_start (args=<optimized out>) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:519
#12 0x00007f8b2ada2fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486
#13 0x00007f8b2acd14cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Thread 2 (Thread 0x7f8b1c012700 (LWP 14016)):
#0 0x00007f8b2accbf59 in syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
#1 0x000056213ef034ab in qemu_futex_wait (val=<optimized out>, f=<optimized out>) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:455
#2 0x000056213ef034ab in qemu_event_wait (ev=ev@entry=0x56213f55ffe0 <rcu_gp_event>) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:459
#3 0x000056213ef14dc7 in wait_for_readers () at /home/alex/lsrc/qemu.git/util/rcu.c:134
#4 0x000056213ef14dc7 in synchronize_rcu () at /home/alex/lsrc/qemu.git/util/rcu.c:170
#5 0x000056213ef1508d in call_rcu_thread (opaque=opaque@entry=0x0) at /home/alex/lsrc/qemu.git/util/rcu.c:267
#6 0x000056213ef026fa in qemu_thread_start (args=<optimized out>) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:519
#7 0x00007f8b2ada2fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486
#8 0x00007f8b2acd14cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Thread 1 (Thread 0x7f8b1c151680 (LWP 14010)):
#0 0x00007f8b2ada900c in futex_wait_cancelable (private=0, expected=0, futex_word=0x56213f52c7c8 <qemu_pause_cond+40>) at ../sysdeps/unix/sysv/linux/futex-internal.h:88
#1 0x00007f8b2ada900c in __pthread_cond_wait_common (abstime=0x0, mutex=0x56213f52c8c0 <qemu_global_mutex>, cond=0x56213f52c7a0 <qemu_pause_cond>) at pthread_cond_wait.c:502
#2 0x00007f8b2ada900c in __pthread_cond_wait (cond=cond@entry=0x56213f52c7a0 <qemu_pause_cond>, mutex=mutex@entry=0x56213f52c8c0 <qemu_global_mutex>) at pthread_cond_wait.c:655
#3 0x000056213ef02e2b in qemu_cond_wait_impl (cond=0x56213f52c7a0 <qemu_pause_cond>, mutex=0x56213f52c8c0 <qemu_global_mutex>, file=0x56213ef43700 "/home/alex/lsrc/qemu.git/cpus.c", line=1943) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:173
#4 0x000056213e9b74a4 in pause_all_vcpus () at /home/alex/lsrc/qemu.git/cpus.c:1943
#5 0x000056213e9b74a4 in pause_all_vcpus () at /home/alex/lsrc/qemu.git/cpus.c:1923
#6 0x000056213e9b7532 in do_vm_stop (state=RUN_STATE_SHUTDOWN, send_stop=<optimized out>) at /home/alex/lsrc/qemu.git/cpus.c:1102
#7 0x000056213e96b8fc in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /home/alex/lsrc/qemu.git/vl.c:4473
I guess my first question is why do we need a separate mutex/cond
variable for this operation? This seems like the sort of thing that the
BQL could protect.
Secondly if the vCPU is paused (via console or gdbstub) we need to
unwind from our blocking position and be in a position to restart
cleanly.
> +
> +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..4f998d6220 100644
> --- a/linux-user/arm/semihost.c
> +++ b/linux-user/arm/semihost.c
> @@ -14,6 +14,7 @@
> #include "cpu.h"
> #include "hw/semihosting/console.h"
> #include "qemu.h"
> +#include <poll.h>
>
> int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
> {
> @@ -47,3 +48,25 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
> }
> }
> }
> +
> +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/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..7ea8a907fd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4284,6 +4284,9 @@ int main(int argc, char **argv, char **envp)
> qemu_opts_foreach(qemu_find_opts("mon"),
> mon_init_func, NULL, &error_fatal);
>
> + /* connect semihosting console input if requested */
> + qemu_semihosting_console_init();
> +
> if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
> exit(1);
> if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
--
Alex Bennée
On 17/12/19 09:38, Alex Bennée wrote: > Thread 3 (Thread 0x7f8b1959e700 (LWP 14017)): > #0 0x00007f8b2ada900c in futex_wait_cancelable (private=0, expected=0, futex_word=0x56213f5482e8 <console+136>) at ../sysdeps/unix/sysv/linux/futex-internal.h:88 > #1 0x00007f8b2ada900c in __pthread_cond_wait_common (abstime=0x0, mutex=0x56213f548298 <console+56>, cond=0x56213f5482c0 <console+96>) at pthread_cond_wait.c:502 > #2 0x00007f8b2ada900c in __pthread_cond_wait (cond=cond@entry=0x56213f5482c0 <console+96>, mutex=mutex@entry=0x56213f548298 <console+56>) at pthread_cond_wait.c:655 > #3 0x000056213ea31a40 in qemu_semihosting_console_inc (env=env@entry=0x56214138a680) at /home/alex/lsrc/qemu.git/hw/semihosting/console.c:151 > #4 0x000056213eab96e8 in do_arm_semihosting (env=env@entry=0x56214138a680) at /home/alex/lsrc/qemu.git/target/arm/arm-semi.c:805 > #5 0x000056213eacd521 in handle_semihosting (cs=<optimized out>) at /home/alex/lsrc/qemu.git/target/arm/helper.c:8476 > #6 0x000056213eacd521 in arm_cpu_do_interrupt (cs=<optimized out>) at /home/alex/lsrc/qemu.git/target/arm/helper.c:8522 > #7 0x000056213e9e53d0 in cpu_handle_exception (ret=<synthetic pointer>, cpu=0x5621411fe2f0) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:503 > #8 0x000056213e9e53d0 in cpu_exec (cpu=cpu@entry=0x562141381550) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:711 > #9 0x000056213e9b4f1f in tcg_cpu_exec (cpu=0x562141381550) at /home/alex/lsrc/qemu.git/cpus.c:1473 > #10 0x000056213e9b715b in qemu_tcg_cpu_thread_fn (arg=arg@entry=0x562141381550) at /home/alex/lsrc/qemu.git/cpus.c:1781 > #11 0x000056213ef026fa in qemu_thread_start (args=<optimized out>) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:519 > #12 0x00007f8b2ada2fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486 > #13 0x00007f8b2acd14cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > Thread 1 (Thread 0x7f8b1c151680 (LWP 14010)): > #0 0x00007f8b2ada900c in futex_wait_cancelable (private=0, expected=0, futex_word=0x56213f52c7c8 <qemu_pause_cond+40>) at ../sysdeps/unix/sysv/linux/futex-internal.h:88 > #1 0x00007f8b2ada900c in __pthread_cond_wait_common (abstime=0x0, mutex=0x56213f52c8c0 <qemu_global_mutex>, cond=0x56213f52c7a0 <qemu_pause_cond>) at pthread_cond_wait.c:502 > #2 0x00007f8b2ada900c in __pthread_cond_wait (cond=cond@entry=0x56213f52c7a0 <qemu_pause_cond>, mutex=mutex@entry=0x56213f52c8c0 <qemu_global_mutex>) at pthread_cond_wait.c:655 > #3 0x000056213ef02e2b in qemu_cond_wait_impl (cond=0x56213f52c7a0 <qemu_pause_cond>, mutex=0x56213f52c8c0 <qemu_global_mutex>, file=0x56213ef43700 "/home/alex/lsrc/qemu.git/cpus.c", line=1943) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:173 > #4 0x000056213e9b74a4 in pause_all_vcpus () at /home/alex/lsrc/qemu.git/cpus.c:1943 > #5 0x000056213e9b74a4 in pause_all_vcpus () at /home/alex/lsrc/qemu.git/cpus.c:1923 > #6 0x000056213e9b7532 in do_vm_stop (state=RUN_STATE_SHUTDOWN, send_stop=<optimized out>) at /home/alex/lsrc/qemu.git/cpus.c:1102 > #7 0x000056213e96b8fc in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /home/alex/lsrc/qemu.git/vl.c:4473 > > I guess my first question is why do we need a separate mutex/cond > variable for this operation? This seems like the sort of thing that the > BQL could protect. No, please do not introduce more uses of the BQL from the CPU thread. The problem seems to lie with the condition variable, not the mutex. > Secondly if the vCPU is paused (via console or gdbstub) we need to > unwind from our blocking position and be in a position to restart > cleanly. Perhaps if fifo8_is_empty(&c->fifo) the CPU could update the PC back to the SVC instruction and enter a halted state? Perhaps with a new CPU_INTERRUPT_* flag that would be checked in arm_cpu_has_work. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 17/12/19 09:38, Alex Bennée wrote: >> Thread 3 (Thread 0x7f8b1959e700 (LWP 14017)): >> #0 0x00007f8b2ada900c in futex_wait_cancelable (private=0, expected=0, futex_word=0x56213f5482e8 <console+136>) at ../sysdeps/unix/sysv/linux/futex-internal.h:88 >> #1 0x00007f8b2ada900c in __pthread_cond_wait_common (abstime=0x0, mutex=0x56213f548298 <console+56>, cond=0x56213f5482c0 <console+96>) at pthread_cond_wait.c:502 >> #2 0x00007f8b2ada900c in __pthread_cond_wait (cond=cond@entry=0x56213f5482c0 <console+96>, mutex=mutex@entry=0x56213f548298 <console+56>) at pthread_cond_wait.c:655 >> #3 0x000056213ea31a40 in qemu_semihosting_console_inc (env=env@entry=0x56214138a680) at /home/alex/lsrc/qemu.git/hw/semihosting/console.c:151 >> #4 0x000056213eab96e8 in do_arm_semihosting (env=env@entry=0x56214138a680) at /home/alex/lsrc/qemu.git/target/arm/arm-semi.c:805 >> #5 0x000056213eacd521 in handle_semihosting (cs=<optimized out>) at /home/alex/lsrc/qemu.git/target/arm/helper.c:8476 >> #6 0x000056213eacd521 in arm_cpu_do_interrupt (cs=<optimized out>) at /home/alex/lsrc/qemu.git/target/arm/helper.c:8522 >> #7 0x000056213e9e53d0 in cpu_handle_exception (ret=<synthetic pointer>, cpu=0x5621411fe2f0) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:503 >> #8 0x000056213e9e53d0 in cpu_exec (cpu=cpu@entry=0x562141381550) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:711 >> #9 0x000056213e9b4f1f in tcg_cpu_exec (cpu=0x562141381550) at /home/alex/lsrc/qemu.git/cpus.c:1473 >> #10 0x000056213e9b715b in qemu_tcg_cpu_thread_fn (arg=arg@entry=0x562141381550) at /home/alex/lsrc/qemu.git/cpus.c:1781 >> #11 0x000056213ef026fa in qemu_thread_start (args=<optimized out>) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:519 >> #12 0x00007f8b2ada2fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486 >> #13 0x00007f8b2acd14cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 >> >> Thread 1 (Thread 0x7f8b1c151680 (LWP 14010)): >> #0 0x00007f8b2ada900c in futex_wait_cancelable (private=0, expected=0, futex_word=0x56213f52c7c8 <qemu_pause_cond+40>) at ../sysdeps/unix/sysv/linux/futex-internal.h:88 >> #1 0x00007f8b2ada900c in __pthread_cond_wait_common (abstime=0x0, mutex=0x56213f52c8c0 <qemu_global_mutex>, cond=0x56213f52c7a0 <qemu_pause_cond>) at pthread_cond_wait.c:502 >> #2 0x00007f8b2ada900c in __pthread_cond_wait (cond=cond@entry=0x56213f52c7a0 <qemu_pause_cond>, mutex=mutex@entry=0x56213f52c8c0 <qemu_global_mutex>) at pthread_cond_wait.c:655 >> #3 0x000056213ef02e2b in qemu_cond_wait_impl (cond=0x56213f52c7a0 <qemu_pause_cond>, mutex=0x56213f52c8c0 <qemu_global_mutex>, file=0x56213ef43700 "/home/alex/lsrc/qemu.git/cpus.c", line=1943) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:173 >> #4 0x000056213e9b74a4 in pause_all_vcpus () at /home/alex/lsrc/qemu.git/cpus.c:1943 >> #5 0x000056213e9b74a4 in pause_all_vcpus () at /home/alex/lsrc/qemu.git/cpus.c:1923 >> #6 0x000056213e9b7532 in do_vm_stop (state=RUN_STATE_SHUTDOWN, send_stop=<optimized out>) at /home/alex/lsrc/qemu.git/cpus.c:1102 >> #7 0x000056213e96b8fc in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /home/alex/lsrc/qemu.git/vl.c:4473 >> >> I guess my first question is why do we need a separate mutex/cond >> variable for this operation? This seems like the sort of thing that the >> BQL could protect. > > No, please do not introduce more uses of the BQL from the CPU thread. > The problem seems to lie with the condition variable, not the mutex. Well in this case we are holding the BQL anyway as we are being called from the interrupt context. The BQL protects all shared HW state outside of MMIO which is explicitly marked as doing it's own locking. That said I don't know if the semihosting console will always be called from a BQL held context. > >> Secondly if the vCPU is paused (via console or gdbstub) we need to >> unwind from our blocking position and be in a position to restart >> cleanly. > > Perhaps if fifo8_is_empty(&c->fifo) the CPU could update the PC back to > the SVC instruction and enter a halted state? Perhaps with a new > CPU_INTERRUPT_* flag that would be checked in arm_cpu_has_work. I don't think the PC has been updated at this point - but we don't want that logic in the common semihosting code. If we cpu_loop_exit the exception is still in effect and will re-run when we start again. What we really want to do is fall back to the same halting semantics that leave us in qemu_wait_io_event until there is something to process. Is there any particular reason a blocking semihosting event isn't like any other IO event? > > Paolo -- Alex Bennée
On 17/12/19 10:51, Alex Bennée wrote: >>> Secondly if the vCPU is paused (via console or gdbstub) we need to >>> unwind from our blocking position and be in a position to restart >>> cleanly. >> Perhaps if fifo8_is_empty(&c->fifo) the CPU could update the PC back to >> the SVC instruction and enter a halted state? Perhaps with a new >> CPU_INTERRUPT_* flag that would be checked in arm_cpu_has_work. > I don't think the PC has been updated at this point - but we don't want > that logic in the common semihosting code. If we cpu_loop_exit the > exception is still in effect and will re-run when we start again. So that would work? cpu_loop_exit if the FIFO is empty, reentering via cpu_interrupt and clearing the interrupt signal in do_arm_semihosting. > What we really want to do is fall back to the same halting semantics > that leave us in qemu_wait_io_event until there is something to process. > Is there any particular reason a blocking semihosting event isn't like > any other IO event? The "io" in wait_io_event really stands for "iothread". Usually in system emulation "waiting for I/O events" means "waiting for an interrupt" with a halt instruction (for ARM, WFE/WFI), hence my suggestion. Thanks, Paolo
Sleeping in the semihosting code is problematic as we deadlock the
whole system. This includes issuing a "quit" via the HMP or presumably
if gdbserver got involved. What we really want is to return to the
main loop and gt woken up when there is data to process. We can then
re-execute the instruction which will succeed this time.
[AJB:
So this at least solves the hang of not being able to quit system
emulation while blocked. However there are two things we still need to
ensure:
- the PC has not advanced until completion so we can redo the instruction
- we actually wake up the CPU in console_read
In my testcase console_read never seems to get called. I've tried with
both an external pipe loopback and using the ringbuf:
qemu-system-aarch64 -M virt --display none -cpu cortex-a57 -kernel systest-a64-with-console.axf -semihosting-config
enable=on,chardev=sh0 -serial mon:stdio -chardev ringbuf,logfile=foo,id=sh0
]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
include/exec/cpu-all.h | 1 +
hw/semihosting/console.c | 34 +++++++++++++++++-----------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index e96781a4559..093d7a76edd 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -31,6 +31,7 @@
#define EXCP_HALTED 0x10003 /* cpu is halted (waiting for external event) */
#define EXCP_YIELD 0x10004 /* cpu wants to yield timeslice to another */
#define EXCP_ATOMIC 0x10005 /* stop-the-world and emulate atomic */
+#define EXCP_BLOCKED 0x10006 /* cpu is blocked (semihosting) */
/* some important defines:
*
diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index 4db68d62270..bda457a0608 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -20,6 +20,7 @@
#include "hw/semihosting/semihost.h"
#include "hw/semihosting/console.h"
#include "exec/gdbstub.h"
+#include "exec/exec-all.h"
#include "qemu/log.h"
#include "chardev/char.h"
#include <pthread.h>
@@ -109,50 +110,49 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
typedef struct SemihostingConsole {
CharBackend backend;
- pthread_mutex_t mutex;
- pthread_cond_t cond;
+ CPUState *sleeping_cpu;
bool got;
Fifo8 fifo;
} SemihostingConsole;
-static SemihostingConsole console = {
- .mutex = PTHREAD_MUTEX_INITIALIZER,
- .cond = PTHREAD_COND_INITIALIZER
-};
+static SemihostingConsole console;
static int console_can_read(void *opaque)
{
SemihostingConsole *c = opaque;
int ret;
- pthread_mutex_lock(&c->mutex);
+ g_assert(qemu_mutex_iothread_locked());
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);
+ g_assert(qemu_mutex_iothread_locked());
while (size-- && !fifo8_is_full(&c->fifo)) {
fifo8_push(&c->fifo, *buf++);
}
- pthread_cond_broadcast(&c->cond);
- pthread_mutex_unlock(&c->mutex);
+ if (c->sleeping_cpu) {
+ cpu_resume(c->sleeping_cpu);
+ }
}
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);
+ g_assert(qemu_mutex_iothread_locked());
+ g_assert(current_cpu);
+ if (fifo8_is_empty(&c->fifo)) {
+ c->sleeping_cpu = current_cpu;
+ c->sleeping_cpu->stop = true;
+ c->sleeping_cpu->exception_index = EXCP_BLOCKED;
+ cpu_loop_exit(c->sleeping_cpu);
+ /* never returns */
}
+ c->sleeping_cpu = NULL;
ch = fifo8_pop(&c->fifo);
- pthread_mutex_unlock(&c->mutex);
- qemu_mutex_lock_iothread();
return (target_ulong) ch;
}
--
2.20.1
On 17/12/19 13:14, Alex Bennée wrote:
> [AJB:
>
> So this at least solves the hang of not being able to quit system
> emulation while blocked. However there are two things we still need to
> ensure:
>
> - the PC has not advanced until completion so we can redo the instruction
> - we actually wake up the CPU in console_read
>
> In my testcase console_read never seems to get called. I've tried with
> both an external pipe loopback and using the ringbuf:
>
> qemu-system-aarch64 -M virt --display none -cpu cortex-a57 -kernel systest-a64-with-console.axf -semihosting-config
> enable=on,chardev=sh0 -serial mon:stdio -chardev ringbuf,logfile=foo,id=sh0
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/exec/cpu-all.h | 1 +
> hw/semihosting/console.c | 34 +++++++++++++++++-----------------
> 2 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index e96781a4559..093d7a76edd 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -31,6 +31,7 @@
> #define EXCP_HALTED 0x10003 /* cpu is halted (waiting for external event) */
> #define EXCP_YIELD 0x10004 /* cpu wants to yield timeslice to another */
> #define EXCP_ATOMIC 0x10005 /* stop-the-world and emulate atomic */
> +#define EXCP_BLOCKED 0x10006 /* cpu is blocked (semihosting) */
>
> /* some important defines:
> *
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index 4db68d62270..bda457a0608 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -20,6 +20,7 @@
> #include "hw/semihosting/semihost.h"
> #include "hw/semihosting/console.h"
> #include "exec/gdbstub.h"
> +#include "exec/exec-all.h"
> #include "qemu/log.h"
> #include "chardev/char.h"
> #include <pthread.h>
> @@ -109,50 +110,49 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
>
> typedef struct SemihostingConsole {
> CharBackend backend;
> - pthread_mutex_t mutex;
> - pthread_cond_t cond;
> + CPUState *sleeping_cpu;
> bool got;
> Fifo8 fifo;
> } SemihostingConsole;
>
> -static SemihostingConsole console = {
> - .mutex = PTHREAD_MUTEX_INITIALIZER,
> - .cond = PTHREAD_COND_INITIALIZER
> -};
> +static SemihostingConsole console;
>
> static int console_can_read(void *opaque)
> {
> SemihostingConsole *c = opaque;
> int ret;
> - pthread_mutex_lock(&c->mutex);
> + g_assert(qemu_mutex_iothread_locked());
> 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);
> + g_assert(qemu_mutex_iothread_locked());
> while (size-- && !fifo8_is_full(&c->fifo)) {
> fifo8_push(&c->fifo, *buf++);
> }
> - pthread_cond_broadcast(&c->cond);
> - pthread_mutex_unlock(&c->mutex);
> + if (c->sleeping_cpu) {
> + cpu_resume(c->sleeping_cpu);
> + }
> }
>
> 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);
> + g_assert(qemu_mutex_iothread_locked());
> + g_assert(current_cpu);
> + if (fifo8_is_empty(&c->fifo)) {
> + c->sleeping_cpu = current_cpu;
> + c->sleeping_cpu->stop = true;
> + c->sleeping_cpu->exception_index = EXCP_BLOCKED;
Why do you need to set exception_index to something other than -1 (using
cpu_loop_exit_noexc for example)?
Using ->stop here is a bit weird, since ->stop is usually related to
pause_all_vcpus.
Paolo
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 17/12/19 13:14, Alex Bennée wrote:
>> [AJB:
>>
>> So this at least solves the hang of not being able to quit system
>> emulation while blocked. However there are two things we still need to
>> ensure:
>>
>> - the PC has not advanced until completion so we can redo the instruction
>> - we actually wake up the CPU in console_read
>>
>> In my testcase console_read never seems to get called. I've tried with
>> both an external pipe loopback and using the ringbuf:
>>
>> qemu-system-aarch64 -M virt --display none -cpu cortex-a57 -kernel systest-a64-with-console.axf -semihosting-config
>> enable=on,chardev=sh0 -serial mon:stdio -chardev ringbuf,logfile=foo,id=sh0
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> include/exec/cpu-all.h | 1 +
>> hw/semihosting/console.c | 34 +++++++++++++++++-----------------
>> 2 files changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index e96781a4559..093d7a76edd 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -31,6 +31,7 @@
>> #define EXCP_HALTED 0x10003 /* cpu is halted (waiting for external event) */
>> #define EXCP_YIELD 0x10004 /* cpu wants to yield timeslice to another */
>> #define EXCP_ATOMIC 0x10005 /* stop-the-world and emulate atomic */
>> +#define EXCP_BLOCKED 0x10006 /* cpu is blocked (semihosting) */
>>
>> /* some important defines:
>> *
>> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
>> index 4db68d62270..bda457a0608 100644
>> --- a/hw/semihosting/console.c
>> +++ b/hw/semihosting/console.c
>> @@ -20,6 +20,7 @@
>> #include "hw/semihosting/semihost.h"
>> #include "hw/semihosting/console.h"
>> #include "exec/gdbstub.h"
>> +#include "exec/exec-all.h"
>> #include "qemu/log.h"
>> #include "chardev/char.h"
>> #include <pthread.h>
>> @@ -109,50 +110,49 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
>>
>> typedef struct SemihostingConsole {
>> CharBackend backend;
>> - pthread_mutex_t mutex;
>> - pthread_cond_t cond;
>> + CPUState *sleeping_cpu;
>> bool got;
>> Fifo8 fifo;
>> } SemihostingConsole;
>>
>> -static SemihostingConsole console = {
>> - .mutex = PTHREAD_MUTEX_INITIALIZER,
>> - .cond = PTHREAD_COND_INITIALIZER
>> -};
>> +static SemihostingConsole console;
>>
>> static int console_can_read(void *opaque)
>> {
>> SemihostingConsole *c = opaque;
>> int ret;
>> - pthread_mutex_lock(&c->mutex);
>> + g_assert(qemu_mutex_iothread_locked());
>> 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);
>> + g_assert(qemu_mutex_iothread_locked());
>> while (size-- && !fifo8_is_full(&c->fifo)) {
>> fifo8_push(&c->fifo, *buf++);
>> }
>> - pthread_cond_broadcast(&c->cond);
>> - pthread_mutex_unlock(&c->mutex);
>> + if (c->sleeping_cpu) {
>> + cpu_resume(c->sleeping_cpu);
>> + }
>> }
>>
>> 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);
>> + g_assert(qemu_mutex_iothread_locked());
>> + g_assert(current_cpu);
>> + if (fifo8_is_empty(&c->fifo)) {
>> + c->sleeping_cpu = current_cpu;
>> + c->sleeping_cpu->stop = true;
>> + c->sleeping_cpu->exception_index = EXCP_BLOCKED;
>
> Why do you need to set exception_index to something other than -1 (using
> cpu_loop_exit_noexc for example)?
If there is no exception to process we won't exit the main loop which we
need to do if we want to wait until there is data to read.
> Using ->stop here is a bit weird, since ->stop is usually related to
> pause_all_vcpus.
Arguably we could come up with a better API to cpu.c but this allows us
to use cpu_resume(c->sleeping_cpu) when waking up rather than hand
rolling our own wake-up mechanism.
>
> Paolo
--
Alex Bennée
On 17/12/19 14:42, Alex Bennée wrote: >> Why do you need to set exception_index to something other than -1 (using >> cpu_loop_exit_noexc for example)? > If there is no exception to process we won't exit the main loop which we > need to do if we want to wait until there is data to read. Okay. >> Using ->stop here is a bit weird, since ->stop is usually related to >> pause_all_vcpus. > > Arguably we could come up with a better API to cpu.c but this allows us > to use cpu_resume(c->sleeping_cpu) when waking up rather than hand > rolling our own wake-up mechanism. But we already have the right wake-up mechanism, which is cpu->halted/cpu_has_work. That also makes it possible to just use EXCP_HALTED instead of adding a new EXCP_BLOCKED. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 17/12/19 14:42, Alex Bennée wrote: >>> Why do you need to set exception_index to something other than -1 (using >>> cpu_loop_exit_noexc for example)? >> If there is no exception to process we won't exit the main loop which we >> need to do if we want to wait until there is data to read. > > Okay. > >>> Using ->stop here is a bit weird, since ->stop is usually related to >>> pause_all_vcpus. >> >> Arguably we could come up with a better API to cpu.c but this allows us >> to use cpu_resume(c->sleeping_cpu) when waking up rather than hand >> rolling our own wake-up mechanism. > > But we already have the right wake-up mechanism, which is > cpu->halted/cpu_has_work. cpu_has_work is a guest function though and semihosting_console is a common hw module. It can't peek into the guests internal state. This all comes back to cpu_thread_is_idle anyway in making our decision about if we do or do not sleep on the halt_cond. > That also makes it possible to just use > EXCP_HALTED instead of adding a new EXCP_BLOCKED. We can certainly use EXCP_HALTED but maybe come up with a common way of entering the state? There seems to be a combination of messing around with special interrupts and direct poking of cs->halted = 1 while setting the exception. Maybe this could finally clear up the #if defined(TARGET_I386) hacking in cpus.c? > > Paolo -- Alex Bennée
On 17/12/19 15:18, Alex Bennée wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 17/12/19 14:42, Alex Bennée wrote: >>>> Why do you need to set exception_index to something other than -1 (using >>>> cpu_loop_exit_noexc for example)? >>> If there is no exception to process we won't exit the main loop which we >>> need to do if we want to wait until there is data to read. >> >> Okay. >> >>>> Using ->stop here is a bit weird, since ->stop is usually related to >>>> pause_all_vcpus. >>> >>> Arguably we could come up with a better API to cpu.c but this allows us >>> to use cpu_resume(c->sleeping_cpu) when waking up rather than hand >>> rolling our own wake-up mechanism. >> >> But we already have the right wake-up mechanism, which is >> cpu->halted/cpu_has_work. > > cpu_has_work is a guest function though and semihosting_console is a > common hw module. It can't peek into the guests internal state. semihosting_console only needs to something like cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). (By the way, the stopped_cpu should probably be a list to mimic the condition variable---for example a GList). > This all > comes back to cpu_thread_is_idle anyway in making our decision about if > we do or do not sleep on the halt_cond. > >> That also makes it possible to just use >> EXCP_HALTED instead of adding a new EXCP_BLOCKED. > > We can certainly use EXCP_HALTED but maybe come up with a common way of > entering the state? There seems to be a combination of messing around > with special interrupts and direct poking of cs->halted = 1 while > setting the exception. Maybe this could finally clear up the #if > defined(TARGET_I386) hacking in cpus.c? If you're talking accel/tcg/cpu-exec.c, that's different; the issue there is that x86 has a kind of warm reset pin that is not equivalent to cpu_reset. Removing that would only entail adding a new member function to CPUClass. Paolo
On 17/12/19 15:18, Alex Bennée wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 17/12/19 14:42, Alex Bennée wrote: >>>> Why do you need to set exception_index to something other than -1 (using >>>> cpu_loop_exit_noexc for example)? >>> If there is no exception to process we won't exit the main loop which we >>> need to do if we want to wait until there is data to read. >> >> Okay. >> >>>> Using ->stop here is a bit weird, since ->stop is usually related to >>>> pause_all_vcpus. >>> >>> Arguably we could come up with a better API to cpu.c but this allows us >>> to use cpu_resume(c->sleeping_cpu) when waking up rather than hand >>> rolling our own wake-up mechanism. >> >> But we already have the right wake-up mechanism, which is >> cpu->halted/cpu_has_work. > > cpu_has_work is a guest function though and semihosting_console is a > common hw module. It can't peek into the guests internal state. semihosting_console only needs to something like cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). (By the way, the stopped_cpu should probably be a list to mimic the condition variable---for example a GSList). > This all > comes back to cpu_thread_is_idle anyway in making our decision about if > we do or do not sleep on the halt_cond. > >> That also makes it possible to just use >> EXCP_HALTED instead of adding a new EXCP_BLOCKED. > > We can certainly use EXCP_HALTED but maybe come up with a common way of > entering the state? There seems to be a combination of messing around > with special interrupts and direct poking of cs->halted = 1 while > setting the exception. Maybe this could finally clear up the #if > defined(TARGET_I386) hacking in cpus.c? If you're talking accel/tcg/cpu-exec.c, that's different; the issue there is that x86 has a kind of warm reset pin that is not equivalent to cpu_reset. Removing that would only entail adding a new member function to CPUClass. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 17/12/19 15:18, Alex Bennée wrote: >> >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 17/12/19 14:42, Alex Bennée wrote: >>>>> Why do you need to set exception_index to something other than -1 (using >>>>> cpu_loop_exit_noexc for example)? >>>> If there is no exception to process we won't exit the main loop which we >>>> need to do if we want to wait until there is data to read. >>> >>> Okay. >>> >>>>> Using ->stop here is a bit weird, since ->stop is usually related to >>>>> pause_all_vcpus. >>>> >>>> Arguably we could come up with a better API to cpu.c but this allows us >>>> to use cpu_resume(c->sleeping_cpu) when waking up rather than hand >>>> rolling our own wake-up mechanism. >>> >>> But we already have the right wake-up mechanism, which is >>> cpu->halted/cpu_has_work. >> >> cpu_has_work is a guest function though and semihosting_console is a >> common hw module. It can't peek into the guests internal state. > > semihosting_console only needs to something like > cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). As an exception is being delivered we just end up re-executing the EXCP_SEMIHOST. I still don't see why using cpu_interrupt is an improvement seeing as it is secondary to exception processing. > (By the way, > the stopped_cpu should probably be a list to mimic the condition > variable---for example a GSList). ok > >> This all >> comes back to cpu_thread_is_idle anyway in making our decision about if >> we do or do not sleep on the halt_cond. >> >>> That also makes it possible to just use >>> EXCP_HALTED instead of adding a new EXCP_BLOCKED. >> >> We can certainly use EXCP_HALTED but maybe come up with a common way of >> entering the state? There seems to be a combination of messing around >> with special interrupts and direct poking of cs->halted = 1 while >> setting the exception. Maybe this could finally clear up the #if >> defined(TARGET_I386) hacking in cpus.c? > > If you're talking accel/tcg/cpu-exec.c, that's different; the issue > there is that x86 has a kind of warm reset pin that is not equivalent to > cpu_reset. Removing that would only entail adding a new member function > to CPUClass. > > Paolo -- Alex Bennée
Il mer 18 dic 2019, 18:36 Alex Bennée <alex.bennee@linaro.org> ha scritto: > > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 17/12/19 15:18, Alex Bennée wrote: > >> cpu_has_work is a guest function though and semihosting_console is a > >> common hw module. It can't peek into the guests internal state. > > > > semihosting_console only needs to something like > > cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). > > As an exception is being delivered we just end up re-executing the > EXCP_SEMIHOST. I still don't see why using cpu_interrupt is an > improvement seeing as it is secondary to exception processing. > FWIW I skimmed your patch and yes an interrupt is not needed since you are delaying the update of the program counter; that's nicer. Paolo
© 2016 - 2024 Red Hat, Inc.