[PATCH] Semihost SYS_READC implementation (v3)

[PATCH] Semihost SYS_READC implementation (v3)

[Keith Packard]

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.

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

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/semihosting/console.c          | 73 +++++++++++++++++++++++++++++++
 include/hw/semihosting/console.h  | 12 +++++
 include/hw/semihosting/semihost.h |  4 ++
 linux-user/arm/semihost.c         | 24 ++++++++++
 target/arm/arm-semi.c             |  3 +-
 vl.c                              |  3 ++
 6 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index b4b17c8afb..197bff079b 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -98,3 +98,76 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
                       __func__, addr);
     }
 }
+
+#include <pthread.h>
+#include "chardev/char-fe.h"
+#include "sysemu/sysemu.h"
+#include "qemu/main-loop.h"
+#include "qapi/error.h"
+#include "qemu/fifo8.h"
+
+#define FIFO_SIZE   1024
+
+typedef struct SemihostingConsole {
+    CharBackend         backend;
+    pthread_mutex_t     mutex;
+    pthread_cond_t      cond;
+    bool                got;
+    Fifo8               fifo;
+} SemihostingConsole;
+
+static SemihostingConsole console = {
+    .mutex = PTHREAD_MUTEX_INITIALIZER,
+    .cond = PTHREAD_COND_INITIALIZER
+};
+
+static int console_can_read(void *opaque)
+{
+    SemihostingConsole *c = opaque;
+    int ret;
+    pthread_mutex_lock(&c->mutex);
+    ret = (int) fifo8_num_free(&c->fifo);
+    pthread_mutex_unlock(&c->mutex);
+    return ret;
+}
+
+static void console_read(void *opaque, const uint8_t *buf, int size)
+{
+    SemihostingConsole *c = opaque;
+    pthread_mutex_lock(&c->mutex);
+    while (size-- && !fifo8_is_full(&c->fifo)) {
+        fifo8_push(&c->fifo, *buf++);
+    }
+    pthread_cond_broadcast(&c->cond);
+    pthread_mutex_unlock(&c->mutex);
+}
+
+target_ulong qemu_semihosting_console_inc(CPUArchState *env)
+{
+    uint8_t ch;
+    SemihostingConsole *c = &console;
+    qemu_mutex_unlock_iothread();
+    pthread_mutex_lock(&c->mutex);
+    while (fifo8_is_empty(&c->fifo)) {
+        pthread_cond_wait(&c->cond, &c->mutex);
+    }
+    ch = fifo8_pop(&c->fifo);
+    pthread_mutex_unlock(&c->mutex);
+    qemu_mutex_lock_iothread();
+    return (target_ulong) ch;
+}
+
+void qemu_semihosting_console_init(void)
+{
+    Chardev *chr = semihosting_get_chardev();
+
+    if  (chr) {
+        fifo8_create(&console.fifo, FIFO_SIZE);
+        qemu_chr_fe_init(&console.backend, chr, &error_abort);
+        qemu_chr_fe_set_handlers(&console.backend,
+                                 console_can_read,
+                                 console_read,
+                                 NULL, NULL, &console,
+                                 NULL, true);
+    }
+}
diff --git a/include/hw/semihosting/console.h b/include/hw/semihosting/console.h
index 9be9754bcd..f7d5905b41 100644
--- a/include/hw/semihosting/console.h
+++ b/include/hw/semihosting/console.h
@@ -37,6 +37,18 @@ int qemu_semihosting_console_outs(CPUArchState *env, target_ulong s);
  */
 void qemu_semihosting_console_outc(CPUArchState *env, target_ulong c);
 
+/**
+ * qemu_semihosting_console_inc:
+ * @env: CPUArchState
+ *
+ * Receive single character from debug console. This
+ * may be the remote gdb session if a softmmu guest is currently being
+ * debugged.
+ *
+ * Returns: character read or -1 on error
+ */
+target_ulong qemu_semihosting_console_inc(CPUArchState *env);
+
 /**
  * qemu_semihosting_log_out:
  * @s: pointer to string
diff --git a/include/hw/semihosting/semihost.h b/include/hw/semihosting/semihost.h
index 60fc42d851..b8ce5117ae 100644
--- a/include/hw/semihosting/semihost.h
+++ b/include/hw/semihosting/semihost.h
@@ -56,6 +56,9 @@ static inline Chardev *semihosting_get_chardev(void)
 {
     return NULL;
 }
+static inline void qemu_semihosting_console_init(void)
+{
+}
 #else /* !CONFIG_USER_ONLY */
 bool semihosting_enabled(void);
 SemihostingTarget semihosting_get_target(void);
@@ -68,6 +71,7 @@ Chardev *semihosting_get_chardev(void);
 void qemu_semihosting_enable(void);
 int qemu_semihosting_config_options(const char *opt);
 void qemu_semihosting_connect_chardevs(void);
+void qemu_semihosting_console_init(void);
 #endif /* CONFIG_USER_ONLY */
 
 #endif /* SEMIHOST_H */
diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
index a16b525eec..13a097515b 100644
--- a/linux-user/arm/semihost.c
+++ b/linux-user/arm/semihost.c
@@ -47,3 +47,27 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
         }
     }
 }
+
+#include <poll.h>
+
+target_ulong qemu_semihosting_console_inc(CPUArchState *env)
+{
+    uint8_t c;
+    struct pollfd pollfd = {
+        .fd = STDIN_FILENO,
+        .events = POLLIN
+    };
+
+    if (poll(&pollfd, 1, -1) != 1) {
+        qemu_log_mask(LOG_UNIMP, "%s: unexpected read from stdin failure",
+                      __func__);
+        return (target_ulong) -1;
+    }
+
+    if (read(STDIN_FILENO, &c, 1) != 1) {
+        qemu_log_mask(LOG_UNIMP, "%s: unexpected read from stdin failure",
+                      __func__);
+        return (target_ulong) -1;
+    }
+    return (target_ulong) c;
+}
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 6f7b6d801b..47d61f6fe1 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -802,8 +802,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 
         return guestfd_fns[gf->type].readfn(cpu, gf, arg1, len);
     case TARGET_SYS_READC:
-        qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
-        return 0;
+        return qemu_semihosting_console_inc(env);
     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.24.0.rc0

Re: [PATCH] Semihost SYS_READC implementation (v3)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191023192640.13125-1-keithp@keithp.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/target/arm/translate-sve.o
../vl.o: In function `main':
/tmp/qemu-test/src/vl.c:4385: undefined reference to `qemu_semihosting_console_init'
collect2: error: ld returned 1 exit status
make[1]: *** [qemu-system-x86_64] Error 1
make: *** [x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
  LINK    aarch64-softmmu/qemu-system-aarch64
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3c82d370996f429bb4be7fef56e7247b', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-nju6kwxi/src/docker-src.2019-10-24-13.29.33.24205:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=3c82d370996f429bb4be7fef56e7247b
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-nju6kwxi/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    3m52.464s
user    0m8.464s


The full log is available at
http://patchew.org/logs/20191023192640.13125-1-keithp@keithp.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] Semihost SYS_READC implementation (v3)

[Paolo Bonzini]

On 24/10/19 19:33, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20191023192640.13125-1-keithp@keithp.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   CC      aarch64-softmmu/target/arm/translate-sve.o
> ../vl.o: In function `main':
> /tmp/qemu-test/src/vl.c:4385: undefined reference to `qemu_semihosting_console_init'
> collect2: error: ld returned 1 exit status
> make[1]: *** [qemu-system-x86_64] Error 1
> make: *** [x86_64-softmmu/all] Error 2
> make: *** Waiting for unfinished jobs....
>   LINK    aarch64-softmmu/qemu-system-aarch64
> Traceback (most recent call last):
> ---
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3c82d370996f429bb4be7fef56e7247b', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-nju6kwxi/src/docker-src.2019-10-24-13.29.33.24205:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
> filter=--filter=label=com.qemu.instance.uuid=3c82d370996f429bb4be7fef56e7247b
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-nju6kwxi/src'
> make: *** [docker-run-test-quick@centos7] Error 2

Looks like you need to add a dummy implementation of the new function to
stubs/semihost.c, for use in targets that don't support semihosting.

Thanks,

Paolo

[PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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.

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

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/semihosting/console.c          | 73 +++++++++++++++++++++++++++++++
 include/hw/semihosting/console.h  | 12 +++++
 include/hw/semihosting/semihost.h |  4 ++
 linux-user/arm/semihost.c         | 24 ++++++++++
 target/arm/arm-semi.c             |  3 +-
 vl.c                              |  3 ++
 6 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index b4b17c8afb..197bff079b 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -98,3 +98,76 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
                       __func__, addr);
     }
 }
+
+#include <pthread.h>
+#include "chardev/char-fe.h"
+#include "sysemu/sysemu.h"
+#include "qemu/main-loop.h"
+#include "qapi/error.h"
+#include "qemu/fifo8.h"
+
+#define FIFO_SIZE   1024
+
+typedef struct SemihostingConsole {
+    CharBackend         backend;
+    pthread_mutex_t     mutex;
+    pthread_cond_t      cond;
+    bool                got;
+    Fifo8               fifo;
+} SemihostingConsole;
+
+static SemihostingConsole console = {
+    .mutex = PTHREAD_MUTEX_INITIALIZER,
+    .cond = PTHREAD_COND_INITIALIZER
+};
+
+static int console_can_read(void *opaque)
+{
+    SemihostingConsole *c = opaque;
+    int ret;
+    pthread_mutex_lock(&c->mutex);
+    ret = (int) fifo8_num_free(&c->fifo);
+    pthread_mutex_unlock(&c->mutex);
+    return ret;
+}
+
+static void console_read(void *opaque, const uint8_t *buf, int size)
+{
+    SemihostingConsole *c = opaque;
+    pthread_mutex_lock(&c->mutex);
+    while (size-- && !fifo8_is_full(&c->fifo)) {
+        fifo8_push(&c->fifo, *buf++);
+    }
+    pthread_cond_broadcast(&c->cond);
+    pthread_mutex_unlock(&c->mutex);
+}
+
+target_ulong qemu_semihosting_console_inc(CPUArchState *env)
+{
+    uint8_t ch;
+    SemihostingConsole *c = &console;
+    qemu_mutex_unlock_iothread();
+    pthread_mutex_lock(&c->mutex);
+    while (fifo8_is_empty(&c->fifo)) {
+        pthread_cond_wait(&c->cond, &c->mutex);
+    }
+    ch = fifo8_pop(&c->fifo);
+    pthread_mutex_unlock(&c->mutex);
+    qemu_mutex_lock_iothread();
+    return (target_ulong) ch;
+}
+
+void qemu_semihosting_console_init(void)
+{
+    Chardev *chr = semihosting_get_chardev();
+
+    if  (chr) {
+        fifo8_create(&console.fifo, FIFO_SIZE);
+        qemu_chr_fe_init(&console.backend, chr, &error_abort);
+        qemu_chr_fe_set_handlers(&console.backend,
+                                 console_can_read,
+                                 console_read,
+                                 NULL, NULL, &console,
+                                 NULL, true);
+    }
+}
diff --git a/include/hw/semihosting/console.h b/include/hw/semihosting/console.h
index 9be9754bcd..f7d5905b41 100644
--- a/include/hw/semihosting/console.h
+++ b/include/hw/semihosting/console.h
@@ -37,6 +37,18 @@ int qemu_semihosting_console_outs(CPUArchState *env, target_ulong s);
  */
 void qemu_semihosting_console_outc(CPUArchState *env, target_ulong c);
 
+/**
+ * qemu_semihosting_console_inc:
+ * @env: CPUArchState
+ *
+ * Receive single character from debug console. This
+ * may be the remote gdb session if a softmmu guest is currently being
+ * debugged.
+ *
+ * Returns: character read or -1 on error
+ */
+target_ulong qemu_semihosting_console_inc(CPUArchState *env);
+
 /**
  * qemu_semihosting_log_out:
  * @s: pointer to string
diff --git a/include/hw/semihosting/semihost.h b/include/hw/semihosting/semihost.h
index 60fc42d851..b8ce5117ae 100644
--- a/include/hw/semihosting/semihost.h
+++ b/include/hw/semihosting/semihost.h
@@ -56,6 +56,9 @@ static inline Chardev *semihosting_get_chardev(void)
 {
     return NULL;
 }
+static inline void qemu_semihosting_console_init(void)
+{
+}
 #else /* !CONFIG_USER_ONLY */
 bool semihosting_enabled(void);
 SemihostingTarget semihosting_get_target(void);
@@ -68,6 +71,7 @@ Chardev *semihosting_get_chardev(void);
 void qemu_semihosting_enable(void);
 int qemu_semihosting_config_options(const char *opt);
 void qemu_semihosting_connect_chardevs(void);
+void qemu_semihosting_console_init(void);
 #endif /* CONFIG_USER_ONLY */
 
 #endif /* SEMIHOST_H */
diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
index a16b525eec..13a097515b 100644
--- a/linux-user/arm/semihost.c
+++ b/linux-user/arm/semihost.c
@@ -47,3 +47,27 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
         }
     }
 }
+
+#include <poll.h>
+
+target_ulong qemu_semihosting_console_inc(CPUArchState *env)
+{
+    uint8_t c;
+    struct pollfd pollfd = {
+        .fd = STDIN_FILENO,
+        .events = POLLIN
+    };
+
+    if (poll(&pollfd, 1, -1) != 1) {
+        qemu_log_mask(LOG_UNIMP, "%s: unexpected read from stdin failure",
+                      __func__);
+        return (target_ulong) -1;
+    }
+
+    if (read(STDIN_FILENO, &c, 1) != 1) {
+        qemu_log_mask(LOG_UNIMP, "%s: unexpected read from stdin failure",
+                      __func__);
+        return (target_ulong) -1;
+    }
+    return (target_ulong) c;
+}
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 6f7b6d801b..47d61f6fe1 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -802,8 +802,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 
         return guestfd_fns[gf->type].readfn(cpu, gf, arg1, len);
     case TARGET_SYS_READC:
-        qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
-        return 0;
+        return qemu_semihosting_console_inc(env);
     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.24.0.rc0

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Alex Bennée]

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

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

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

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

>
> v2:
> 	Add implementation in linux-user/arm/semihost.c
>
> v3:  (thanks to Paolo Bonzini <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

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

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

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

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

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

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

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

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


--
Alex Bennée

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

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

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

I'm adding semihosting support to picolibc
(https://keithp.com/picolibc/) and need a way to automate tests for it's
SYS_READC support, so eventually I'll have automated tests there, but
that depends on qemu support...

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

Sure, I'll edit the mail before sending. In my repo, I'm leaving the
version history in git so I can keep track of it.

>> v4:
>> 	Add qemu_semihosting_console_init to stubs/semihost.c for
>> 	hosts that don't support semihosting
>
> This doesn't appear to be in the diff which is why I'm seeing a compile
> failure for non-CONFIG_SEMIHOST machines. However...

Argh! Just git operation failure -- I'm building another patch on top of
this (for RISC-V semihosting support) and the stubs/semihost.c change
got caught in there. My overall check for changes missed this mistake.

>> +
>> +#include <poll.h>
>
> Headers should go at the top.

Thanks; I've fixed this file and hw/semihosting/console.c

>>      case TARGET_SYS_READC:
>> -        qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
>> -        return 0;
>> +        return qemu_semihosting_console_inc(env);
>
> I'm not sure this would be correct if there was no character available.
> The docs imply it blocks although don't say so explicitly AFAICT.

Here's what the docs say:

        https://static.docs.arm.com/100863/0200/semihosting.pdf

        Return

        On exit, the RETURN REGISTER contains the byte read from the console.

If this call didn't block, they'd have to define a value which indicated
that no byte was available? Openocd also implements SYS_READC using
'getchar()', which definitely blocks. So, at least qemu would be the
same.

I realize it's really weird to block the whole emulation for this call,
but given the target environment (deeply embedded devices), it's quite
convenient as the whole qemu process blocks, instead of spinning.

>> +    /* connect semihosting console input if requested */
>> +    qemu_semihosting_console_init();
>> +
>
> I'd rather rename qemu_semihosting_connect_chardevs to
> qemu_semihosting_init and keep all our bits of semihosting setup in
> there rather than having multiple calls out of vl.c

I also thought this would be a nice cleanup. However, the last caller to
qemu_chr_fe_set_handlers gets the focus for input, and connect_chardevs
is called before the serial ports and monitor are initialized, so
semihosting gets pushed aside and stdio input ends up hooked to the monitor
instead.

Adding this function and placing the call after the other stdio users
get hooked up means that semihosting starts with the input focus.

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Fri, 25 Oct 2019 at 17:40, Keith Packard <keithp@keithp.com> wrote:
>
> Alex Bennée <alex.bennee@linaro.org> writes:
>
> > I can see the use for this but I'd like to know what you are testing
> > with. We only have very basic smoketests in check-tcg but I've tested
> > with the latest arm-semihosting tests and they are all fine so no
> > regressions there.
>
> I'm adding semihosting support to picolibc
> (https://keithp.com/picolibc/) and need a way to automate tests for it's
> SYS_READC support, so eventually I'll have automated tests there, but
> that depends on qemu support...

I have a cheesy-but-functional set of test code
https://git.linaro.org/people/peter.maydell/semihosting-tests.git/
here, fwiw.

> Argh! Just git operation failure -- I'm building another patch on top of
> this (for RISC-V semihosting support) and the stubs/semihost.c change
> got caught in there. My overall check for changes missed this mistake.

Is there a specification for RISC-V semihosting? This is
likely to be my first question when the support comes
round for review, so you can have it early :-)  We'd
prefer to implement specified interfaces, not random
ad-hoc "this seems to be what newlib wants to see,
which is turn got hacked together by copying some other
architecture's code".

> Here's what the docs say:
>
>         https://static.docs.arm.com/100863/0200/semihosting.pdf
>
>         Return
>
>         On exit, the RETURN REGISTER contains the byte read from the console.
>
> If this call didn't block, they'd have to define a value which indicated
> that no byte was available? Openocd also implements SYS_READC using
> 'getchar()', which definitely blocks. So, at least qemu would be the
> same.
>
> I realize it's really weird to block the whole emulation for this call,
> but given the target environment (deeply embedded devices), it's quite
> convenient as the whole qemu process blocks, instead of spinning.

Yeah, the docs could perhaps spell it out more clearly, but
the intention is that the call blocks.

It's possible if necessary to implement this so it doesn't
block the entire simulation: you just have the QEMU implementation
do a block-until-character-or-timeout operation, and if it times
out then you rewind the guest PC to point at the BRK/HLT insn that
triggered the semihosting call, and resume simulation. That would
give an opportunity for things like interrupts to fire before going
back into waiting for a character. This feels to me like it's a bit
overcomplicated unless it turns out we actually require it though.

> I also thought this would be a nice cleanup. However, the last caller to
> qemu_chr_fe_set_handlers gets the focus for input, and connect_chardevs
> is called before the serial ports and monitor are initialized, so
> semihosting gets pushed aside and stdio input ends up hooked to the monitor
> instead.

Isn't the answer to this "don't use a command line that tries
to connect stdio to multiple things" ?

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Peter Maydell <peter.maydell@linaro.org> writes:

> Is there a specification for RISC-V semihosting? This is
> likely to be my first question when the support comes
> round for review, so you can have it early :-)  We'd
> prefer to implement specified interfaces, not random
> ad-hoc "this seems to be what newlib wants to see,
> which is turn got hacked together by copying some other
> architecture's code".

There seems to be convergence on a pretty simple interface which uses
ebreak surrounded by a couple of specific no-ops:

      slli x0, x0, 0x1f
      ebreak
      srai x0, x0, 0x7

There are implementations in rust and openocd, and I've got one for
picolibc. The risc-v semihosting code is sitting on a branch in my repo
on github:

        https://github.com/keith-packard/qemu/tree/riscv-semihost

> (describing a mechanism to avoid stopping the emulator)
> This feels to me like it's a bit overcomplicated unless it turns out
> we actually require it though.

Would also be nice for multi-core setups. I'd like to start with the
simple plan for now.

> Isn't the answer to this "don't use a command line that tries
> to connect stdio to multiple things" ?

Uh, we do that all the time? The mux device is designed to handle this
so that you can use stdio for both monitor commands and application
I/O. It's very convenient, the only issue is that the last device that
hooks to the mux ends up getting input first (you use ^Ac to rotate
among the selected devices).

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Fri, 25 Oct 2019 at 20:15, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > Is there a specification for RISC-V semihosting? This is
> > likely to be my first question when the support comes
> > round for review, so you can have it early :-)  We'd
> > prefer to implement specified interfaces, not random
> > ad-hoc "this seems to be what newlib wants to see,
> > which is turn got hacked together by copying some other
> > architecture's code".
>
> There seems to be convergence on a pretty simple interface which uses
> ebreak surrounded by a couple of specific no-ops:
>
>       slli x0, x0, 0x1f
>       ebreak
>       srai x0, x0, 0x7
>
> There are implementations in rust and openocd, and I've got one for
> picolibc.

I'm going to push for somebody actually writing out a
document and putting it somewhere that we can point to
and say "that's the authoritative spec", please...
it doesn't have to be a big formal thing, but I do
think you want it written down, because the whole point
is for multiple implementations and users to interoperate.


> > Isn't the answer to this "don't use a command line that tries
> > to connect stdio to multiple things" ?
>
> Uh, we do that all the time? The mux device is designed to handle this
> so that you can use stdio for both monitor commands and application
> I/O. It's very convenient, the only issue is that the last device that
> hooks to the mux ends up getting input first (you use ^Ac to rotate
> among the selected devices).

Yeah, the mux works fine for this kind of thing. There's
no inherent reason why semihosting ought to "win" as
the initially selected thing on the mux, though --
typically that would be expected to be the UART/serial
console.

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Peter Maydell <peter.maydell@linaro.org> writes:

> I'm going to push for somebody actually writing out a
> document and putting it somewhere that we can point to
> and say "that's the authoritative spec", please...
> it doesn't have to be a big formal thing, but I do
> think you want it written down, because the whole point
> is for multiple implementations and users to interoperate.

I can work within the RISC-V foundation to get an 'official' document
written. Having a handful of existing inter-operable implementations
will make that really easy to do :-)

> Yeah, the mux works fine for this kind of thing. There's
> no inherent reason why semihosting ought to "win" as
> the initially selected thing on the mux, though --
> typically that would be expected to be the UART/serial
> console.

That would just require moving the call to qemu_semihosting_console_init
up in the function. Doesn't really matter to me; I suspect that most
users will either user serial or semihosting, but probably not both
(except when debugging the serial driver).

This does the trick (on top of the latest patch). Let me know if this is
what you want. To get semihosting to be first, you have to disable the
serial driver if the hardware has a serial port:

        qemu -chardev stdio,mux=on,id=stdio0 \
             -serial null \
             -semihosting-config enable=on,chardev=stdio0 \
             -mon chardev=stdio0,mode=readline"

diff --git a/vl.c b/vl.c
index ac584d97ea..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)
@@ -4381,9 +4384,6 @@ 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();
 
-- 
-keith

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

[PATCH] Semihost SYS_READC implementation (v6)

[Keith Packard]

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

Re: [PATCH] Semihost SYS_READC implementation (v6)

[Alex Bennée]

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

Re: [PATCH] Semihost SYS_READC implementation (v6)

[Paolo Bonzini]

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

Re: [PATCH] Semihost SYS_READC implementation (v6)

[Alex Bennée]

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

Re: [PATCH] Semihost SYS_READC implementation (v6)

[Paolo Bonzini]

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

[RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Alex Bennée]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Paolo Bonzini]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Alex Bennée]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Paolo Bonzini]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Alex Bennée]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Paolo Bonzini]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Paolo Bonzini]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Alex Bennée]

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

Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

[Paolo Bonzini]

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

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

[-- Attachment #2: Type: text/html, Size: 1303 bytes --]

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Peter Maydell <peter.maydell@linaro.org> writes:

> I'm going to push for somebody actually writing out a
> document and putting it somewhere that we can point to
> and say "that's the authoritative spec", please...
> it doesn't have to be a big formal thing, but I do
> think you want it written down, because the whole point
> is for multiple implementations and users to interoperate.

That happened in June -- I was just looking at the wrong version of the
spec. In the current version, which can be found here:

        https://riscv.org/specifications/

                   The RISC-V Instruction Set Manual
                       Volume I: Unprivileged ISA
                Document Version 20190608-Base-Ratified
                                    
Section 2.8 says:

        Another use of EBREAK is to support “semihosting”, where the
        execution environment includes a debugger that can provide
        services over an alternate system call interface built around
        the EBREAK instruction.  Because the RISC-V base ISA does not
        provide more than one EBREAK instruction, RISC-V semihosting
        uses a special sequence of instructions to distinguish a
        semihosting EBREAK from a debugger inserted EBREAK.

                slli x0, x0, 0x1f   # Entry NOP
                ebreak              # Break to debugger
                srai x0, x0, 7      # NOP encoding the semihosting call number 7

        Note that these three instructions must be 32-bit-wide
        instructions, i.e., they mustn’t be among the compressed 16-bit
        instructions described in Chapter 16.

        The shift NOP instructions are still considered available for
        use as HINTS.

        Semihosting is a form of service call and would be more
        naturally encoded as an ECALL using an existing ABI, but this
        would require the debugger to be able to intercept ECALLs, which
        is a newer addition to the debug standard.  We intend to move
        over to using ECALLs with a standard ABI, in which case,
        semihosting can share a service ABI with an existing standard.

        We note that ARM processors have also moved to using SVC instead
        of BKPT for semihosting calls in newer designs.

I'd like to get the READC patch landed and then post the RISC-V patch
afterwards as the RISC-V patch currently includes READC support.

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Tue, 5 Nov 2019 at 05:10, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > I'm going to push for somebody actually writing out a
> > document and putting it somewhere that we can point to
> > and say "that's the authoritative spec", please...
> > it doesn't have to be a big formal thing, but I do
> > think you want it written down, because the whole point
> > is for multiple implementations and users to interoperate.
>
> That happened in June -- I was just looking at the wrong version of the
> spec. In the current version, which can be found here:
>
>         https://riscv.org/specifications/
>
>                    The RISC-V Instruction Set Manual
>                        Volume I: Unprivileged ISA
>                 Document Version 20190608-Base-Ratified
>
> Section 2.8 says:
>
>         Another use of EBREAK is to support “semihosting”, where the
>         execution environment includes a debugger that can provide
>         services over an alternate system call interface built around
>         the EBREAK instruction.  Because the RISC-V base ISA does not
>         provide more than one EBREAK instruction, RISC-V semihosting
>         uses a special sequence of instructions to distinguish a
>         semihosting EBREAK from a debugger inserted EBREAK.
>
>                 slli x0, x0, 0x1f   # Entry NOP
>                 ebreak              # Break to debugger
>                 srai x0, x0, 7      # NOP encoding the semihosting call number 7
>
>         Note that these three instructions must be 32-bit-wide
>         instructions, i.e., they mustn’t be among the compressed 16-bit
>         instructions described in Chapter 16.
>
>         The shift NOP instructions are still considered available for
>         use as HINTS.
>
>         Semihosting is a form of service call and would be more
>         naturally encoded as an ECALL using an existing ABI, but this
>         would require the debugger to be able to intercept ECALLs, which
>         is a newer addition to the debug standard.  We intend to move
>         over to using ECALLs with a standard ABI, in which case,
>         semihosting can share a service ABI with an existing standard.
>
>         We note that ARM processors have also moved to using SVC instead
>         of BKPT for semihosting calls in newer designs.

That defines the instruction sequence used to make a semihosting
call, but not the specification of what the calls are:
 * what call numbers perform which functions
 * how arguments are passed to the call (registers? parameter
   blocks in memory? other?)
 * the semantics of each function supported (number of arguments,
   behaviour, error handling)

That's really what I had in mind by the overall semihosting spec.

PS: the parenthetical about ARM semihosting at the bottom  of
the text you quote is wrong, incidentally. The traditional insn
for semihosting on A-profile devices has always been SWI/SVC; it
is BKPT only on M-profile devices; and the latest revision of the
semihosting spec recommends the HLT instruction for both A- and M-.

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Alistair Francis]

On Mon, Nov 11, 2019 at 6:51 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 5 Nov 2019 at 05:10, Keith Packard <keithp@keithp.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > I'm going to push for somebody actually writing out a
> > > document and putting it somewhere that we can point to
> > > and say "that's the authoritative spec", please...
> > > it doesn't have to be a big formal thing, but I do
> > > think you want it written down, because the whole point
> > > is for multiple implementations and users to interoperate.
> >
> > That happened in June -- I was just looking at the wrong version of the
> > spec. In the current version, which can be found here:
> >
> >         https://riscv.org/specifications/
> >
> >                    The RISC-V Instruction Set Manual
> >                        Volume I: Unprivileged ISA
> >                 Document Version 20190608-Base-Ratified
> >
> > Section 2.8 says:
> >
> >         Another use of EBREAK is to support “semihosting”, where the
> >         execution environment includes a debugger that can provide
> >         services over an alternate system call interface built around
> >         the EBREAK instruction.  Because the RISC-V base ISA does not
> >         provide more than one EBREAK instruction, RISC-V semihosting
> >         uses a special sequence of instructions to distinguish a
> >         semihosting EBREAK from a debugger inserted EBREAK.
> >
> >                 slli x0, x0, 0x1f   # Entry NOP
> >                 ebreak              # Break to debugger
> >                 srai x0, x0, 7      # NOP encoding the semihosting call number 7
> >
> >         Note that these three instructions must be 32-bit-wide
> >         instructions, i.e., they mustn’t be among the compressed 16-bit
> >         instructions described in Chapter 16.
> >
> >         The shift NOP instructions are still considered available for
> >         use as HINTS.
> >
> >         Semihosting is a form of service call and would be more
> >         naturally encoded as an ECALL using an existing ABI, but this
> >         would require the debugger to be able to intercept ECALLs, which
> >         is a newer addition to the debug standard.  We intend to move
> >         over to using ECALLs with a standard ABI, in which case,
> >         semihosting can share a service ABI with an existing standard.
> >
> >         We note that ARM processors have also moved to using SVC instead
> >         of BKPT for semihosting calls in newer designs.
>
> That defines the instruction sequence used to make a semihosting
> call, but not the specification of what the calls are:
>  * what call numbers perform which functions
>  * how arguments are passed to the call (registers? parameter
>    blocks in memory? other?)
>  * the semantics of each function supported (number of arguments,
>    behaviour, error handling)
>
> That's really what I had in mind by the overall semihosting spec.

This sounds like something that the platform spec should contain.

+Atish +Palmer who are in charge of that.

@Keith maybe you should contact the platform spec group and ask them
to look into this?

Alistair

>
> PS: the parenthetical about ARM semihosting at the bottom  of
> the text you quote is wrong, incidentally. The traditional insn
> for semihosting on A-profile devices has always been SWI/SVC; it
> is BKPT only on M-profile devices; and the latest revision of the
> semihosting spec recommends the HLT instruction for both A- and M-.
>
> thanks
> -- PMM
>

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Alistair Francis <alistair23@gmail.com> writes:

> This sounds like something that the platform spec should contain.

I'm frankly happy with it specifying the semantics by reference to the
ARM docs -- that way we can easily share existing code without concern
about subtle semantic differences.

The only thing that would be useful is to clarify the low-level ABI
details about argument passing, but given that the ARM semihosting spec
passes arguments in the standard registers, extrapolating that to RISC-V
is not exactly difficult.

> +Atish +Palmer who are in charge of that.
>
> @Keith maybe you should contact the platform spec group and ask them
> to look into this?

I can certainly ask to have the argument passing details clarified; I
doubt that group would be interested in adopting the whole semihosting
spec and publishing it separately.

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Peter Maydell <peter.maydell@linaro.org> writes:

> That defines the instruction sequence used to make a semihosting
> call, but not the specification of what the calls are:
>  * what call numbers perform which functions
>  * how arguments are passed to the call (registers? parameter
>    blocks in memory? other?)
>  * the semantics of each function supported (number of arguments,
>    behaviour, error handling)
>
> That's really what I had in mind by the overall semihosting spec.

There isn't anything more provided by the RISC-V foundation at this
point. I'm not sure what else they *should* provide as the goal is to
match the ARM design, which does specify all of the above.

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Thu, 14 Nov 2019 at 17:39, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > That defines the instruction sequence used to make a semihosting
> > call, but not the specification of what the calls are:
> >  * what call numbers perform which functions
> >  * how arguments are passed to the call (registers? parameter
> >    blocks in memory? other?)
> >  * the semantics of each function supported (number of arguments,
> >    behaviour, error handling)
> >
> > That's really what I had in mind by the overall semihosting spec.
>
> There isn't anything more provided by the RISC-V foundation at this
> point. I'm not sure what else they *should* provide as the goal is to
> match the ARM design, which does specify all of the above.

This isn't obvious if you just say "semihosting".
QEMU currently provides 'semihosting' ABIs for:
 * arm
 * lm32
 * m68k
 * mips
 * nios2
 * xtensa

m68k and nios2 provide basically the same set of calls,
but all the other architectures differe from each other.
"Semihosting" just means "the concept of an ABI from guest
to the debugger or emulator", not a particular ABI.

The ARM semihosting ABI also has a number of warts
which are basically historical legacy. With a clean
sheet you get to avoid some of them. (Notably you could
skip the whole 'negotiate presence of extensions' business
by just getting those parts right from the start. Actually
specifying errno values would also be sensible and is
something the ARM spec sadly does not do and can't really
at this point with multiple implementations in play.)

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Thu, 14 Nov 2019 at 17:47, Peter Maydell <peter.maydell@linaro.org> wrote:

> The ARM semihosting ABI also has a number of warts
> which are basically historical legacy. With a clean
> sheet you get to avoid some of them. (Notably you could
> skip the whole 'negotiate presence of extensions' business
> by just getting those parts right from the start

...better still, if you can define (a) a mandatory "return
version and feature bit info" call right from the start
and (b) the required behaviour for attempts to make
a semihosting call with an unknown/unimplemented
function number -- then you can avoid the nasty
kludge "magic behaviour by opening a magic filename"
we were stuck with specifying in arm semihosting 2.0.

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Fri, 25 Oct 2019 at 20:15, Keith Packard <keithp@keithp.com> wrote:
> There seems to be convergence on a pretty simple interface which uses
> ebreak surrounded by a couple of specific no-ops:
>
>       slli x0, x0, 0x1f
>       ebreak
>       srai x0, x0, 0x7
>
> There are implementations in rust and openocd, and I've got one for
> picolibc. The risc-v semihosting code is sitting on a branch in my repo
> on github:
>
>         https://github.com/keith-packard/qemu/tree/riscv-semihost

I had an idle glance at this implementation, and this:

   uint32_t pre = opcode_at(&ctx->base, ctx->base.pc_next - 4);
   uint32_t ebreak = opcode_at(&ctx->base, ctx->base.pc_next);
   uint32_t post = opcode_at(&ctx->base, ctx->base.pc_next + 4);

(where opcode_at() is a wrapper for cpu_ldl_code()) has
some unfortunate side effects: if the previous instruction
is in the previous MMU page, or the following instruction
is in the next MMU page, you might incorrectly trigger
an exception (where QEMU will just longjmp straight out of
the cpu_ldl_code()) if that other page isn't actually mapped
in the guest's page table. You need to be careful not to access
code outside the page you're actually on unless you're really
going to execute it and are OK with it faulting.

I'm not sure what the best way on QEMU to identify this kind
of multiple-instruction-sequence is -- Richard might have an
idea. One approach would be to always take an exception
(or call a helper function) for the 'ebreak', and then
distinguish semihosting from other kinds of ebreak by doing
the load of the preceding/succeeding instructions at runtime
rather than translate time.

Does your semihosting spec expect to have the semihosting
call work if the sequence crosses a page boundary, the
code is being executed by a userspace process, and one of
the two pages has been paged out by the OS ?

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Peter Maydell <peter.maydell@linaro.org> writes:

> I had an idle glance at this implementation, and this:
>
>    uint32_t pre = opcode_at(&ctx->base, ctx->base.pc_next - 4);
>    uint32_t ebreak = opcode_at(&ctx->base, ctx->base.pc_next);
>    uint32_t post = opcode_at(&ctx->base, ctx->base.pc_next + 4);
>
> (where opcode_at() is a wrapper for cpu_ldl_code()) has
> some unfortunate side effects: if the previous instruction
> is in the previous MMU page, or the following instruction
> is in the next MMU page, you might incorrectly trigger
> an exception (where QEMU will just longjmp straight out of
> the cpu_ldl_code()) if that other page isn't actually mapped
> in the guest's page table. You need to be careful not to access
> code outside the page you're actually on unless you're really
> going to execute it and are OK with it faulting.

I can't even find the implementation of cpu_ldl_code; the qemu source
code is somewhat obscure in this area. But, longjmp'ing out of the
middle of that seems like a bad idea.

> Does your semihosting spec expect to have the semihosting
> call work if the sequence crosses a page boundary, the
> code is being executed by a userspace process, and one of
> the two pages has been paged out by the OS ?

You've seen the entirety of the RISC-V semihosting spec already.  For
now, perhaps we should limit RISC-V semihosting support to devices
without paging support and await a more complete spec.

As you suggest, disallowing the sequence from crossing a page boundary
would be a simple fix, but that would require wording changes to the
spec.

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Thu, 14 Nov 2019 at 18:05, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > I had an idle glance at this implementation, and this:
> >
> >    uint32_t pre = opcode_at(&ctx->base, ctx->base.pc_next - 4);
> >    uint32_t ebreak = opcode_at(&ctx->base, ctx->base.pc_next);
> >    uint32_t post = opcode_at(&ctx->base, ctx->base.pc_next + 4);
> >
> > (where opcode_at() is a wrapper for cpu_ldl_code()) has
> > some unfortunate side effects: if the previous instruction
> > is in the previous MMU page, or the following instruction
> > is in the next MMU page, you might incorrectly trigger
> > an exception (where QEMU will just longjmp straight out of
> > the cpu_ldl_code()) if that other page isn't actually mapped
> > in the guest's page table. You need to be careful not to access
> > code outside the page you're actually on unless you're really
> > going to execute it and are OK with it faulting.
>
> I can't even find the implementation of cpu_ldl_code; the qemu source
> code is somewhat obscure in this area. But, longjmp'ing out of the
> middle of that seems like a bad idea.

It's the way QEMU works -- generally load/store operations
that work on virtual addresses are expected to only return
in the success case; on failure they longjmp out to cause
the guest exception. (Load/stores on physical addresses
generally return a memory transaction status for the caller
to check and handle.) I agree that within the translation code
it's a bit weird and it might be nicer for the translate.c
code to explicitly handle failures to load an insn, but it
would be a bit of an upheaval to try to rewrite it at this point.

cpu_ldl_code() is provided by include/exec/cpu_ldst.h, incidentally
(via preprocessor macros and repeated inclusion of some template
.h files, which is why a grep for the function name misses it).

> > Does your semihosting spec expect to have the semihosting
> > call work if the sequence crosses a page boundary, the
> > code is being executed by a userspace process, and one of
> > the two pages has been paged out by the OS ?
>
> You've seen the entirety of the RISC-V semihosting spec already.  For
> now, perhaps we should limit RISC-V semihosting support to devices
> without paging support and await a more complete spec.
>
> As you suggest, disallowing the sequence from crossing a page boundary
> would be a simple fix, but that would require wording changes to the
> spec.

Yeah, I'm implicitly suggesting that a bit more thought/revision
of the spec might not be a bad idea. Things that are
effectively supposed to be treated like a single instruction
but which can cross cacheline or page boundaries turn out
to be a fertile source of implementation bugs. (The arm
translate.c code has to be quite careful about handling
32-bit Thumb insns that cross pages. The x86 translate.c
code is less careful and may well be buggy in this area.)

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Richard Henderson]

On 11/14/19 5:14 PM, Peter Maydell wrote:
> On Fri, 25 Oct 2019 at 20:15, Keith Packard <keithp@keithp.com> wrote:
>> There seems to be convergence on a pretty simple interface which uses
>> ebreak surrounded by a couple of specific no-ops:
>>
>>       slli x0, x0, 0x1f
>>       ebreak
>>       srai x0, x0, 0x7
>>
>> There are implementations in rust and openocd, and I've got one for
>> picolibc. The risc-v semihosting code is sitting on a branch in my repo
>> on github:
>>
>>         https://github.com/keith-packard/qemu/tree/riscv-semihost
> 
> I had an idle glance at this implementation, and this:
> 
>    uint32_t pre = opcode_at(&ctx->base, ctx->base.pc_next - 4);
>    uint32_t ebreak = opcode_at(&ctx->base, ctx->base.pc_next);
>    uint32_t post = opcode_at(&ctx->base, ctx->base.pc_next + 4);
> 
> (where opcode_at() is a wrapper for cpu_ldl_code()) has
> some unfortunate side effects: if the previous instruction
> is in the previous MMU page, or the following instruction
> is in the next MMU page, you might incorrectly trigger
> an exception (where QEMU will just longjmp straight out of
> the cpu_ldl_code()) if that other page isn't actually mapped
> in the guest's page table. You need to be careful not to access
> code outside the page you're actually on unless you're really
> going to execute it and are OK with it faulting.
> 
> I'm not sure what the best way on QEMU to identify this kind
> of multiple-instruction-sequence is -- Richard might have an
> idea. One approach would be to always take an exception
> (or call a helper function) for the 'ebreak', and then
> distinguish semihosting from other kinds of ebreak by doing
> the load of the preceding/succeeding instructions at runtime
> rather than translate time.

It's irritating that this 3 insn sequence is already a thing, baked into other
sim/emulators.  Seems to me that it would have been easier to abscond with

    ebreak + func3 field != 0.

For semi-hosting, it seems even better if the semi-hosting syscall instruction
is not "real", because you're explicitly requesting services from "unreal"
hardware.  It should be specified to generate a SIGILL type of exception
anywhere semi-hosting is not enabled.

That said, it is possible to do this in the translator, by considering this
sequence to be one 12-byte instruction.

You'd build in a recognizer into the decoder such that, when semi-hosting is
enabled, seeing the entry nop starts looking ahead:

  - If this is not the first insn in the TB, and there are
    not 8 more bytes remaining on the page, terminate the
    current TB.  This will re-start the machine in the next TB.

  - If the complete sequence is not found, then advance the pc
    past the entry nop and let nature take its course wrt the
    subsequent instructions.

  - If the sequence crosses a page, then so be it.  Because of
    step 1, this only happens when we *must* cross a page, and
    will have recognized any paging exception anyway.
    The generic parts of qemu will handle proper invalidation of
    a TB that crosses a page boundary.

  - This is kinda similar to the target/sh4 code to handle
    generic user-space atomic sequences: c.f. decode_gusa().

  - This implementation does mean that you can't jump directly
    to the ebreak insn and have the sequence be recognized.
    Control must flow through the entry nop.
    This is a bug or a feature depending on the reviewer.  ;-)

With that in mind, it may be simpler to handle all of this not in the
translator, but in the function that delivers the ebreak exception.  At that
point one can arrange to read memory without raising additional exceptions.


r~

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Thu, 14 Nov 2019 at 19:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>   - If the sequence crosses a page, then so be it.  Because of
>     step 1, this only happens when we *must* cross a page, and
>     will have recognized any paging exception anyway.
>     The generic parts of qemu will handle proper invalidation of
>     a TB that crosses a page boundary.

I'm not sure this would work. If you have
  insn1 insn2 || other-insn
(where || is the page boundary and page 2 is non-executable)
then the required behaviour is "execute insn1 and insn2 with
normal behaviour, then fault trying to read other-insn, with
the fault address being that of other-insn".
Whereas for
  insn1 insn2 || insn3
you want to treat it as a semihosting sequence. But you can't distinguish
the two because trying to read the word in page 2 will cause us to
generate a fault with the fault address being that of insn1. Or
have I forgotten how the page-crossing handling works ?

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Richard Henderson]

On 11/14/19 8:29 PM, Peter Maydell wrote:
> On Thu, 14 Nov 2019 at 19:18, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>   - If the sequence crosses a page, then so be it.  Because of
>>     step 1, this only happens when we *must* cross a page, and
>>     will have recognized any paging exception anyway.
>>     The generic parts of qemu will handle proper invalidation of
>>     a TB that crosses a page boundary.
> 
> I'm not sure this would work. If you have
>   insn1 insn2 || other-insn
> (where || is the page boundary and page 2 is non-executable)
> then the required behaviour is "execute insn1 and insn2 with
> normal behaviour, then fault trying to read other-insn, with
> the fault address being that of other-insn".
> Whereas for
>   insn1 insn2 || insn3
> you want to treat it as a semihosting sequence. But you can't distinguish
> the two because trying to read the word in page 2 will cause us to
> generate a fault with the fault address being that of insn1. Or
> have I forgotten how the page-crossing handling works ?

Yet another reason why I prefer any semi-hosting call to use an encoding that
is otherwise reserved illegal.

For this, you have to make up your mind: is it important to execute the
instructions as specified by the ISA, or as specified by the semi-hosting spec?

In this case, semi-hosting defines an "entry nop" that begins the sequence, and
I think that we are well within our rights to ignore the validity of "insn1
insn2 || other-insn".


r~

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Thu, 14 Nov 2019 at 20:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Yet another reason why I prefer any semi-hosting call to use an encoding that
> is otherwise reserved illegal.
>
> For this, you have to make up your mind: is it important to execute the
> instructions as specified by the ISA, or as specified by the semi-hosting spec?
>
> In this case, semi-hosting defines an "entry nop" that begins the sequence, and
> I think that we are well within our rights to ignore the validity of "insn1
> insn2 || other-insn".

Perhaps. I think you get the same issue with
  insn1 || insn2
vs
  insn1 || some-other-insn

though. (And the spec has wording that explicitly wants the
latter to be handled with the normal "I'm a hint instruction"
behaviour of insn1.)

-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

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

> For semi-hosting, it seems even better if the semi-hosting syscall instruction
> is not "real", because you're explicitly requesting services from "unreal"
> hardware.  It should be specified to generate a SIGILL type of exception
> anywhere semi-hosting is not enabled.

In the QEMU case, yes, it's virtual hardware. However, the other common case
for semihosting is when doing hardware bringup using JTAG or other
remote debugging link -- having an instruction which safely traps to the
debugger is required to make semihosting usable there. Reading through
the history of the current RISC-V semihosting mechanism, there were many
designs considered and rejected because they wouldn't work in the JTAG
environment.

> With that in mind, it may be simpler to handle all of this not in the
> translator, but in the function that delivers the ebreak exception.  At that
> point one can arrange to read memory without raising additional
> exceptions.

I'll go explore and see if I can figure any of this out.

I'd still like to get the non-RISC-V SYS_READC patch landed someday :-)

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell]

On Thu, 14 Nov 2019 at 22:27, Keith Packard <keithp@keithp.com> wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
> > For semi-hosting, it seems even better if the semi-hosting syscall instruction
> > is not "real", because you're explicitly requesting services from "unreal"
> > hardware.  It should be specified to generate a SIGILL type of exception
> > anywhere semi-hosting is not enabled.
>
> In the QEMU case, yes, it's virtual hardware. However, the other common case
> for semihosting is when doing hardware bringup using JTAG or other
> remote debugging link -- having an instruction which safely traps to the
> debugger is required to make semihosting usable there. Reading through
> the history of the current RISC-V semihosting mechanism, there were many
> designs considered and rejected because they wouldn't work in the JTAG
> environment.
>
> > With that in mind, it may be simpler to handle all of this not in the
> > translator, but in the function that delivers the ebreak exception.  At that
> > point one can arrange to read memory without raising additional
> > exceptions.
>
> I'll go explore and see if I can figure any of this out.
>
> I'd still like to get the non-RISC-V SYS_READC patch landed someday :-)

It's on my queue to review if nobody else gets to it first, but since
we're in freeze right now it won't be landing til after the release
happens (expected mid-December).

thanks
-- PMM

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Keith Packard]

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

Peter Maydell <peter.maydell@linaro.org> writes:

> It's on my queue to review if nobody else gets to it first, but since
> we're in freeze right now it won't be landing til after the release
> happens (expected mid-December).

Thanks in advance! I'll get started pushing questions about the RISC-V
semihosting ABI into that standard group and see if we can't at least
have the existing situation clarified. I think at a minimum we need:

 1) Explicit reference to the intended ARM API, with an explicit mapping
    between ARM architecture concepts to the ones used for RISC-V,
    especially for how arguments are passed.

 2) Resolution of how to handle sequence which cross page boundaries.

-- 
-keith

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

Re: [PATCH] Semihost SYS_READC implementation (v4)

[Alex Bennée]

Keith Packard <keithp@keithp.com> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
<snip>
>> Please keep version history bellow --- so they get dropped when the
>> patch is applied.
>
> Sure, I'll edit the mail before sending. In my repo, I'm leaving the
> version history in git so I can keep track of it.

It's OK to keep the history in git, I do it all the time. But by using a
--- divider in your commit message the git-am tool will trim it off when
it gets applied from a mailing list. There is no need to manually edit
your patches as that would be error prone.

To pick a random example of what I mean from my tree:

  https://github.com/stsquad/qemu/commit/1b648ac695d45a4e4d72cf64a97346d9695b863b

--
Alex Bennée

Re: [PATCH] Semihost SYS_READC implementation (v4)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191024224622.12371-1-keithp@keithp.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/trace/generated-helpers.o
../vl.o: In function `main':
/tmp/qemu-test/src/vl.c:4385: undefined reference to `qemu_semihosting_console_init'
collect2: error: ld returned 1 exit status
make[1]: *** [qemu-system-x86_64] Error 1
make: *** [x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
  LINK    aarch64-softmmu/qemu-system-aarch64
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=56d753bff38f4c6794bd90f9a5f3e2df', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-kcdksyw0/src/docker-src.2019-10-25-14.14.31.23494:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=56d753bff38f4c6794bd90f9a5f3e2df
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-kcdksyw0/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m45.581s
user    0m8.220s


The full log is available at
http://patchew.org/logs/20191024224622.12371-1-keithp@keithp.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] Semihost SYS_READC implementation (v4)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191024224622.12371-1-keithp@keithp.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/target/arm/translate-sve.o
../vl.o: In function `qemu_main':
/tmp/qemu-test/src/vl.c:4385: undefined reference to `qemu_semihosting_console_init'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:206: qemu-system-x86_64w.exe] Error 1
make: *** [Makefile:482: x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
  LINK    aarch64-softmmu/qemu-system-aarch64w.exe
  GEN     aarch64-softmmu/qemu-system-aarch64.exe
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=c8023af7d17f49c389a9dbb7c2292a6e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-wurz4hht/src/docker-src.2019-10-25-14.17.51.2074:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=c8023af7d17f49c389a9dbb7c2292a6e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-wurz4hht/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m52.599s
user    0m8.472s


The full log is available at
http://patchew.org/logs/20191024224622.12371-1-keithp@keithp.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] Semihost SYS_READC implementation (v3)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191023192640.13125-1-keithp@keithp.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/target/arm/translate-sve.o
../vl.o: In function `qemu_main':
/tmp/qemu-test/src/vl.c:4385: undefined reference to `qemu_semihosting_console_init'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:206: qemu-system-x86_64w.exe] Error 1
make: *** [Makefile:482: x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
  LINK    aarch64-softmmu/qemu-system-aarch64w.exe
  GEN     aarch64-softmmu/qemu-system-aarch64.exe
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2075532ea0a14cd29becb8e11046979f', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-62cgc9xy/src/docker-src.2019-10-24-13.40.35.23389:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=2075532ea0a14cd29becb8e11046979f
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-62cgc9xy/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    3m14.419s
user    0m7.056s


The full log is available at
http://patchew.org/logs/20191023192640.13125-1-keithp@keithp.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com