Re: [PATCH v1 3/4] semihosting: add qemu_semihosting_console_inc for SYS_READC

[Richard Henderson <richard.henderson@linaro.org>]

On 12/18/19 8:00 AM, Alex Bennée wrote:
> From: Keith Packard <keithp@keithp.com>
> 
> 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~