diff mbox

[v7,04/14] icount: exit cpu loop on expire

Message ID 20170124071719.4572.92155.stgit@PASHA-ISP (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Dovgalyuk Jan. 24, 2017, 7:17 a.m. UTC
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(-)

Comments

Paolo Bonzini Jan. 25, 2017, 11:06 a.m. UTC | #1
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
>
Pavel Dovgalyuk Jan. 25, 2017, 11:50 a.m. UTC | #2
> 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
Paolo Bonzini Jan. 25, 2017, noon UTC | #3
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 mbox

Patch

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