| Message ID | 1436544486-31169-3-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
fred.konrad@greensocs.com writes: > From: KONRAD Frederic <fred.konrad@greensocs.com> > > This flag indicates if the VCPU is currently executing TCG code. > > Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> > > Changes V1 -> V2: > * do both tcg_executing = 0 or 1 in cpu_exec(). > --- > cpu-exec.c | 2 ++ > include/qom/cpu.h | 3 +++ > qom/cpu.c | 1 + > 3 files changed, 6 insertions(+) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 75694f3..2fdf89d 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -371,6 +371,7 @@ int cpu_exec(CPUState *cpu) > cpu->halted = 0; > } > > + cpu->tcg_executing = 1; > current_cpu = cpu; > > /* As long as current_cpu is null, up to the assignment just above, > @@ -583,5 +584,6 @@ int cpu_exec(CPUState *cpu) > > /* fail safe : never use current_cpu outside cpu_exec() */ > current_cpu = NULL; > + cpu->tcg_executing = 0; > return ret; > } > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index efa9624..a2de536 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -226,6 +226,7 @@ struct kvm_run; > * @stopped: Indicates the CPU has been artificially stopped. > * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this > * CPU and return to its top level loop. > + * @tcg_executing: This TCG thread is in cpu_exec(). > * @singlestep_enabled: Flags for single-stepping. > * @icount_extra: Instructions until next timer event. > * @icount_decr: Number of cycles left, with interrupt flag in high bit. > @@ -322,6 +323,8 @@ struct CPUState { > (absolute value) offset as small as possible. This reduces code > size, especially for hosts without large memory offsets. */ > volatile sig_atomic_t tcg_exit_req; > + > + volatile int tcg_executing; My concern is on weakly ordered backends is volatile enough for this flag or do we need some sort of memory barrier when we update it? Does it just introduce an inefficiency that other threads may spin a few times waiting to find out the vCPU has halted? If other threads are waiting for it to halt is there a mechanism that ensures we'll never start-up again until everything is done? > }; > > QTAILQ_HEAD(CPUTailQ, CPUState); > diff --git a/qom/cpu.c b/qom/cpu.c > index 4e12598..62663e5 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -249,6 +249,7 @@ static void cpu_common_reset(CPUState *cpu) > cpu->icount_decr.u32 = 0; > cpu->can_do_io = 0; > cpu->exception_index = -1; > + cpu->tcg_executing = 0; > memset(cpu->tb_jmp_cache, 0, TB_JMP_CACHE_SIZE * sizeof(void *)); > }
On 13 July 2015 at 16:56, Alex Bennée <alex.bennee@linaro.org> wrote: >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> @@ -322,6 +323,8 @@ struct CPUState { >> (absolute value) offset as small as possible. This reduces code >> size, especially for hosts without large memory offsets. */ >> volatile sig_atomic_t tcg_exit_req; >> + >> + volatile int tcg_executing; > > My concern is on weakly ordered backends is volatile enough for this > flag or do we need some sort of memory barrier when we update it? Does > it just introduce an inefficiency that other threads may spin a few > times waiting to find out the vCPU has halted? Plus "volatile int" is a bit of an "is this really right?" red flag... Currently in QEMU we use 'volatile' for: (1) "volatile sig_atomic_t", which is dealing with variables accessed from signal handlers (2) marking local variables which mustn't be trashed by longjmp() (3) some things in tests/ code which I'm ignoring because they're only test code (4) a few other things which are suspicious at best: hw/intc/apic_common.c: volatile int a_i_d = apic_irq_delivered; hw/xen/xen_pt_msi.c: const volatile uint32_t *vec_ctrl; trace/simple.c:static volatile gint trace_idx; trace/simple.c:static volatile gint dropped_events; and I would be very dubious about adding more direct uses of volatile. You almost certainly want something from atomic.h instead. thanks -- PMM
On 13/07/2015 18:10, Peter Maydell wrote: > (4) a few other things which are suspicious at best: > hw/intc/apic_common.c: volatile int a_i_d = apic_irq_delivered; This one has a comment above: /* Copy this into a local variable to encourage gcc to emit a plain * register for a sys/sdt.h marker. For details on this workaround, see: * https://sourceware.org/bugzilla/show_bug.cgi?id=13296 */ > hw/xen/xen_pt_msi.c: const volatile uint32_t *vec_ctrl; Seems to be MMIO (yes, really), so okay. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 13/07/2015 18:10, Peter Maydell wrote: >> (4) a few other things which are suspicious at best: >> hw/intc/apic_common.c: volatile int a_i_d = apic_irq_delivered; > > This one has a comment above: > > /* Copy this into a local variable to encourage gcc to emit a plain > * register for a sys/sdt.h marker. For details on this workaround, see: > * https://sourceware.org/bugzilla/show_bug.cgi?id=13296 > */ > >> hw/xen/xen_pt_msi.c: const volatile uint32_t *vec_ctrl; > > Seems to be MMIO (yes, really), so okay. For some reason I don't find the use of the word "encourage" w.r.t compiler behaviour particularly encouraging ;-)
On 13/07/2015 17:56, Alex Bennée wrote: > fred.konrad@greensocs.com writes: > >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> >> This flag indicates if the VCPU is currently executing TCG code. >> >> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >> >> Changes V1 -> V2: >> * do both tcg_executing = 0 or 1 in cpu_exec(). >> --- >> cpu-exec.c | 2 ++ >> include/qom/cpu.h | 3 +++ >> qom/cpu.c | 1 + >> 3 files changed, 6 insertions(+) >> >> diff --git a/cpu-exec.c b/cpu-exec.c >> index 75694f3..2fdf89d 100644 >> --- a/cpu-exec.c >> +++ b/cpu-exec.c >> @@ -371,6 +371,7 @@ int cpu_exec(CPUState *cpu) >> cpu->halted = 0; >> } >> >> + cpu->tcg_executing = 1; >> current_cpu = cpu; >> >> /* As long as current_cpu is null, up to the assignment just above, >> @@ -583,5 +584,6 @@ int cpu_exec(CPUState *cpu) >> >> /* fail safe : never use current_cpu outside cpu_exec() */ >> current_cpu = NULL; >> + cpu->tcg_executing = 0; >> return ret; >> } >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index efa9624..a2de536 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -226,6 +226,7 @@ struct kvm_run; >> * @stopped: Indicates the CPU has been artificially stopped. >> * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this >> * CPU and return to its top level loop. >> + * @tcg_executing: This TCG thread is in cpu_exec(). >> * @singlestep_enabled: Flags for single-stepping. >> * @icount_extra: Instructions until next timer event. >> * @icount_decr: Number of cycles left, with interrupt flag in high bit. >> @@ -322,6 +323,8 @@ struct CPUState { >> (absolute value) offset as small as possible. This reduces code >> size, especially for hosts without large memory offsets. */ >> volatile sig_atomic_t tcg_exit_req; >> + >> + volatile int tcg_executing; > My concern is on weakly ordered backends is volatile enough for this > flag or do we need some sort of memory barrier when we update it? Does > it just introduce an inefficiency that other threads may spin a few > times waiting to find out the vCPU has halted? I think it will just spin (see in flush_queued_safe_work in patch 3). > > If other threads are waiting for it to halt is there a mechanism that > ensures we'll never start-up again until everything is done? This flag is not supposed to do that, it's in the third patch as well. It will check async_safe_work_pending before starting the execution. We might have a race here, if the flush is triggered between async_safe_work_pending and the tcg_executing flag set in cpu-exec. if (async_safe_work_pending()) { cpu->exit_request = 1; return 0; } if (cpu->halted) { if (!cpu_has_work(cpu)) { return EXCP_HALTED; } cpu->halted = 0; } cpu->tcg_executing = 1; I need to check and fix that. Fred > > >> }; >> >> QTAILQ_HEAD(CPUTailQ, CPUState); >> diff --git a/qom/cpu.c b/qom/cpu.c >> index 4e12598..62663e5 100644 >> --- a/qom/cpu.c >> +++ b/qom/cpu.c >> @@ -249,6 +249,7 @@ static void cpu_common_reset(CPUState *cpu) >> cpu->icount_decr.u32 = 0; >> cpu->can_do_io = 0; >> cpu->exception_index = -1; >> + cpu->tcg_executing = 0; >> memset(cpu->tb_jmp_cache, 0, TB_JMP_CACHE_SIZE * sizeof(void *)); >> }
diff --git a/cpu-exec.c b/cpu-exec.c index 75694f3..2fdf89d 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -371,6 +371,7 @@ int cpu_exec(CPUState *cpu) cpu->halted = 0; } + cpu->tcg_executing = 1; current_cpu = cpu; /* As long as current_cpu is null, up to the assignment just above, @@ -583,5 +584,6 @@ int cpu_exec(CPUState *cpu) /* fail safe : never use current_cpu outside cpu_exec() */ current_cpu = NULL; + cpu->tcg_executing = 0; return ret; } diff --git a/include/qom/cpu.h b/include/qom/cpu.h index efa9624..a2de536 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -226,6 +226,7 @@ struct kvm_run; * @stopped: Indicates the CPU has been artificially stopped. * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this * CPU and return to its top level loop. + * @tcg_executing: This TCG thread is in cpu_exec(). * @singlestep_enabled: Flags for single-stepping. * @icount_extra: Instructions until next timer event. * @icount_decr: Number of cycles left, with interrupt flag in high bit. @@ -322,6 +323,8 @@ struct CPUState { (absolute value) offset as small as possible. This reduces code size, especially for hosts without large memory offsets. */ volatile sig_atomic_t tcg_exit_req; + + volatile int tcg_executing; }; QTAILQ_HEAD(CPUTailQ, CPUState); diff --git a/qom/cpu.c b/qom/cpu.c index 4e12598..62663e5 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -249,6 +249,7 @@ static void cpu_common_reset(CPUState *cpu) cpu->icount_decr.u32 = 0; cpu->can_do_io = 0; cpu->exception_index = -1; + cpu->tcg_executing = 0; memset(cpu->tb_jmp_cache, 0, TB_JMP_CACHE_SIZE * sizeof(void *)); }