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

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> 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.)

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