Re: [PATCH] Semihost SYS_READC implementation

[Paolo Bonzini]

On 22/10/19 20:12, Keith Packard wrote:
> $ qemu-system-arm -chardev stdio,mux=on,id=stdio0 -serial chardev:stdio0 
> -semihosting-config enable=on,chardev=stdio0 -mon 
> chardev=stdio0,mode=readline 
> 
> It might be nice if this could be shortened, but it certainly provides
> the necessary options to make it all work.

Yeah, it's not easy to cram all of the possibilities in the command
line, so in general you need two options, one for the backend ("stdio")
and one for the front-end ("semihosting").  In some cases there are
shortcuts that refer the front-end name in the option name ("-serial"),
but semihosting isn't one of those.

There is no particular reason for this, except for the fact that the
older option "-semihosting" was added without an argument many many
years ago.

> I'll post an updated version of the patch in a while, after waiting to
> see if there are any additional comments.

Indeed I have a couple comments, mostly on coding style and duplication:

> 
> +typedef struct SemihostingFifo {
> +    unsigned int     insert, remove;
> +
> +    uint8_t fifo[FIFO_SIZE];
> +} SemihostingFifo;
> +

Please take a look at include/qemu/fifo8.h instead of rolling your own
ring buffer.  Note that it is not thread-safe so you'll have to keep
that part.

> 
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> +    (void) env;

No need for this, and also...

> +    SemihostingConsole *c = &console;
> +    qemu_mutex_unlock_iothread();
> +    pthread_mutex_lock(&c->mutex);
> +    while (fifo_empty(&c->fifo)) {
> +        pthread_cond_wait(&c->cond, &c->mutex);
> +    }
> +    uint8_t ch;

... we tend not to mix declarations and statements.

> +    fifo_remove(&c->fifo, ch);
> +    pthread_mutex_unlock(&c->mutex);
> +    qemu_mutex_lock_iothread();
> +    return (target_ulong) ch;
> +}
> +

Kudos for the unlock/lock_iothread; I am not really familiar with the
semihosting code and I would have naively assumed that it runs without
that lock taken.  It is indeed better to have your own mutex, because we
do want to pull the unlock/lock up into do_arm_semihosting or even its
callers.

Thanks,

Paolo