[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS
From: |
Richard Henderson |
Subject: |
Re: [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC |
Date: |
Wed, 18 Dec 2019 10:16:32 -1000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 12/18/19 8:00 AM, Alex Bennée wrote:
> From: Keith Packard <address@hidden>
>
> 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.)
> +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?
> +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.
r~
- [PATCH v1 0/4] semihosting read console support, Alex Bennée, 2019/12/18
- [PATCH v1 1/4] target/arm: remove unused EXCP_SEMIHOST leg, Alex Bennée, 2019/12/18
- [PATCH v1 2/4] target/arm: only update pc after semihosting completes, Alex Bennée, 2019/12/18
- [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC, Alex Bennée, 2019/12/18
- Re: [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC,
Richard Henderson <=
- [PATCH v1 4/4] tests/tcg: add a dumb-as-bricks semihosting console test, Alex Bennée, 2019/12/18
- Re: [PATCH v1 0/4] semihosting read console support, Keith Packard, 2019/12/18