| Message ID | 20191217121443.14757-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] semihosting: suspend recieving CPU when blocked (HACK, WIP) | expand |
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)? Using ->stop here is a bit weird, since ->stop is usually related to pause_all_vcpus. Paolo
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
On 17/12/19 14:42, Alex Bennée wrote: >> 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. Okay. >> 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. But we already have the right wake-up mechanism, which is cpu->halted/cpu_has_work. That also makes it possible to just use EXCP_HALTED instead of adding a new EXCP_BLOCKED. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 17/12/19 14:42, Alex Bennée wrote: >>> 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. > > Okay. > >>> 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. > > But we already have the right wake-up mechanism, which is > cpu->halted/cpu_has_work. cpu_has_work is a guest function though and semihosting_console is a common hw module. It can't peek into the guests internal state. This all comes back to cpu_thread_is_idle anyway in making our decision about if we do or do not sleep on the halt_cond. > That also makes it possible to just use > EXCP_HALTED instead of adding a new EXCP_BLOCKED. We can certainly use EXCP_HALTED but maybe come up with a common way of entering the state? There seems to be a combination of messing around with special interrupts and direct poking of cs->halted = 1 while setting the exception. Maybe this could finally clear up the #if defined(TARGET_I386) hacking in cpus.c? > > Paolo -- Alex Bennée
On 17/12/19 15:18, Alex Bennée wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 17/12/19 14:42, Alex Bennée wrote: >>>> 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. >> >> Okay. >> >>>> 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. >> >> But we already have the right wake-up mechanism, which is >> cpu->halted/cpu_has_work. > > cpu_has_work is a guest function though and semihosting_console is a > common hw module. It can't peek into the guests internal state. semihosting_console only needs to something like cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). (By the way, the stopped_cpu should probably be a list to mimic the condition variable---for example a GList). > This all > comes back to cpu_thread_is_idle anyway in making our decision about if > we do or do not sleep on the halt_cond. > >> That also makes it possible to just use >> EXCP_HALTED instead of adding a new EXCP_BLOCKED. > > We can certainly use EXCP_HALTED but maybe come up with a common way of > entering the state? There seems to be a combination of messing around > with special interrupts and direct poking of cs->halted = 1 while > setting the exception. Maybe this could finally clear up the #if > defined(TARGET_I386) hacking in cpus.c? If you're talking accel/tcg/cpu-exec.c, that's different; the issue there is that x86 has a kind of warm reset pin that is not equivalent to cpu_reset. Removing that would only entail adding a new member function to CPUClass. Paolo
On 17/12/19 15:18, Alex Bennée wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 17/12/19 14:42, Alex Bennée wrote: >>>> 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. >> >> Okay. >> >>>> 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. >> >> But we already have the right wake-up mechanism, which is >> cpu->halted/cpu_has_work. > > cpu_has_work is a guest function though and semihosting_console is a > common hw module. It can't peek into the guests internal state. semihosting_console only needs to something like cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). (By the way, the stopped_cpu should probably be a list to mimic the condition variable---for example a GSList). > This all > comes back to cpu_thread_is_idle anyway in making our decision about if > we do or do not sleep on the halt_cond. > >> That also makes it possible to just use >> EXCP_HALTED instead of adding a new EXCP_BLOCKED. > > We can certainly use EXCP_HALTED but maybe come up with a common way of > entering the state? There seems to be a combination of messing around > with special interrupts and direct poking of cs->halted = 1 while > setting the exception. Maybe this could finally clear up the #if > defined(TARGET_I386) hacking in cpus.c? If you're talking accel/tcg/cpu-exec.c, that's different; the issue there is that x86 has a kind of warm reset pin that is not equivalent to cpu_reset. Removing that would only entail adding a new member function to CPUClass. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 17/12/19 15:18, Alex Bennée wrote: >> >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 17/12/19 14:42, Alex Bennée wrote: >>>>> 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. >>> >>> Okay. >>> >>>>> 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. >>> >>> But we already have the right wake-up mechanism, which is >>> cpu->halted/cpu_has_work. >> >> cpu_has_work is a guest function though and semihosting_console is a >> common hw module. It can't peek into the guests internal state. > > semihosting_console only needs to something like > cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). As an exception is being delivered we just end up re-executing the EXCP_SEMIHOST. I still don't see why using cpu_interrupt is an improvement seeing as it is secondary to exception processing. > (By the way, > the stopped_cpu should probably be a list to mimic the condition > variable---for example a GSList). ok > >> This all >> comes back to cpu_thread_is_idle anyway in making our decision about if >> we do or do not sleep on the halt_cond. >> >>> That also makes it possible to just use >>> EXCP_HALTED instead of adding a new EXCP_BLOCKED. >> >> We can certainly use EXCP_HALTED but maybe come up with a common way of >> entering the state? There seems to be a combination of messing around >> with special interrupts and direct poking of cs->halted = 1 while >> setting the exception. Maybe this could finally clear up the #if >> defined(TARGET_I386) hacking in cpus.c? > > If you're talking accel/tcg/cpu-exec.c, that's different; the issue > there is that x86 has a kind of warm reset pin that is not equivalent to > cpu_reset. Removing that would only entail adding a new member function > to CPUClass. > > Paolo -- Alex Bennée
Il mer 18 dic 2019, 18:36 Alex Bennée <alex.bennee@linaro.org> ha scritto: > > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 17/12/19 15:18, Alex Bennée wrote: > >> cpu_has_work is a guest function though and semihosting_console is a > >> common hw module. It can't peek into the guests internal state. > > > > semihosting_console only needs to something like > > cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST). > > As an exception is being delivered we just end up re-executing the > EXCP_SEMIHOST. I still don't see why using cpu_interrupt is an > improvement seeing as it is secondary to exception processing. > FWIW I skimmed your patch and yes an interrupt is not needed since you are delaying the update of the program counter; that's nicer. Paolo <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il mer 18 dic 2019, 18:36 Alex Bennée <<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br> Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank" rel="noreferrer">pbonzini@redhat.com</a>> writes:<br> <br> > On 17/12/19 15:18, Alex Bennée wrote:<br>>> cpu_has_work is a guest function though and semihosting_console is a<br> >> common hw module. It can't peek into the guests internal state.<br> ><br> > semihosting_console only needs to something like<br> > cpu_interrupt(cpu->stopped_cpu, CPU_INTERRUPT_SEMIHOST).<br> <br> As an exception is being delivered we just end up re-executing the<br> EXCP_SEMIHOST. I still don't see why using cpu_interrupt is an<br> improvement seeing as it is secondary to exception processing.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">FWIW I skimmed your patch and yes an interrupt is not needed since you are delaying the update of the program counter; that's nicer.</div><div dir="auto"><br></div><div dir="auto">Paolo</div></div>
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; + cpu_loop_exit(c->sleeping_cpu); + /* never returns */ } + c->sleeping_cpu = NULL; ch = fifo8_pop(&c->fifo); - pthread_mutex_unlock(&c->mutex); - qemu_mutex_lock_iothread(); return (target_ulong) ch; }
Sleeping in the semihosting code is problematic as we deadlock the whole system. This includes issuing a "quit" via the HMP or presumably if gdbserver got involved. What we really want is to return to the main loop and gt woken up when there is data to process. We can then re-execute the instruction which will succeed this time. [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(-) -- 2.20.1