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

[Max Filippov]

On Tue, Jun 7, 2022 at 10:36 PM 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 | 323 +++++++++++-------------------------
>  3 files changed, 97 insertions(+), 230 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));
> -    }

Do I understand correctly that the sim machine will no longer
support the -serial option with this change?

>      return cpu;
>  }
>
> diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
> index 5375f106fc..7ef4be353e 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"

This does not build on top of the current master, is there a branch where
it's buildable?

...

> -    switch (host_errno) {
> -    case 0:         return 0;
> -    case EPERM:     return TARGET_EPERM;
> -    case ENOENT:    return TARGET_ENOENT;
> -    case ESRCH:     return TARGET_ESRCH;
> -    case EINTR:     return TARGET_EINTR;
> -    case EIO:       return TARGET_EIO;
> -    case ENXIO:     return TARGET_ENXIO;
> -    case E2BIG:     return TARGET_E2BIG;
> -    case ENOEXEC:   return TARGET_ENOEXEC;
> -    case EBADF:     return TARGET_EBADF;
> -    case ECHILD:    return TARGET_ECHILD;
> -    case EAGAIN:    return TARGET_EAGAIN;
> -    case ENOMEM:    return TARGET_ENOMEM;
> -    case EACCES:    return TARGET_EACCES;
> -    case EFAULT:    return TARGET_EFAULT;
> -#ifdef ENOTBLK
> -    case ENOTBLK:   return TARGET_ENOTBLK;
> -#endif

AFAIR there were reports that qemu doesn't build on some
systems because they were missing ENOTBLK and other
error codes that were made conditional here.

...

> +#define E(N) case E##N: err = TARGET_E##N; break
...
> +    E(PERM);
> +    E(NOENT);
> +    E(SRCH);
> +    E(INTR);
> +    E(IO);
> +    E(NXIO);
> +    E(2BIG);
> +    E(NOEXEC);
> +    E(BADF);
> +    E(CHILD);
> +    E(AGAIN);
> +    E(NOMEM);
> +    E(ACCES);
> +    E(FAULT);
> +    E(NOTBLK);
> +    E(BUSY);
> +    E(EXIST);
> +    E(XDEV);
> +    E(NODEV);
> +    E(NOTDIR);
> +    E(ISDIR);
> +    E(INVAL);
> +    E(NFILE);
> +    E(MFILE);
> +    E(NOTTY);
> +    E(TXTBSY);
> +    E(FBIG);
> +    E(NOSPC);
> +    E(SPIPE);
> +    E(ROFS);
> +    E(MLINK);
> +    E(PIPE);
> +    E(DOM);
> +    E(RANGE);
> +    E(NOSYS);
> +    E(LOOP);

I'm not sure mangling error code names is a good idea.

-- 
Thanks.
-- Max