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

[Max Filippov <jcmvbkbc@gmail.com>]

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