| Message ID | 20220628114307.697943-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/xtensa: semihosting cleanup | expand |
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.
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~
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~
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.
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.
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
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.
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;
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(-)