[PATCH v5 0/2] target/xtensa: semihosting cleanup

[PATCH v5 0/2] target/xtensa: semihosting cleanup

[Richard Henderson]

Changes for v5:
  * Rebase on master.

r~


Richard Henderson (2):
  target/xtensa: Use an exception for semihosting
  target/xtensa: Use semihosting/syscalls.h

 target/xtensa/cpu.h         |   3 +-
 target/xtensa/helper.h      |   3 -
 hw/xtensa/sim.c             |   3 -
 target/xtensa/exc_helper.c  |   4 +
 target/xtensa/translate.c   |   3 +-
 target/xtensa/xtensa-semi.c | 229 ++++++++----------------------------
 6 files changed, 59 insertions(+), 186 deletions(-)

-- 
2.34.1

[PATCH v5 1/2] target/xtensa: Use an exception for semihosting

[Richard Henderson]

Within do_interrupt, we hold the iothread lock, which
is required for Chardev access for the console, and for
the round trip for use_gdb_syscalls().

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/xtensa/cpu.h         | 2 ++
 target/xtensa/helper.h      | 3 ---
 target/xtensa/exc_helper.c  | 4 ++++
 target/xtensa/translate.c   | 3 ++-
 target/xtensa/xtensa-semi.c | 3 +--
 5 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 579adcb769..ea66895e7f 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -260,6 +260,7 @@ enum {
     EXC_USER,
     EXC_DOUBLE,
     EXC_DEBUG,
+    EXC_SEMIHOST,
     EXC_MAX
 };
 
@@ -576,6 +577,7 @@ void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
                                       unsigned size, MMUAccessType access_type,
                                       int mmu_idx, MemTxAttrs attrs,
                                       MemTxResult response, uintptr_t retaddr);
+void xtensa_semihosting(CPUXtensaState *env);
 #endif
 void xtensa_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 hwaddr xtensa_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
diff --git a/target/xtensa/helper.h b/target/xtensa/helper.h
index ae938ceedb..531679cd86 100644
--- a/target/xtensa/helper.h
+++ b/target/xtensa/helper.h
@@ -11,9 +11,6 @@ DEF_HELPER_2(retw, void, env, i32)
 DEF_HELPER_3(window_check, noreturn, env, i32, i32)
 DEF_HELPER_1(restore_owb, void, env)
 DEF_HELPER_2(movsp, void, env, i32)
-#ifndef CONFIG_USER_ONLY
-DEF_HELPER_1(simcall, void, env)
-#endif
 
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_3(waiti, void, env, i32, i32)
diff --git a/target/xtensa/exc_helper.c b/target/xtensa/exc_helper.c
index d4823a65cd..d54a518875 100644
--- a/target/xtensa/exc_helper.c
+++ b/target/xtensa/exc_helper.c
@@ -219,6 +219,10 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
     }
 
     switch (cs->exception_index) {
+    case EXC_SEMIHOST:
+        xtensa_semihosting(env);
+        return;
+
     case EXC_WINDOW_OVERFLOW4:
     case EXC_WINDOW_UNDERFLOW4:
     case EXC_WINDOW_OVERFLOW8:
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 70e11eeb45..b65c8b8428 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -2377,7 +2377,8 @@ static void translate_simcall(DisasContext *dc, const OpcodeArg arg[],
 {
 #ifndef CONFIG_USER_ONLY
     if (semihosting_enabled()) {
-        gen_helper_simcall(cpu_env);
+        tcg_gen_movi_i32(cpu_pc, dc->base.pc_next);
+        gen_exception(dc, EXC_SEMIHOST);
     }
 #endif
 }
diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
index fa21b7e11f..5375f106fc 100644
--- a/target/xtensa/xtensa-semi.c
+++ b/target/xtensa/xtensa-semi.c
@@ -28,7 +28,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "chardev/char-fe.h"
-#include "exec/helper-proto.h"
 #include "semihosting/semihost.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
@@ -188,7 +187,7 @@ void xtensa_sim_open_console(Chardev *chr)
     sim_console = &console;
 }
 
-void HELPER(simcall)(CPUXtensaState *env)
+void xtensa_semihosting(CPUXtensaState *env)
 {
     CPUState *cs = env_cpu(env);
     uint32_t *regs = env->regs;
-- 
2.34.1

[PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Richard Henderson]

This separates guest file descriptors from host file descriptors,
and utilizes shared infrastructure for integration with gdbstub.
Remove the xtensa custom console handing and rely on the
generic -semihosting-config handling of chardevs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/xtensa/cpu.h         |   1 -
 hw/xtensa/sim.c             |   3 -
 target/xtensa/xtensa-semi.c | 226 ++++++++----------------------------
 3 files changed, 50 insertions(+), 180 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index ea66895e7f..99ac3efd71 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -612,7 +612,6 @@ void xtensa_translate_init(void);
 void **xtensa_get_regfile_by_name(const char *name, int entries, int bits);
 void xtensa_breakpoint_handler(CPUState *cs);
 void xtensa_register_core(XtensaConfigList *node);
-void xtensa_sim_open_console(Chardev *chr);
 void check_interrupts(CPUXtensaState *s);
 void xtensa_irq_init(CPUXtensaState *env);
 qemu_irq *xtensa_get_extints(CPUXtensaState *env);
diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
index 946c71cb5b..5cca6a170e 100644
--- a/hw/xtensa/sim.c
+++ b/hw/xtensa/sim.c
@@ -87,9 +87,6 @@ XtensaCPU *xtensa_sim_common_init(MachineState *machine)
         xtensa_create_memory_regions(&sysram, "xtensa.sysram",
                                      get_system_memory());
     }
-    if (serial_hd(0)) {
-        xtensa_sim_open_console(serial_hd(0));
-    }
     return cpu;
 }
 
diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
index 5375f106fc..79431f5a64 100644
--- a/target/xtensa/xtensa-semi.c
+++ b/target/xtensa/xtensa-semi.c
@@ -27,8 +27,10 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "chardev/char-fe.h"
+#include "exec/gdbstub.h"
 #include "semihosting/semihost.h"
+#include "semihosting/syscalls.h"
+#include "semihosting/softmmu-uaccess.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
 
@@ -143,48 +145,21 @@ static uint32_t errno_h2g(int host_errno)
     return TARGET_EINVAL;
 }
 
-typedef struct XtensaSimConsole {
-    CharBackend be;
-    struct {
-        char buffer[16];
-        size_t offset;
-    } input;
-} XtensaSimConsole;
-
-static XtensaSimConsole *sim_console;
-
-static IOCanReadHandler sim_console_can_read;
-static int sim_console_can_read(void *opaque)
+static void xtensa_cb(CPUState *cs, uint64_t ret, int err)
 {
-    XtensaSimConsole *p = opaque;
+    CPUXtensaState *env = cs->env_ptr;
 
-    return sizeof(p->input.buffer) - p->input.offset;
+    env->regs[3] = errno_h2g(err);
+    env->regs[2] = ret;
 }
 
-static IOReadHandler sim_console_read;
-static void sim_console_read(void *opaque, const uint8_t *buf, int size)
+static void xtensa_select_cb(CPUState *cs, uint64_t ret, int err)
 {
-    XtensaSimConsole *p = opaque;
-    size_t copy = sizeof(p->input.buffer) - p->input.offset;
-
-    if (size < copy) {
-        copy = size;
+    if (ret & G_IO_NVAL) {
+        xtensa_cb(cs, -1, EBADF);
+    } else {
+        xtensa_cb(cs, ret != 0, 0);
     }
-    memcpy(p->input.buffer + p->input.offset, buf, copy);
-    p->input.offset += copy;
-}
-
-void xtensa_sim_open_console(Chardev *chr)
-{
-    static XtensaSimConsole console;
-
-    qemu_chr_fe_init(&console.be, chr, &error_abort);
-    qemu_chr_fe_set_handlers(&console.be,
-                             sim_console_can_read,
-                             sim_console_read,
-                             NULL, NULL, &console,
-                             NULL, true);
-    sim_console = &console;
 }
 
 void xtensa_semihosting(CPUXtensaState *env)
@@ -194,165 +169,64 @@ void xtensa_semihosting(CPUXtensaState *env)
 
     switch (regs[2]) {
     case TARGET_SYS_exit:
+        gdb_exit(regs[3]);
         exit(regs[3]);
         break;
 
     case TARGET_SYS_read:
+        semihost_sys_read(cs, xtensa_cb, regs[3], regs[4], regs[5]);
+        break;
     case TARGET_SYS_write:
-        {
-            bool is_write = regs[2] == TARGET_SYS_write;
-            uint32_t fd = regs[3];
-            uint32_t vaddr = regs[4];
-            uint32_t len = regs[5];
-            uint32_t len_done = 0;
-
-            while (len > 0) {
-                hwaddr paddr = cpu_get_phys_page_debug(cs, vaddr);
-                uint32_t page_left =
-                    TARGET_PAGE_SIZE - (vaddr & (TARGET_PAGE_SIZE - 1));
-                uint32_t io_sz = page_left < len ? page_left : len;
-                hwaddr sz = io_sz;
-                void *buf = cpu_physical_memory_map(paddr, &sz, !is_write);
-                uint32_t io_done;
-                bool error = false;
-
-                if (buf) {
-                    vaddr += io_sz;
-                    len -= io_sz;
-                    if (fd < 3 && sim_console) {
-                        if (is_write && (fd == 1 || fd == 2)) {
-                            io_done = qemu_chr_fe_write_all(&sim_console->be,
-                                                            buf, io_sz);
-                            regs[3] = errno_h2g(errno);
-                        } else if (!is_write && fd == 0) {
-                            if (sim_console->input.offset) {
-                                io_done = sim_console->input.offset;
-                                if (io_sz < io_done) {
-                                    io_done = io_sz;
-                                }
-                                memcpy(buf, sim_console->input.buffer, io_done);
-                                memmove(sim_console->input.buffer,
-                                        sim_console->input.buffer + io_done,
-                                        sim_console->input.offset - io_done);
-                                sim_console->input.offset -= io_done;
-                                qemu_chr_fe_accept_input(&sim_console->be);
-                            } else {
-                                io_done = -1;
-                                regs[3] = TARGET_EAGAIN;
-                            }
-                        } else {
-                            qemu_log_mask(LOG_GUEST_ERROR,
-                                          "%s fd %d is not supported with chardev console\n",
-                                          is_write ?
-                                          "writing to" : "reading from", fd);
-                            io_done = -1;
-                            regs[3] = TARGET_EBADF;
-                        }
-                    } else {
-                        io_done = is_write ?
-                            write(fd, buf, io_sz) :
-                            read(fd, buf, io_sz);
-                        regs[3] = errno_h2g(errno);
-                    }
-                    if (io_done == -1) {
-                        error = true;
-                        io_done = 0;
-                    }
-                    cpu_physical_memory_unmap(buf, sz, !is_write, io_done);
-                } else {
-                    error = true;
-                    regs[3] = TARGET_EINVAL;
-                    break;
-                }
-                if (error) {
-                    if (!len_done) {
-                        len_done = -1;
-                    }
-                    break;
-                }
-                len_done += io_done;
-                if (io_done < io_sz) {
-                    break;
-                }
-            }
-            regs[2] = len_done;
-        }
+        semihost_sys_write(cs, xtensa_cb, regs[3], regs[4], regs[5]);
         break;
-
     case TARGET_SYS_open:
-        {
-            char name[1024];
-            int rc;
-            int i;
-
-            for (i = 0; i < ARRAY_SIZE(name); ++i) {
-                rc = cpu_memory_rw_debug(cs, regs[3] + i,
-                                         (uint8_t *)name + i, 1, 0);
-                if (rc != 0 || name[i] == 0) {
-                    break;
-                }
-            }
-
-            if (rc == 0 && i < ARRAY_SIZE(name)) {
-                regs[2] = open(name, regs[4], regs[5]);
-                regs[3] = errno_h2g(errno);
-            } else {
-                regs[2] = -1;
-                regs[3] = TARGET_EINVAL;
-            }
-        }
+        semihost_sys_open(cs, xtensa_cb, regs[3], 0, regs[4], regs[5]);
         break;
-
     case TARGET_SYS_close:
-        if (regs[3] < 3) {
-            regs[2] = regs[3] = 0;
-        } else {
-            regs[2] = close(regs[3]);
-            regs[3] = errno_h2g(errno);
-        }
+        semihost_sys_close(cs, xtensa_cb, regs[3]);
         break;
-
     case TARGET_SYS_lseek:
-        regs[2] = lseek(regs[3], (off_t)(int32_t)regs[4], regs[5]);
-        regs[3] = errno_h2g(errno);
+        semihost_sys_lseek(cs, xtensa_cb, regs[3], regs[4], regs[5]);
         break;
 
     case TARGET_SYS_select_one:
         {
-            uint32_t fd = regs[3];
-            uint32_t rq = regs[4];
-            uint32_t target_tv = regs[5];
-            uint32_t target_tvv[2];
+            int timeout, events;
 
-            struct timeval tv = {0};
+            if (regs[5]) {
+                uint32_t tv_sec, tv_usec;
+                uint64_t msec;
 
-            if (target_tv) {
-                cpu_memory_rw_debug(cs, target_tv,
-                        (uint8_t *)target_tvv, sizeof(target_tvv), 0);
-                tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
-                tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
-            }
-            if (fd < 3 && sim_console) {
-                if ((fd == 1 || fd == 2) && rq == SELECT_ONE_WRITE) {
-                    regs[2] = 1;
-                } else if (fd == 0 && rq == SELECT_ONE_READ) {
-                    regs[2] = sim_console->input.offset > 0;
-                } else {
-                    regs[2] = 0;
+                if (get_user_u32(tv_sec, regs[5]) ||
+                    get_user_u32(tv_usec, regs[5])) {
+                    xtensa_cb(cs, -1, EFAULT);
+                    return;
                 }
-                regs[3] = 0;
-            } else {
-                fd_set fdset;
 
-                FD_ZERO(&fdset);
-                FD_SET(fd, &fdset);
-                regs[2] = select(fd + 1,
-                                 rq == SELECT_ONE_READ   ? &fdset : NULL,
-                                 rq == SELECT_ONE_WRITE  ? &fdset : NULL,
-                                 rq == SELECT_ONE_EXCEPT ? &fdset : NULL,
-                                 target_tv ? &tv : NULL);
-                regs[3] = errno_h2g(errno);
+                /* Poll timeout is in milliseconds; overflow to infinity. */
+                msec = tv_sec * 1000ull + DIV_ROUND_UP(tv_usec, 1000ull);
+                timeout = msec <= INT32_MAX ? msec : -1;
+            } else {
+                timeout = -1;
             }
+
+            switch (regs[4]) {
+            case SELECT_ONE_READ:
+                events = G_IO_IN;
+                break;
+            case SELECT_ONE_WRITE:
+                events = G_IO_OUT;
+                break;
+            case SELECT_ONE_EXCEPT:
+                events = G_IO_PRI;
+                break;
+            default:
+                xtensa_cb(cs, -1, EINVAL);
+                return;
+            }
+
+            semihost_sys_poll_one(cs, xtensa_select_cb,
+                                  regs[3], events, timeout);
         }
         break;
 
-- 
2.34.1

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Max Filippov]

On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This separates guest file descriptors from host file descriptors,
> and utilizes shared infrastructure for integration with gdbstub.
> Remove the xtensa custom console handing and rely on the
> generic -semihosting-config handling of chardevs.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/xtensa/cpu.h         |   1 -
>  hw/xtensa/sim.c             |   3 -
>  target/xtensa/xtensa-semi.c | 226 ++++++++----------------------------
>  3 files changed, 50 insertions(+), 180 deletions(-)
>
> diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
> index ea66895e7f..99ac3efd71 100644
> --- a/target/xtensa/cpu.h
> +++ b/target/xtensa/cpu.h
> @@ -612,7 +612,6 @@ void xtensa_translate_init(void);
>  void **xtensa_get_regfile_by_name(const char *name, int entries, int bits);
>  void xtensa_breakpoint_handler(CPUState *cs);
>  void xtensa_register_core(XtensaConfigList *node);
> -void xtensa_sim_open_console(Chardev *chr);
>  void check_interrupts(CPUXtensaState *s);
>  void xtensa_irq_init(CPUXtensaState *env);
>  qemu_irq *xtensa_get_extints(CPUXtensaState *env);
> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> index 946c71cb5b..5cca6a170e 100644
> --- a/hw/xtensa/sim.c
> +++ b/hw/xtensa/sim.c
> @@ -87,9 +87,6 @@ XtensaCPU *xtensa_sim_common_init(MachineState *machine)
>          xtensa_create_memory_regions(&sysram, "xtensa.sysram",
>                                       get_system_memory());
>      }
> -    if (serial_hd(0)) {
> -        xtensa_sim_open_console(serial_hd(0));
> -    }

I've noticed that with this change '-serial stdio' and its variants are still
accepted in the command line, but now they do nothing. This quiet
change of behavior is unfortunate. I wonder if it would be acceptable
to map the '-serial stdio' option in the presence of '-semihosting' to
something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?

> @@ -194,165 +169,64 @@ void xtensa_semihosting(CPUXtensaState *env)

...

>      case TARGET_SYS_select_one:
>          {
> -            uint32_t fd = regs[3];
> -            uint32_t rq = regs[4];
> -            uint32_t target_tv = regs[5];
> -            uint32_t target_tvv[2];
> +            int timeout, events;
>
> -            struct timeval tv = {0};
> +            if (regs[5]) {
> +                uint32_t tv_sec, tv_usec;
> +                uint64_t msec;
>
> -            if (target_tv) {
> -                cpu_memory_rw_debug(cs, target_tv,
> -                        (uint8_t *)target_tvv, sizeof(target_tvv), 0);
> -                tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
> -                tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
> -            }
> -            if (fd < 3 && sim_console) {
> -                if ((fd == 1 || fd == 2) && rq == SELECT_ONE_WRITE) {
> -                    regs[2] = 1;
> -                } else if (fd == 0 && rq == SELECT_ONE_READ) {
> -                    regs[2] = sim_console->input.offset > 0;
> -                } else {
> -                    regs[2] = 0;
> +                if (get_user_u32(tv_sec, regs[5]) ||
> +                    get_user_u32(tv_usec, regs[5])) {

get_user_u32(tv_usec, regs[5] + 4)?

> +                    xtensa_cb(cs, -1, EFAULT);
> +                    return;
>                  }
> -                regs[3] = 0;
> -            } else {
> -                fd_set fdset;
>
> -                FD_ZERO(&fdset);
> -                FD_SET(fd, &fdset);
> -                regs[2] = select(fd + 1,
> -                                 rq == SELECT_ONE_READ   ? &fdset : NULL,
> -                                 rq == SELECT_ONE_WRITE  ? &fdset : NULL,
> -                                 rq == SELECT_ONE_EXCEPT ? &fdset : NULL,
> -                                 target_tv ? &tv : NULL);
> -                regs[3] = errno_h2g(errno);
> +                /* Poll timeout is in milliseconds; overflow to infinity. */
> +                msec = tv_sec * 1000ull + DIV_ROUND_UP(tv_usec, 1000ull);
> +                timeout = msec <= INT32_MAX ? msec : -1;
> +            } else {
> +                timeout = -1;
>              }
> +
> +            switch (regs[4]) {
> +            case SELECT_ONE_READ:
> +                events = G_IO_IN;
> +                break;
> +            case SELECT_ONE_WRITE:
> +                events = G_IO_OUT;
> +                break;
> +            case SELECT_ONE_EXCEPT:
> +                events = G_IO_PRI;
> +                break;
> +            default:
> +                xtensa_cb(cs, -1, EINVAL);

This doesn't match what there used to be: it was possible to call
select_one with rq other than SELECT_ONE_* and that would've
passed NULL for all fd sets in the select invocation turning it into
a sleep. It would return 0 after the timeout.

-- 
Thanks.
-- Max

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Richard Henderson]

On 6/28/22 19:08, Max Filippov wrote:
> On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This separates guest file descriptors from host file descriptors,
>> and utilizes shared infrastructure for integration with gdbstub.
>> Remove the xtensa custom console handing and rely on the
>> generic -semihosting-config handling of chardevs.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/xtensa/cpu.h         |   1 -
>>   hw/xtensa/sim.c             |   3 -
>>   target/xtensa/xtensa-semi.c | 226 ++++++++----------------------------
>>   3 files changed, 50 insertions(+), 180 deletions(-)
>>
>> diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
>> index ea66895e7f..99ac3efd71 100644
>> --- a/target/xtensa/cpu.h
>> +++ b/target/xtensa/cpu.h
>> @@ -612,7 +612,6 @@ void xtensa_translate_init(void);
>>   void **xtensa_get_regfile_by_name(const char *name, int entries, int bits);
>>   void xtensa_breakpoint_handler(CPUState *cs);
>>   void xtensa_register_core(XtensaConfigList *node);
>> -void xtensa_sim_open_console(Chardev *chr);
>>   void check_interrupts(CPUXtensaState *s);
>>   void xtensa_irq_init(CPUXtensaState *env);
>>   qemu_irq *xtensa_get_extints(CPUXtensaState *env);
>> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
>> index 946c71cb5b..5cca6a170e 100644
>> --- a/hw/xtensa/sim.c
>> +++ b/hw/xtensa/sim.c
>> @@ -87,9 +87,6 @@ XtensaCPU *xtensa_sim_common_init(MachineState *machine)
>>           xtensa_create_memory_regions(&sysram, "xtensa.sysram",
>>                                        get_system_memory());
>>       }
>> -    if (serial_hd(0)) {
>> -        xtensa_sim_open_console(serial_hd(0));
>> -    }
> 
> I've noticed that with this change '-serial stdio' and its variants are still
> accepted in the command line, but now they do nothing.

Pardon?  They certainly will do something, via writes to the serial hardware.


> This quiet
> change of behavior is unfortunate. I wonder if it would be acceptable
> to map the '-serial stdio' option in the presence of '-semihosting' to
> something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?

I dunno.  I'm wary of having xtensa be unique here.  Alex, thoughts?

>> +                if (get_user_u32(tv_sec, regs[5]) ||
>> +                    get_user_u32(tv_usec, regs[5])) {
> 
> get_user_u32(tv_usec, regs[5] + 4)?

Oops, yes.

>> -                regs[2] = select(fd + 1,
>> -                                 rq == SELECT_ONE_READ   ? &fdset : NULL,
>> -                                 rq == SELECT_ONE_WRITE  ? &fdset : NULL,
>> -                                 rq == SELECT_ONE_EXCEPT ? &fdset : NULL,
>> -                                 target_tv ? &tv : NULL);
>> -                regs[3] = errno_h2g(errno);
>> +                /* Poll timeout is in milliseconds; overflow to infinity. */
>> +                msec = tv_sec * 1000ull + DIV_ROUND_UP(tv_usec, 1000ull);
>> +                timeout = msec <= INT32_MAX ? msec : -1;
>> +            } else {
>> +                timeout = -1;
>>               }
>> +
>> +            switch (regs[4]) {
>> +            case SELECT_ONE_READ:
>> +                events = G_IO_IN;
>> +                break;
>> +            case SELECT_ONE_WRITE:
>> +                events = G_IO_OUT;
>> +                break;
>> +            case SELECT_ONE_EXCEPT:
>> +                events = G_IO_PRI;
>> +                break;
>> +            default:
>> +                xtensa_cb(cs, -1, EINVAL);
> 
> This doesn't match what there used to be: it was possible to call
> select_one with rq other than SELECT_ONE_* and that would've
> passed NULL for all fd sets in the select invocation turning it into
> a sleep. It would return 0 after the timeout.

Hmm.  Is there any documentation of what it was *supposed* to do?  Passing rq == 
0xdeadbeef and expecting a specific behaviour seems odd.


r~

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Alex Bennée]

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

> On 6/28/22 19:08, Max Filippov wrote:
>> On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> This separates guest file descriptors from host file descriptors,
>>> and utilizes shared infrastructure for integration with gdbstub.
>>> Remove the xtensa custom console handing and rely on the
>>> generic -semihosting-config handling of chardevs.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>   target/xtensa/cpu.h         |   1 -
>>>   hw/xtensa/sim.c             |   3 -
>>>   target/xtensa/xtensa-semi.c | 226 ++++++++----------------------------
>>>   3 files changed, 50 insertions(+), 180 deletions(-)
>>>
>>> diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
>>> index ea66895e7f..99ac3efd71 100644
>>> --- a/target/xtensa/cpu.h
>>> +++ b/target/xtensa/cpu.h
>>> @@ -612,7 +612,6 @@ void xtensa_translate_init(void);
>>>   void **xtensa_get_regfile_by_name(const char *name, int entries, int bits);
>>>   void xtensa_breakpoint_handler(CPUState *cs);
>>>   void xtensa_register_core(XtensaConfigList *node);
>>> -void xtensa_sim_open_console(Chardev *chr);
>>>   void check_interrupts(CPUXtensaState *s);
>>>   void xtensa_irq_init(CPUXtensaState *env);
>>>   qemu_irq *xtensa_get_extints(CPUXtensaState *env);
>>> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
>>> index 946c71cb5b..5cca6a170e 100644
>>> --- a/hw/xtensa/sim.c
>>> +++ b/hw/xtensa/sim.c
>>> @@ -87,9 +87,6 @@ XtensaCPU *xtensa_sim_common_init(MachineState *machine)
>>>           xtensa_create_memory_regions(&sysram, "xtensa.sysram",
>>>                                        get_system_memory());
>>>       }
>>> -    if (serial_hd(0)) {
>>> -        xtensa_sim_open_console(serial_hd(0));
>>> -    }
>> I've noticed that with this change '-serial stdio' and its variants
>> are still
>> accepted in the command line, but now they do nothing.
>
> Pardon?  They certainly will do something, via writes to the serial hardware.
>
>
>> This quiet
>> change of behavior is unfortunate. I wonder if it would be acceptable
>> to map the '-serial stdio' option in the presence of '-semihosting' to
>> something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?
>
> I dunno.  I'm wary of having xtensa be unique here.  Alex, thoughts?

Is semihosting *the* serial hardware for xtensa-sim or is it overriding
another serial interface? I'm wary of adding more magical behaviour for
-serial as it can be confusing enough already what actually gets routed
to it if not doing everything explicitly.

>
>>> +                if (get_user_u32(tv_sec, regs[5]) ||
>>> +                    get_user_u32(tv_usec, regs[5])) {
>> get_user_u32(tv_usec, regs[5] + 4)?
>
> Oops, yes.
>
>>> -                regs[2] = select(fd + 1,
>>> -                                 rq == SELECT_ONE_READ   ? &fdset : NULL,
>>> -                                 rq == SELECT_ONE_WRITE  ? &fdset : NULL,
>>> -                                 rq == SELECT_ONE_EXCEPT ? &fdset : NULL,
>>> -                                 target_tv ? &tv : NULL);
>>> -                regs[3] = errno_h2g(errno);
>>> +                /* Poll timeout is in milliseconds; overflow to infinity. */
>>> +                msec = tv_sec * 1000ull + DIV_ROUND_UP(tv_usec, 1000ull);
>>> +                timeout = msec <= INT32_MAX ? msec : -1;
>>> +            } else {
>>> +                timeout = -1;
>>>               }
>>> +
>>> +            switch (regs[4]) {
>>> +            case SELECT_ONE_READ:
>>> +                events = G_IO_IN;
>>> +                break;
>>> +            case SELECT_ONE_WRITE:
>>> +                events = G_IO_OUT;
>>> +                break;
>>> +            case SELECT_ONE_EXCEPT:
>>> +                events = G_IO_PRI;
>>> +                break;
>>> +            default:
>>> +                xtensa_cb(cs, -1, EINVAL);
>> This doesn't match what there used to be: it was possible to call
>> select_one with rq other than SELECT_ONE_* and that would've
>> passed NULL for all fd sets in the select invocation turning it into
>> a sleep. It would return 0 after the timeout.
>
> Hmm.  Is there any documentation of what it was *supposed* to do?
> Passing rq == 0xdeadbeef and expecting a specific behaviour seems odd.
>
>
> r~


-- 
Alex Bennée

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Max Filippov]

On Wed, Jun 29, 2022 at 1:09 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
> > On 6/28/22 19:08, Max Filippov wrote:
> >> On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
> >> <richard.henderson@linaro.org> wrote:

> >>>       }
> >>> -    if (serial_hd(0)) {
> >>> -        xtensa_sim_open_console(serial_hd(0));
> >>> -    }
> >> I've noticed that with this change '-serial stdio' and its variants
> >> are still
> >> accepted in the command line, but now they do nothing.
> >
> > Pardon?  They certainly will do something, via writes to the serial hardware.
> >
> >
> >> This quiet
> >> change of behavior is unfortunate. I wonder if it would be acceptable
> >> to map the '-serial stdio' option in the presence of '-semihosting' to
> >> something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?
> >
> > I dunno.  I'm wary of having xtensa be unique here.  Alex, thoughts?
>
> Is semihosting *the* serial hardware for xtensa-sim or is it overriding
> another serial interface? I'm wary of adding more magical behaviour for
> -serial as it can be confusing enough already what actually gets routed
> to it if not doing everything explicitly.

There's no notion of 'serial hardware' for the xtensa-sim, all it has is
the three standard stdio file descriptors. But it was convenient thinking
of them as a serial port. I agree that no magic is needed here, but
the change shouldn't be quiet eiter, so xtensa-sim should warn (or
maybe even quit with an error code) when it sees the -serial option.

-- 
Thanks.
-- Max

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Alex Bennée]

Max Filippov <jcmvbkbc@gmail.com> writes:

> On Wed, Jun 29, 2022 at 1:09 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> > On 6/28/22 19:08, Max Filippov wrote:
>> >> On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
>> >> <richard.henderson@linaro.org> wrote:
>
>> >>>       }
>> >>> -    if (serial_hd(0)) {
>> >>> -        xtensa_sim_open_console(serial_hd(0));
>> >>> -    }
>> >> I've noticed that with this change '-serial stdio' and its variants
>> >> are still
>> >> accepted in the command line, but now they do nothing.
>> >
>> > Pardon?  They certainly will do something, via writes to the serial hardware.
>> >
>> >
>> >> This quiet
>> >> change of behavior is unfortunate. I wonder if it would be acceptable
>> >> to map the '-serial stdio' option in the presence of '-semihosting' to
>> >> something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?
>> >
>> > I dunno.  I'm wary of having xtensa be unique here.  Alex, thoughts?
>>
>> Is semihosting *the* serial hardware for xtensa-sim or is it overriding
>> another serial interface? I'm wary of adding more magical behaviour for
>> -serial as it can be confusing enough already what actually gets routed
>> to it if not doing everything explicitly.
>
> There's no notion of 'serial hardware' for the xtensa-sim, all it has is
> the three standard stdio file descriptors.

Which are accessed via semihosting calls? Are they implicitly mapped to
3 chardev devices for stdin, stdout and stderr?

> But it was convenient thinking
> of them as a serial port. I agree that no magic is needed here, but
> the change shouldn't be quiet eiter, so xtensa-sim should warn (or
> maybe even quit with an error code) when it sees the -serial option.

If the default chardevs already map to the 3 FDs then perhaps -serial
should be invalid because it is more explicit to use -chardev to
redirect the stream you want somewhere else. However I don't see them at
the moment:

  ➜  ./qemu-system-xtensa -M sim -semihosting -S -display none -monitor stdio
  QEMU 7.0.50 monitor - type 'help' for more information
  (qemu) info chardev 
  compat_monitor0: filename=stdio
  parallel0: filename=vc

-- 
Alex Bennée

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Max Filippov]

On Wed, Jun 29, 2022 at 3:14 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> Max Filippov <jcmvbkbc@gmail.com> writes:

> > There's no notion of 'serial hardware' for the xtensa-sim, all it has is
> > the three standard stdio file descriptors.
>
> Which are accessed via semihosting calls?

Yes.

> Are they implicitly mapped to
> 3 chardev devices for stdin, stdout and stderr?

In the absence of -serial option they are not mapped.
In the presence of that option they are mapped to the single chardev
that was passed as the parameter of that option.

> > But it was convenient thinking
> > of them as a serial port. I agree that no magic is needed here, but
> > the change shouldn't be quiet eiter, so xtensa-sim should warn (or
> > maybe even quit with an error code) when it sees the -serial option.
>
> If the default chardevs already map to the 3 FDs then perhaps -serial
> should be invalid because it is more explicit to use -chardev to
> redirect the stream you want somewhere else. However I don't see them at
> the moment:
>
>   ➜  ./qemu-system-xtensa -M sim -semihosting -S -display none -monitor stdio
>   QEMU 7.0.50 monitor - type 'help' for more information
>   (qemu) info chardev
>   compat_monitor0: filename=stdio
>   parallel0: filename=vc

Well, that mapping was done by me, manually (grep for sim_console in the
target/xtensa/xtensa-semi.c), so no wonder that parts like this don't work.

-- 
Thanks.
-- Max

Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Max Filippov]

On Tue, Jun 28, 2022 at 5:36 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 6/28/22 19:08, Max Filippov wrote:
> > On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
> > <richard.henderson@linaro.org> wrote:

...

> >> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> >> index 946c71cb5b..5cca6a170e 100644
> >> --- a/hw/xtensa/sim.c
> >> +++ b/hw/xtensa/sim.c
> >> @@ -87,9 +87,6 @@ XtensaCPU *xtensa_sim_common_init(MachineState *machine)
> >>           xtensa_create_memory_regions(&sysram, "xtensa.sysram",
> >>                                        get_system_memory());
> >>       }
> >> -    if (serial_hd(0)) {
> >> -        xtensa_sim_open_console(serial_hd(0));
> >> -    }
> >
> > I've noticed that with this change '-serial stdio' and its variants are still
> > accepted in the command line, but now they do nothing.
>
> Pardon?  They certainly will do something, via writes to the serial hardware.

What I meant was that with '-serial' option prior to this change it was
possible to redirect the standard streams of the sim machine, to stdio,
or socket or wherever, but after this change the option will be accepted,
but the machine will always have its first three file descriptors connected
to the QEMU's first three file descriptors.

I'd print a warning here, saying that the behavior has changed and
the '-semihosting-config chardev' must be used now.

> > This quiet
> > change of behavior is unfortunate. I wonder if it would be acceptable
> > to map the '-serial stdio' option in the presence of '-semihosting' to
> > something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?
>
> I dunno.  I'm wary of having xtensa be unique here.  Alex, thoughts?

Yeah, I thought about it some more and now it doesn't look like a good
idea to me either.

> >> +            switch (regs[4]) {
> >> +            case SELECT_ONE_READ:
> >> +                events = G_IO_IN;
> >> +                break;
> >> +            case SELECT_ONE_WRITE:
> >> +                events = G_IO_OUT;
> >> +                break;
> >> +            case SELECT_ONE_EXCEPT:
> >> +                events = G_IO_PRI;
> >> +                break;
> >> +            default:
> >> +                xtensa_cb(cs, -1, EINVAL);
> >
> > This doesn't match what there used to be: it was possible to call
> > select_one with rq other than SELECT_ONE_* and that would've
> > passed NULL for all fd sets in the select invocation turning it into
> > a sleep. It would return 0 after the timeout.
>
> Hmm.  Is there any documentation of what it was *supposed* to do?  Passing rq ==
> 0xdeadbeef and expecting a specific behaviour seems odd.

I haven't found any documentation for that simcall.
All I can say is that the logic in the code that used to be here is matching
exactly the logic in the code of the xtensa ISS from Cadence/Tensilica.

-- 
Thanks.
-- Max

Re: [PATCH v5 0/2] target/xtensa: semihosting cleanup

[Max Filippov]

On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Changes for v5:
>   * Rebase on master.
>
> r~
>
>
> Richard Henderson (2):
>   target/xtensa: Use an exception for semihosting
>   target/xtensa: Use semihosting/syscalls.h
>
>  target/xtensa/cpu.h         |   3 +-
>  target/xtensa/helper.h      |   3 -
>  hw/xtensa/sim.c             |   3 -
>  target/xtensa/exc_helper.c  |   4 +
>  target/xtensa/translate.c   |   3 +-
>  target/xtensa/xtensa-semi.c | 229 ++++++++----------------------------
>  6 files changed, 59 insertions(+), 186 deletions(-)

Tested-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max