[PATCH v1 0/4] semihosting read console support

[PATCH v1 0/4] semihosting read console support

[Alex Bennée]

Hi,

This series introduces a new "blocking" console read function for
semihosting and plumbs it in to the ARM semihosting code. The main bit
of work is Keith's patch (with a few tweaks by me). The other
preparatory patches make sure the PC is updated after semihosting
succeeds and a little bit of clean-up.

The following patches need review
   01 - target arm remove unused EXCP_SEMIHOST leg
   02 - target arm only update pc after semihosting compl
   04 - tests tcg add a dumb as bricks semihosting consol

Alex Bennée (3):
  target/arm: remove unused EXCP_SEMIHOST leg
  target/arm: only update pc after semihosting completes
  tests/tcg: add a dumb-as-bricks semihosting console test

Keith Packard (1):
  semihosting: add qemu_semihosting_console_inc for SYS_READC

 include/hw/semihosting/console.h          | 16 +++++
 include/hw/semihosting/semihost.h         |  4 ++
 hw/semihosting/console.c                  | 78 +++++++++++++++++++++++
 linux-user/aarch64/cpu_loop.c             |  1 +
 linux-user/arm/cpu_loop.c                 |  1 +
 linux-user/arm/semihost.c                 | 23 +++++++
 stubs/semihost.c                          |  4 ++
 target/arm/arm-semi.c                     |  3 +-
 target/arm/helper.c                       |  8 +--
 target/arm/m_helper.c                     |  1 +
 target/arm/translate-a64.c                |  2 +-
 target/arm/translate.c                    |  6 +-
 tests/tcg/aarch64/system/semiconsole.c    | 36 +++++++++++
 vl.c                                      |  3 +
 tests/tcg/aarch64/Makefile.softmmu-target |  9 ++-
 15 files changed, 182 insertions(+), 13 deletions(-)
 create mode 100644 tests/tcg/aarch64/system/semiconsole.c

-- 
2.20.1

[PATCH v1 1/4] target/arm: remove unused EXCP_SEMIHOST leg

[Alex Bennée]

All semihosting exceptions are dealt with earlier in the common code
so we should never get here.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5074b5f69ca..b4dc2274c8b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8554,12 +8554,6 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     case EXCP_VFIQ:
         addr += 0x100;
         break;
-    case EXCP_SEMIHOST:
-        qemu_log_mask(CPU_LOG_INT,
-                      "...handling as semihosting call 0x%" PRIx64 "\n",
-                      env->xregs[0]);
-        env->xregs[0] = do_arm_semihosting(env);
-        return;
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
-- 
2.20.1

[PATCH v1 2/4] target/arm: only update pc after semihosting completes

[Alex Bennée]

Before we introduce blocking semihosting calls we need to ensure we
can restart the system on semi hosting exception. To be able to do
this the EXCP_SEMIHOST operation should be idempotent until it finally
completes. Practically this means ensureing we only update the pc
after the semihosting call has completed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/aarch64/cpu_loop.c | 1 +
 linux-user/arm/cpu_loop.c     | 1 +
 target/arm/helper.c           | 2 ++
 target/arm/m_helper.c         | 1 +
 target/arm/translate-a64.c    | 2 +-
 target/arm/translate.c        | 6 +++---
 6 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 31c845a70d4..bbe9fefca81 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -130,6 +130,7 @@ void cpu_loop(CPUARMState *env)
             break;
         case EXCP_SEMIHOST:
             env->xregs[0] = do_arm_semihosting(env);
+            env->pc += 4;
             break;
         case EXCP_YIELD:
             /* nothing to do here for user-mode, just resume guest code */
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 7be40717518..1fae90c6dfc 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -377,6 +377,7 @@ void cpu_loop(CPUARMState *env)
             break;
         case EXCP_SEMIHOST:
             env->regs[0] = do_arm_semihosting(env);
+            env->regs[15] += env->thumb ? 2 : 4;
             break;
         case EXCP_INTERRUPT:
             /* just indicate that signals should be handled asap */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b4dc2274c8b..088e2693df8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8602,11 +8602,13 @@ static void handle_semihosting(CPUState *cs)
                       "...handling as semihosting call 0x%" PRIx64 "\n",
                       env->xregs[0]);
         env->xregs[0] = do_arm_semihosting(env);
+        env->pc += 4;
     } else {
         qemu_log_mask(CPU_LOG_INT,
                       "...handling as semihosting call 0x%x\n",
                       env->regs[0]);
         env->regs[0] = do_arm_semihosting(env);
+        env->regs[15] += env->thumb ? 2 : 4;
     }
 }
 #endif
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 76de317e6af..33d414a684b 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2185,6 +2185,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
                       "...handling as semihosting call 0x%x\n",
                       env->regs[0]);
         env->regs[0] = do_arm_semihosting(env);
+        env->regs[15] += env->thumb ? 2 : 4;
         return;
     case EXCP_BKPT:
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_DEBUG, false);
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index d4bebbe6295..972c28c3c95 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1937,7 +1937,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
                 break;
             }
 #endif
-            gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+            gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
         } else {
             unsupported_encoding(s, insn);
         }
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 2b6c1f91bf9..5185e08641b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1124,7 +1124,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
         s->current_el != 0 &&
 #endif
         (imm == (s->thumb ? 0x3c : 0xf000))) {
-        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+        gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
         return;
     }
 
@@ -8457,7 +8457,7 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
         !IS_USER(s) &&
 #endif
         (a->imm == 0xab)) {
-        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+        gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
     } else {
         gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
     }
@@ -10266,7 +10266,7 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a)
         !IS_USER(s) &&
 #endif
         (a->imm == semihost_imm)) {
-        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+        gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
     } else {
         gen_set_pc_im(s, s->base.pc_next);
         s->svc_imm = a->imm;
-- 
2.20.1

[PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC

[Alex Bennée]

From: Keith Packard <keithp@keithp.com>

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>
Message-Id: <20191104204230.12249-1-keithp@keithp.com>
[AJB: fixed up deadlock, minor commit title reword]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>

---
v7
  - reword commit title
  - remove mutexs, halt CPU until data available
  - document cpu_loop_exit behavior in function API
---
 include/hw/semihosting/console.h  | 16 +++++++
 include/hw/semihosting/semihost.h |  4 ++
 hw/semihosting/console.c          | 78 +++++++++++++++++++++++++++++++
 linux-user/arm/semihost.c         | 23 +++++++++
 stubs/semihost.c                  |  4 ++
 target/arm/arm-semi.c             |  3 +-
 vl.c                              |  3 ++
 7 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/include/hw/semihosting/console.h b/include/hw/semihosting/console.h
index 9be9754bcdf..a3bd6ca2419 100644
--- a/include/hw/semihosting/console.h
+++ b/include/hw/semihosting/console.h
@@ -37,6 +37,22 @@ 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. As this
+ * call may block if no data is available we suspend the CPU and will
+ * rexecute the instruction when data is there. Therefor two
+ * conditions must be met:
+ *   - CPUState is syncronised before callinging this function
+ *   - pc is only updated once the character is succesfully returned
+ *
+ * Returns: character read OR cpu_loop_exit!
+ */
+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 60fc42d851e..b8ce5117ae0 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/hw/semihosting/console.c b/hw/semihosting/console.c
index b4b17c8afbc..6180f33ef21 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -20,8 +20,15 @@
 #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>
+#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 +105,74 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
                       __func__, addr);
     }
 }
+
+#define FIFO_SIZE   1024
+
+typedef struct SemihostingConsole {
+    CharBackend         backend;
+    GSList              *sleeping_cpus;
+    bool                got;
+    Fifo8               fifo;
+} SemihostingConsole;
+
+static SemihostingConsole console;
+
+static int console_can_read(void *opaque)
+{
+    SemihostingConsole *c = opaque;
+    int ret;
+    g_assert(qemu_mutex_iothread_locked());
+    ret = (int) fifo8_num_free(&c->fifo);
+    return ret;
+}
+
+static void console_wake_up(gpointer data, gpointer user_data)
+{
+    CPUState *cs = (CPUState *) data;
+    /* cpu_handle_halt won't know we have work so just unbung here */
+    cs->halted = 0;
+    qemu_cpu_kick(cs);
+}
+
+static void console_read(void *opaque, const uint8_t *buf, int size)
+{
+    SemihostingConsole *c = opaque;
+    g_assert(qemu_mutex_iothread_locked());
+    while (size-- && !fifo8_is_full(&c->fifo)) {
+        fifo8_push(&c->fifo, *buf++);
+    }
+    g_slist_foreach(c->sleeping_cpus, console_wake_up, NULL);
+}
+
+target_ulong qemu_semihosting_console_inc(CPUArchState *env)
+{
+    uint8_t ch;
+    SemihostingConsole *c = &console;
+    g_assert(qemu_mutex_iothread_locked());
+    g_assert(current_cpu);
+    if (fifo8_is_empty(&c->fifo)) {
+        c->sleeping_cpus = g_slist_prepend(c->sleeping_cpus, current_cpu);
+        current_cpu->halted = 1;
+        current_cpu->exception_index = EXCP_HALTED;
+        cpu_loop_exit(current_cpu);
+        /* never returns */
+    }
+    c->sleeping_cpus = g_slist_remove_all(c->sleeping_cpus, current_cpu);
+    ch = fifo8_pop(&c->fifo);
+    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/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
index a16b525eec0..4f998d62201 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 f90589259c0..1d8b37f7b2f 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 6f7b6d801bf..47d61f6fe1f 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 94508300c3c..1912f87822b 100644
--- a/vl.c
+++ b/vl.c
@@ -4142,6 +4142,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.20.1

[PATCH v1 4/4] tests/tcg: add a dumb-as-bricks semihosting console test

[Alex Bennée]

We don't run this during check-tcg as we would need to check stuff is
echoed back. However we can still build the binary so people can test
it manually.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/system/semiconsole.c    | 36 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  9 +++++-
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/system/semiconsole.c

diff --git a/tests/tcg/aarch64/system/semiconsole.c b/tests/tcg/aarch64/system/semiconsole.c
new file mode 100644
index 00000000000..636537fbe4b
--- /dev/null
+++ b/tests/tcg/aarch64/system/semiconsole.c
@@ -0,0 +1,36 @@
+/*
+ * Semihosting Console Test
+ *
+ * Copyright (c) 2019 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <inttypes.h>
+#include <minilib.h>
+
+#define SYS_READC 0x7
+
+uintptr_t __semi_call(uintptr_t type, uintptr_t arg0)
+{
+    register uintptr_t t asm("x0") = type;
+    register uintptr_t a0 asm("x1") = arg0;
+    asm("hlt 0xf000"
+        : "=r" (t)
+        : "r" (t), "r" (a0));
+}
+
+int main(void)
+{
+    char c;
+
+    ml_printf("Semihosting Console Test\n");
+    ml_printf("hit X to exit:");
+
+    do {
+        c = __semi_call(SYS_READC, 0);
+        __sys_outc(c);
+    } while (c != 'X');
+
+    return 0;
+}
diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index 950dbb4bac2..9bdcfd9e7e4 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -31,7 +31,14 @@ LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc
 memory: CFLAGS+=-DCHECK_UNALIGNED=1
 
 # Running
-QEMU_OPTS+=-M virt -cpu max -display none -semihosting-config enable=on,target=native,chardev=output -kernel
+QEMU_BASE_MACHINE=-M virt -cpu max -display none
+QEMU_OPTS+=$(QEMU_BASE_MACHINE) -semihosting-config enable=on,target=native,chardev=output -kernel
+
+# console test is manual only
+QEMU_SEMIHOST=-chardev stdio,mux=on,id=stdio0 -semihosting-config enable=on,chardev=stdio0 -mon chardev=stdio0,mode=readline
+run-semiconsole: QEMU_OPTS=$(QEMU_BASE_MACHINE) $(QEMU_SEMIHOST)  -kernel
+run-semiconsole: semiconsole
+	$(call skip-test, $<, "MANUAL ONLY")
 
 # Simple Record/Replay Test
 .PHONY: memory-record
-- 
2.20.1

Re: [PATCH v1 1/4] target/arm: remove unused EXCP_SEMIHOST leg

[Richard Henderson]

On 12/18/19 8:00 AM, Alex Bennée wrote:
> All semihosting exceptions are dealt with earlier in the common code
> so we should never get here.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/helper.c | 6 ------
>  1 file changed, 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v1 2/4] target/arm: only update pc after semihosting completes

[Richard Henderson]

On 12/18/19 8:00 AM, Alex Bennée wrote:
> Before we introduce blocking semihosting calls we need to ensure we
> can restart the system on semi hosting exception. To be able to do
> this the EXCP_SEMIHOST operation should be idempotent until it finally
> completes. Practically this means ensureing we only update the pc

ensuring.

> after the semihosting call has completed.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/aarch64/cpu_loop.c | 1 +
>  linux-user/arm/cpu_loop.c     | 1 +
>  target/arm/helper.c           | 2 ++
>  target/arm/m_helper.c         | 1 +
>  target/arm/translate-a64.c    | 2 +-
>  target/arm/translate.c        | 6 +++---
>  6 files changed, 9 insertions(+), 4 deletions(-)
...
> +++ b/target/arm/m_helper.c
> @@ -2185,6 +2185,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>                        "...handling as semihosting call 0x%x\n",
>                        env->regs[0]);
>          env->regs[0] = do_arm_semihosting(env);
> +        env->regs[15] += env->thumb ? 2 : 4;

... although thumb should never be false here, it does match the other instances.

I do wonder if it's worth inventing do_arm{32,64}_semihosting wrappers that
consolidate this register manipulation.

But either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC

[Richard Henderson]

On 12/18/19 8:00 AM, Alex Bennée wrote:
> From: Keith Packard <keithp@keithp.com>
> 
> 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

between.

> +/**
> + * 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. As this
> + * call may block if no data is available we suspend the CPU and will
> + * rexecute the instruction when data is there. Therefor two

re-execute, Therefore

> + * conditions must be met:
> + *   - CPUState is syncronised before callinging this function

synchronized, calling

> + *   - pc is only updated once the character is succesfully returned

successfully.


> +static int console_can_read(void *opaque)
> +{
> +    SemihostingConsole *c = opaque;
> +    int ret;
> +    g_assert(qemu_mutex_iothread_locked());
> +    ret = (int) fifo8_num_free(&c->fifo);
> +    return ret;
> +}

Boolean result; better as

  return fifo8_num_free(&c->fifo) > 0

(We could usefully change IOCanReadHandler to return bool to emphasize this.)

> +static void console_wake_up(gpointer data, gpointer user_data)
> +{
> +    CPUState *cs = (CPUState *) data;
> +    /* cpu_handle_halt won't know we have work so just unbung here */
> +    cs->halted = 0;
> +    qemu_cpu_kick(cs);
> +}
> +
> +static void console_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    SemihostingConsole *c = opaque;
> +    g_assert(qemu_mutex_iothread_locked());
> +    while (size-- && !fifo8_is_full(&c->fifo)) {
> +        fifo8_push(&c->fifo, *buf++);
> +    }
> +    g_slist_foreach(c->sleeping_cpus, console_wake_up, NULL);
> +}

I think you should be clearing sleeping_cpus here, after they've all been kicked.

> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> +    uint8_t ch;
> +    SemihostingConsole *c = &console;
> +    g_assert(qemu_mutex_iothread_locked());
> +    g_assert(current_cpu);
> +    if (fifo8_is_empty(&c->fifo)) {
> +        c->sleeping_cpus = g_slist_prepend(c->sleeping_cpus, current_cpu);
> +        current_cpu->halted = 1;
> +        current_cpu->exception_index = EXCP_HALTED;
> +        cpu_loop_exit(current_cpu);
> +        /* never returns */
> +    }
> +    c->sleeping_cpus = g_slist_remove_all(c->sleeping_cpus, current_cpu);

Which would mean you would not have to do this, because current_cpu is only on
the list when it is halted.

I presume all semihosting holds the BQL before we reach here, and we are not
racing on this datastructure?

> +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;
> +    }

Why are you polling stdin?  linux-user isn't system mode, there isn't a
separate monitor thread to get blocked, and you aren't even blocking the thread
to try again just returning -1 to the guest.


r~

Re: [PATCH v1 4/4] tests/tcg: add a dumb-as-bricks semihosting console test

[Richard Henderson]

On 12/18/19 8:00 AM, Alex Bennée wrote:
> We don't run this during check-tcg as we would need to check stuff is
> echoed back. However we can still build the binary so people can test
> it manually.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/aarch64/system/semiconsole.c    | 36 +++++++++++++++++++++++
>  tests/tcg/aarch64/Makefile.softmmu-target |  9 +++++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/aarch64/system/semiconsole.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v1 0/4] semihosting read console support

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> This series introduces a new "blocking" console read function for
> semihosting and plumbs it in to the ARM semihosting code. The main bit
> of work is Keith's patch (with a few tweaks by me). The other
> preparatory patches make sure the PC is updated after semihosting
> succeeds and a little bit of clean-up.
>
> The following patches need review
>    01 - target arm remove unused EXCP_SEMIHOST leg
>    02 - target arm only update pc after semihosting compl
>    04 - tests tcg add a dumb as bricks semihosting consol

I've reviewed all four of these patches to the best of my (limited)
ability. I've also tested this with picolibc's semihosting support for
cortex-m3

Reviewed-by: Keith Packard <keithp@keithp.com>
Tested-by: Keith Packard <keithp@keithp.com>

Thanks much for the rework, definitely beyond my understanding of QEMU
internals.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 12/18/19 8:00 AM, Alex Bennée wrote:
>> From: Keith Packard <keithp@keithp.com>
>> 
>> 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
>
> between.
>
>> +/**
>> + * 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. As this
>> + * call may block if no data is available we suspend the CPU and will
>> + * rexecute the instruction when data is there. Therefor two
>
> re-execute, Therefore
>
>> + * conditions must be met:
>> + *   - CPUState is syncronised before callinging this function
>
> synchronized, calling
>
>> + *   - pc is only updated once the character is succesfully returned
>
> successfully.
>
>
>> +static int console_can_read(void *opaque)
>> +{
>> +    SemihostingConsole *c = opaque;
>> +    int ret;
>> +    g_assert(qemu_mutex_iothread_locked());
>> +    ret = (int) fifo8_num_free(&c->fifo);
>> +    return ret;
>> +}
>
> Boolean result; better as
>
>   return fifo8_num_free(&c->fifo) > 0
>
> (We could usefully change IOCanReadHandler to return bool to emphasize
> this.)

It's documented as the amount you can read and other handlers return
amounts as well. I'm not sure I want to go messing with the chardev code
in this series (although I need to look at Phillipe's series).

>
>> +static void console_wake_up(gpointer data, gpointer user_data)
>> +{
>> +    CPUState *cs = (CPUState *) data;
>> +    /* cpu_handle_halt won't know we have work so just unbung here */
>> +    cs->halted = 0;
>> +    qemu_cpu_kick(cs);
>> +}
>> +
>> +static void console_read(void *opaque, const uint8_t *buf, int size)
>> +{
>> +    SemihostingConsole *c = opaque;
>> +    g_assert(qemu_mutex_iothread_locked());
>> +    while (size-- && !fifo8_is_full(&c->fifo)) {
>> +        fifo8_push(&c->fifo, *buf++);
>> +    }
>> +    g_slist_foreach(c->sleeping_cpus, console_wake_up, NULL);
>> +}
>
> I think you should be clearing sleeping_cpus here, after they've all been kicked.
>
>> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
>> +{
>> +    uint8_t ch;
>> +    SemihostingConsole *c = &console;
>> +    g_assert(qemu_mutex_iothread_locked());
>> +    g_assert(current_cpu);
>> +    if (fifo8_is_empty(&c->fifo)) {
>> +        c->sleeping_cpus = g_slist_prepend(c->sleeping_cpus, current_cpu);
>> +        current_cpu->halted = 1;
>> +        current_cpu->exception_index = EXCP_HALTED;
>> +        cpu_loop_exit(current_cpu);
>> +        /* never returns */
>> +    }
>> +    c->sleeping_cpus = g_slist_remove_all(c->sleeping_cpus, current_cpu);
>
> Which would mean you would not have to do this, because current_cpu is only on
> the list when it is halted.
>
> I presume all semihosting holds the BQL before we reach here, and we are not
> racing on this datastructure?

Yeah this is all under BQL - which I assert is the case. I'll add a
comment to the structure.

>
>> +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;
>> +    }
>
> Why are you polling stdin?  linux-user isn't system mode, there isn't a
> separate monitor thread to get blocked, and you aren't even blocking the thread
> to try again just returning -1 to the guest.

Hmm not sure - I guess we should just bite the bullet and potentially
block here. semihosting is linux-user is a bit of a weird use case
because we are not providing "hardware" but it seems it is used by a
bunch of testcases that want to test things like M-profile non-glibc
binaries without the baggage of a full simulation.

>
>
> r~


-- 
Alex Bennée