Richard Henderson <[email protected]> writes:
> On 12/18/19 8:00 AM, Alex Bennée wrote: >> From: Keith Packard <[email protected]> >> >> 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 > > between. > >> +/** >> + * 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. As this >> + * call may block if no data is available we suspend the CPU and will >> + * rexecute the instruction when data is there. Therefor two > > re-execute, Therefore > >> + * conditions must be met: >> + * - CPUState is syncronised before callinging this function > > synchronized, calling > >> + * - pc is only updated once the character is succesfully returned > > successfully. > > >> +static int console_can_read(void *opaque) >> +{ >> + SemihostingConsole *c = opaque; >> + int ret; >> + g_assert(qemu_mutex_iothread_locked()); >> + ret = (int) fifo8_num_free(&c->fifo); >> + return ret; >> +} > > Boolean result; better as > > return fifo8_num_free(&c->fifo) > 0 > > (We could usefully change IOCanReadHandler to return bool to emphasize > this.) It's documented as the amount you can read and other handlers return amounts as well. I'm not sure I want to go messing with the chardev code in this series (although I need to look at Phillipe's series). > >> +static void console_wake_up(gpointer data, gpointer user_data) >> +{ >> + CPUState *cs = (CPUState *) data; >> + /* cpu_handle_halt won't know we have work so just unbung here */ >> + cs->halted = 0; >> + qemu_cpu_kick(cs); >> +} >> + >> +static void console_read(void *opaque, const uint8_t *buf, int size) >> +{ >> + SemihostingConsole *c = opaque; >> + g_assert(qemu_mutex_iothread_locked()); >> + while (size-- && !fifo8_is_full(&c->fifo)) { >> + fifo8_push(&c->fifo, *buf++); >> + } >> + g_slist_foreach(c->sleeping_cpus, console_wake_up, NULL); >> +} > > I think you should be clearing sleeping_cpus here, after they've all been > kicked. > >> +target_ulong qemu_semihosting_console_inc(CPUArchState *env) >> +{ >> + uint8_t ch; >> + SemihostingConsole *c = &console; >> + g_assert(qemu_mutex_iothread_locked()); >> + g_assert(current_cpu); >> + if (fifo8_is_empty(&c->fifo)) { >> + c->sleeping_cpus = g_slist_prepend(c->sleeping_cpus, current_cpu); >> + current_cpu->halted = 1; >> + current_cpu->exception_index = EXCP_HALTED; >> + cpu_loop_exit(current_cpu); >> + /* never returns */ >> + } >> + c->sleeping_cpus = g_slist_remove_all(c->sleeping_cpus, current_cpu); > > Which would mean you would not have to do this, because current_cpu is only on > the list when it is halted. > > I presume all semihosting holds the BQL before we reach here, and we are not > racing on this datastructure? Yeah this is all under BQL - which I assert is the case. I'll add a comment to the structure. > >> +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; >> + } > > Why are you polling stdin? linux-user isn't system mode, there isn't a > separate monitor thread to get blocked, and you aren't even blocking the > thread > to try again just returning -1 to the guest. Hmm not sure - I guess we should just bite the bullet and potentially block here. semihosting is linux-user is a bit of a weird use case because we are not providing "hardware" but it seems it is used by a bunch of testcases that want to test things like M-profile non-glibc binaries without the baggage of a full simulation. > > > r~ -- Alex Bennée
