Re: [RFC PATCH] semihosting: suspend recieving CPU when blocked (HACK, WIP)

["Alex Bennée" <alex.bennee@linaro.org>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 17/12/19 13:14, Alex Bennée wrote:
>> [AJB:
>> 
>> So this at least solves the hang of not being able to quit system
>> emulation while blocked. However there are two things we still need to
>> ensure:
>> 
>>  - the PC has not advanced until completion so we can redo the instruction
>>  - we actually wake up the CPU in console_read
>> 
>> In my testcase console_read never seems to get called. I've tried with
>> both an external pipe loopback and using the ringbuf:
>> 
>> qemu-system-aarch64 -M virt --display none -cpu cortex-a57 -kernel systest-a64-with-console.axf -semihosting-config
>>  enable=on,chardev=sh0 -serial mon:stdio -chardev ringbuf,logfile=foo,id=sh0
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  include/exec/cpu-all.h   |  1 +
>>  hw/semihosting/console.c | 34 +++++++++++++++++-----------------
>>  2 files changed, 18 insertions(+), 17 deletions(-)
>> 
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index e96781a4559..093d7a76edd 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -31,6 +31,7 @@
>>  #define EXCP_HALTED     0x10003 /* cpu is halted (waiting for external event) */
>>  #define EXCP_YIELD      0x10004 /* cpu wants to yield timeslice to another */
>>  #define EXCP_ATOMIC     0x10005 /* stop-the-world and emulate atomic */
>> +#define EXCP_BLOCKED    0x10006 /* cpu is blocked (semihosting) */
>>  
>>  /* some important defines:
>>   *
>> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
>> index 4db68d62270..bda457a0608 100644
>> --- a/hw/semihosting/console.c
>> +++ b/hw/semihosting/console.c
>> @@ -20,6 +20,7 @@
>>  #include "hw/semihosting/semihost.h"
>>  #include "hw/semihosting/console.h"
>>  #include "exec/gdbstub.h"
>> +#include "exec/exec-all.h"
>>  #include "qemu/log.h"
>>  #include "chardev/char.h"
>>  #include <pthread.h>
>> @@ -109,50 +110,49 @@ void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
>>  
>>  typedef struct SemihostingConsole {
>>      CharBackend         backend;
>> -    pthread_mutex_t     mutex;
>> -    pthread_cond_t      cond;
>> +    CPUState            *sleeping_cpu;
>>      bool                got;
>>      Fifo8               fifo;
>>  } SemihostingConsole;
>>  
>> -static SemihostingConsole console = {
>> -    .mutex = PTHREAD_MUTEX_INITIALIZER,
>> -    .cond = PTHREAD_COND_INITIALIZER
>> -};
>> +static SemihostingConsole console;
>>  
>>  static int console_can_read(void *opaque)
>>  {
>>      SemihostingConsole *c = opaque;
>>      int ret;
>> -    pthread_mutex_lock(&c->mutex);
>> +    g_assert(qemu_mutex_iothread_locked());
>>      ret = (int) fifo8_num_free(&c->fifo);
>> -    pthread_mutex_unlock(&c->mutex);
>>      return ret;
>>  }
>>  
>>  static void console_read(void *opaque, const uint8_t *buf, int size)
>>  {
>>      SemihostingConsole *c = opaque;
>> -    pthread_mutex_lock(&c->mutex);
>> +    g_assert(qemu_mutex_iothread_locked());
>>      while (size-- && !fifo8_is_full(&c->fifo)) {
>>          fifo8_push(&c->fifo, *buf++);
>>      }
>> -    pthread_cond_broadcast(&c->cond);
>> -    pthread_mutex_unlock(&c->mutex);
>> +    if (c->sleeping_cpu) {
>> +        cpu_resume(c->sleeping_cpu);
>> +    }
>>  }
>>  
>>  target_ulong qemu_semihosting_console_inc(CPUArchState *env)
>>  {
>>      uint8_t ch;
>>      SemihostingConsole *c = &console;
>> -    qemu_mutex_unlock_iothread();
>> -    pthread_mutex_lock(&c->mutex);
>> -    while (fifo8_is_empty(&c->fifo)) {
>> -        pthread_cond_wait(&c->cond, &c->mutex);
>> +    g_assert(qemu_mutex_iothread_locked());
>> +    g_assert(current_cpu);
>> +    if (fifo8_is_empty(&c->fifo)) {
>> +        c->sleeping_cpu = current_cpu;
>> +        c->sleeping_cpu->stop = true;
>> +        c->sleeping_cpu->exception_index = EXCP_BLOCKED;
>
> Why do you need to set exception_index to something other than -1 (using
> cpu_loop_exit_noexc for example)?

If there is no exception to process we won't exit the main loop which we
need to do if we want to wait until there is data to read.

> Using ->stop here is a bit weird, since ->stop is usually related to
> pause_all_vcpus.

Arguably we could come up with a better API to cpu.c but this allows us
to use cpu_resume(c->sleeping_cpu) when waking up rather than hand
rolling our own wake-up mechanism.

>
> Paolo


-- 
Alex Bennée