| Message ID | 20141126103925.7772.13043.stgit@PASHA-ISP |
|---|---|
| State | New |
| Headers | show |
On 26/11/2014 11:39, Pavel Dovgalyuk wrote: > +int64_t cpu_get_instructions_counter(void) > +{ > + /* This function calls are synchnonized to timer changes, > + calling cpu_get_instructions_counter_locked without lock is safe */ > + int64_t icount = timers_state.qemu_icount; > + CPUState *cpu = current_cpu; > + > + if (cpu) { > + icount -= (cpu->icount_decr.u16.low + cpu->icount_extra); > + } > + return icount; Why do you need to do this if !cpu_can_do_io(cpu)? Perhaps a better name for the functions is - cpu_get_instructions_counter_locked -> cpu_get_icount_raw - cpu_get_instructions_counter -> cpu_get_icount_raw_nocheck This makes it clear that cpu_get_instructions_counter should raise questions to a reviewer. Paolo
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > On 26/11/2014 11:39, Pavel Dovgalyuk wrote: > > +int64_t cpu_get_instructions_counter(void) > > +{ > > + /* This function calls are synchnonized to timer changes, > > + calling cpu_get_instructions_counter_locked without lock is safe */ > > + int64_t icount = timers_state.qemu_icount; > > + CPUState *cpu = current_cpu; > > + > > + if (cpu) { > > + icount -= (cpu->icount_decr.u16.low + cpu->icount_extra); > > + } > > + return icount; > > Why do you need to do this if !cpu_can_do_io(cpu)? We save number of executed instruction when saving interrupt or exception event. It leads to the call of cpu_get_instructions_counter() from cpu_exec function (through several replay functions). It is correct (because no block is executing at that moment) but is different to prior usage of icount requests. > Perhaps a better name for the functions is > > - cpu_get_instructions_counter_locked -> cpu_get_icount_raw > > - cpu_get_instructions_counter -> cpu_get_icount_raw_nocheck Ok, I'll rename these functions. Pavel Dovgalyuk
On 04/12/2014 12:02, Pavel Dovgaluk wrote: >> > Why do you need to do this if !cpu_can_do_io(cpu)? > We save number of executed instruction when saving interrupt or exception event. > It leads to the call of cpu_get_instructions_counter() from cpu_exec function > (through several replay functions). It is correct (because no block is executing > at that moment) but is different to prior usage of icount requests. Why is !cpu_can_do_io(cpu) if no block is executing? I'm not saying the function is wrong, just that it warrants a more thorough review. Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 04/12/2014 12:02, Pavel Dovgaluk wrote: > >> > Why do you need to do this if !cpu_can_do_io(cpu)? > > We save number of executed instruction when saving interrupt or exception event. > > It leads to the call of cpu_get_instructions_counter() from cpu_exec function > > (through several replay functions). It is correct (because no block is executing > > at that moment) but is different to prior usage of icount requests. > > Why is !cpu_can_do_io(cpu) if no block is executing? Because it returns cpu->can_do_io which is equal to zero at that moment. Pavel Dovgalyuk
On 05/12/2014 06:34, Pavel Dovgaluk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> On 04/12/2014 12:02, Pavel Dovgaluk wrote: >>>>> Why do you need to do this if !cpu_can_do_io(cpu)? >>> We save number of executed instruction when saving interrupt or exception event. >>> It leads to the call of cpu_get_instructions_counter() from cpu_exec function >>> (through several replay functions). It is correct (because no block is executing >>> at that moment) but is different to prior usage of icount requests. >> >> Why is !cpu_can_do_io(cpu) if no block is executing? > > Because it returns cpu->can_do_io which is equal to zero at that moment. And why is can_do_io zero? :) Is the fix to move the place where can_do_io becomes nonzero? Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 05/12/2014 06:34, Pavel Dovgaluk wrote: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> On 04/12/2014 12:02, Pavel Dovgaluk wrote: > >>>>> Why do you need to do this if !cpu_can_do_io(cpu)? > >>> We save number of executed instruction when saving interrupt or exception event. > >>> It leads to the call of cpu_get_instructions_counter() from cpu_exec function > >>> (through several replay functions). It is correct (because no block is executing > >>> at that moment) but is different to prior usage of icount requests. > >> > >> Why is !cpu_can_do_io(cpu) if no block is executing? > > > > Because it returns cpu->can_do_io which is equal to zero at that moment. > > And why is can_do_io zero? :) Is the fix to move the place where > can_do_io becomes nonzero? can_do_io is set by gen_io_start function. As I understand, it is used to protect determinism in icount mode, because it allows non-deterministic (port io, raising interrupt) operations only at the end of the translation blocks. When someone tries to use MMIO in the middle of TB, that TB is recompiled to place this instruction at the end of the block. Do you mean that we can set can_do_io before execution of the block and reset it at the beginning of the execution? Pavel Dovgalyuk
On 05/12/2014 11:55, Pavel Dovgaluk wrote: >> > >> > And why is can_do_io zero? :) Is the fix to move the place where >> > can_do_io becomes nonzero? > can_do_io is set by gen_io_start function. > As I understand, it is used to protect determinism in icount mode, > because it allows non-deterministic (port io, raising interrupt) > operations only at the end of the translation blocks. > When someone tries to use MMIO in the middle of TB, that TB is > recompiled to place this instruction at the end of the block. > > Do you mean that we can set can_do_io before execution of the block > and reset it at the beginning of the execution? Yes, we could try setting it after execution of the block and clearing it afterwards. Peter knows that part of icount better though (I know mostly the timer/warping parts). Paolo
On 05/12/2014 13:59, Pavel Dovgaluk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> On 05/12/2014 11:55, Pavel Dovgaluk wrote: >>>>> >>>>> And why is can_do_io zero? :) Is the fix to move the place where >>>>> can_do_io becomes nonzero? >>> can_do_io is set by gen_io_start function. >>> As I understand, it is used to protect determinism in icount mode, >>> because it allows non-deterministic (port io, raising interrupt) >>> operations only at the end of the translation blocks. >>> When someone tries to use MMIO in the middle of TB, that TB is >>> recompiled to place this instruction at the end of the block. >>> >>> Do you mean that we can set can_do_io before execution of the block >>> and reset it at the beginning of the execution? >> >> Yes, we could try setting it after execution of the block and clearing >> it afterwards. Peter knows that part of icount better though (I know >> mostly the timer/warping parts). > > Ok, how about these changes? > > diff --git a/cpu-exec.c b/cpu-exec.c > index f52f292..88675ca 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -168,7 +168,9 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t *tb_ptr) > } > #endif /* DEBUG_DISAS */ > > + cpu->can_do_io = 0; > next_tb = tcg_qemu_tb_exec(env, tb_ptr); > + cpu->can_do_io = 1; > trace_exec_tb_exit((void *) (next_tb & ~TB_EXIT_MASK), > next_tb & TB_EXIT_MASK); > > @@ -548,6 +550,7 @@ int cpu_exec(CPUArchState *env) > cpu = current_cpu; > env = cpu->env_ptr; > cc = CPU_GET_CLASS(cpu); > + cpu->can_do_io = 1; > #ifdef TARGET_I386 > x86_cpu = X86_CPU(cpu); > #endif > diff --git a/cpus.c b/cpus.c > index 0c33458..7a45a51 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -934,6 +934,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg) > qemu_mutex_lock(&qemu_global_mutex); > qemu_thread_get_self(cpu->thread); > cpu->thread_id = qemu_get_thread_id(); > + cpu->can_do_io = 1; > current_cpu = cpu; > > r = kvm_init_vcpu(cpu); > @@ -974,6 +975,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg) > qemu_mutex_lock_iothread(); > qemu_thread_get_self(cpu->thread); > cpu->thread_id = qemu_get_thread_id(); > + cpu->can_do_io = 1; > > sigemptyset(&waitset); > sigaddset(&waitset, SIG_IPI); > @@ -1016,6 +1018,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > CPU_FOREACH(cpu) { > cpu->thread_id = qemu_get_thread_id(); > cpu->created = true; > + cpu->can_do_io = 1; > } > qemu_cond_signal(&qemu_cpu_cond); Yes, this would work too, thanks for trying it! Paolo
diff --git a/cpus.c b/cpus.c index a7b6c53..492e19a 100644 --- a/cpus.c +++ b/cpus.c @@ -136,8 +136,7 @@ typedef struct TimersState { static TimersState timers_state; -/* Return the virtual CPU time, based on the instruction counter. */ -static int64_t cpu_get_icount_locked(void) +static int64_t cpu_get_instructions_counter_locked(void) { int64_t icount; CPUState *cpu = current_cpu; @@ -145,10 +144,31 @@ static int64_t cpu_get_icount_locked(void) icount = timers_state.qemu_icount; if (cpu) { if (!cpu_can_do_io(cpu)) { - fprintf(stderr, "Bad clock read\n"); + fprintf(stderr, "Bad icount read\n"); + exit(1); } icount -= (cpu->icount_decr.u16.low + cpu->icount_extra); } + return icount; +} + +int64_t cpu_get_instructions_counter(void) +{ + /* This function calls are synchnonized to timer changes, + calling cpu_get_instructions_counter_locked without lock is safe */ + int64_t icount = timers_state.qemu_icount; + CPUState *cpu = current_cpu; + + if (cpu) { + icount -= (cpu->icount_decr.u16.low + cpu->icount_extra); + } + return icount; +} + +/* Return the virtual CPU time, based on the instruction counter. */ +static int64_t cpu_get_icount_locked(void) +{ + int64_t icount = cpu_get_instructions_counter_locked(); return timers_state.qemu_icount_bias + cpu_icount_to_ns(icount); } diff --git a/include/qemu/timer.h b/include/qemu/timer.h index 5f5210d..38a02c5 100644 --- a/include/qemu/timer.h +++ b/include/qemu/timer.h @@ -743,6 +743,7 @@ static inline int64_t get_clock(void) #endif /* icount */ +int64_t cpu_get_instructions_counter(void); int64_t cpu_get_icount(void); int64_t cpu_get_clock(void); int64_t cpu_get_clock_offset(void);
Replay uses number of executed instructions to determine corrent events injection moments. This patch introduces new function for querying the instructions counter. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- cpus.c | 26 +++++++++++++++++++++++--- include/qemu/timer.h | 1 + 2 files changed, 24 insertions(+), 3 deletions(-)