| Message ID | 20170124071719.4572.92155.stgit@PASHA-ISP (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/01/2017 08:17, Pavel Dovgalyuk wrote: > This patch adds check to break cpu loop when icount expires without > setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no > available translated blocks and all instructions were executed. > In icount replay mode unnecessary tb_find will be called (which may > cause an exception) and execution will be non-deterministic. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > cpu-exec.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 79a2167..e603da4 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -582,9 +582,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, > cpu_exec_nocache(cpu, insns_left, *last_tb, false); > align_clocks(sc, cpu); > } > - cpu->exception_index = EXCP_INTERRUPT; > - *last_tb = NULL; > - cpu_loop_exit(cpu); > } > break; > #endif > @@ -638,6 +635,18 @@ int cpu_exec(CPUState *cpu) > > for(;;) { > cpu_handle_interrupt(cpu, &last_tb); > + /* icount has expired, we need to break the execution loop. > + This check is needed before tb_find to make execution > + deterministic - tb_find may cause an exception > + while translating the code from non-mapped page. */ > + if (use_icount && ((cpu->icount_extra == 0 > + && cpu->icount_decr.u16.low == 0) > + || (int32_t)cpu->icount_decr.u32 < 0)) { > + if (cpu->exception_index == -1) { > + cpu->exception_index = EXCP_INTERRUPT; > + } > + cpu_loop_exit(cpu); > + } Can this can be placed in cpu_handle_interrupt itself? Also, can the cpu->exception_index != -1 case happen? We really should add assertions on cpu->exception_index, maybe after cpu_handle_exception and cc->cpu_exec_interrupt returns true. Without some idea of the invariants, I'm not competent enough to review this patch. Paolo > tb = tb_find(cpu, last_tb, tb_exit); > cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc); > /* Try to align the host and virtual clocks >
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 24/01/2017 08:17, Pavel Dovgalyuk wrote: > > This patch adds check to break cpu loop when icount expires without > > setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no > > available translated blocks and all instructions were executed. > > In icount replay mode unnecessary tb_find will be called (which may > > cause an exception) and execution will be non-deterministic. > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > --- > > cpu-exec.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/cpu-exec.c b/cpu-exec.c > > index 79a2167..e603da4 100644 > > --- a/cpu-exec.c > > +++ b/cpu-exec.c > > @@ -582,9 +582,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, > > cpu_exec_nocache(cpu, insns_left, *last_tb, false); > > align_clocks(sc, cpu); > > } > > - cpu->exception_index = EXCP_INTERRUPT; > > - *last_tb = NULL; > > - cpu_loop_exit(cpu); > > } > > break; > > #endif > > @@ -638,6 +635,18 @@ int cpu_exec(CPUState *cpu) > > > > for(;;) { > > cpu_handle_interrupt(cpu, &last_tb); > > + /* icount has expired, we need to break the execution loop. > > + This check is needed before tb_find to make execution > > + deterministic - tb_find may cause an exception > > + while translating the code from non-mapped page. */ > > + if (use_icount && ((cpu->icount_extra == 0 > > + && cpu->icount_decr.u16.low == 0) > > + || (int32_t)cpu->icount_decr.u32 < 0)) { > > + if (cpu->exception_index == -1) { > > + cpu->exception_index = EXCP_INTERRUPT; > > + } > > + cpu_loop_exit(cpu); > > + } > > Can this can be placed in cpu_handle_interrupt itself? I guess it could. I placed it here because it doesn't related to interrupts. > Also, can the cpu->exception_index != -1 case happen? This branch is executed where there are no instructions to replay. It may happen due to an exception (which is already set) or because execution loop has to break. In the first case exception_index != -1. > We really should add assertions on cpu->exception_index, maybe after > cpu_handle_exception and cc->cpu_exec_interrupt returns true. Without > some idea of the invariants, I'm not competent enough to review this patch. Pavel Dovgalyuk
On 25/01/2017 12:50, Pavel Dovgalyuk wrote: >>> + /* icount has expired, we need to break the execution loop. >>> + This check is needed before tb_find to make execution >>> + deterministic - tb_find may cause an exception >>> + while translating the code from non-mapped page. */ >>> + if (use_icount && ((cpu->icount_extra == 0 >>> + && cpu->icount_decr.u16.low == 0) >>> + || (int32_t)cpu->icount_decr.u32 < 0)) { >>> + if (cpu->exception_index == -1) { >>> + cpu->exception_index = EXCP_INTERRUPT; >>> + } >>> + cpu_loop_exit(cpu); >>> + } >> Can this can be placed in cpu_handle_interrupt itself? > I guess it could. I placed it here because it doesn't related to interrupts. True, on the other hand neither is if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) { atomic_set(&cpu->exit_request, 0); cpu->exception_index = EXCP_INTERRUPT; cpu_loop_exit(cpu); } Except for the replay_has_interrupt() that you added, but I don't understand that one either... Paolo
diff --git a/cpu-exec.c b/cpu-exec.c index 79a2167..e603da4 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -582,9 +582,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, cpu_exec_nocache(cpu, insns_left, *last_tb, false); align_clocks(sc, cpu); } - cpu->exception_index = EXCP_INTERRUPT; - *last_tb = NULL; - cpu_loop_exit(cpu); } break; #endif @@ -638,6 +635,18 @@ int cpu_exec(CPUState *cpu) for(;;) { cpu_handle_interrupt(cpu, &last_tb); + /* icount has expired, we need to break the execution loop. + This check is needed before tb_find to make execution + deterministic - tb_find may cause an exception + while translating the code from non-mapped page. */ + if (use_icount && ((cpu->icount_extra == 0 + && cpu->icount_decr.u16.low == 0) + || (int32_t)cpu->icount_decr.u32 < 0)) { + if (cpu->exception_index == -1) { + cpu->exception_index = EXCP_INTERRUPT; + } + cpu_loop_exit(cpu); + } tb = tb_find(cpu, last_tb, tb_exit); cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc); /* Try to align the host and virtual clocks
This patch adds check to break cpu loop when icount expires without setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no available translated blocks and all instructions were executed. In icount replay mode unnecessary tb_find will be called (which may cause an exception) and execution will be non-deterministic. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- cpu-exec.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)