Re: [Qemu-devel] [RFC PATCH v4 11/25] cpu-exec: allow temporary disabling icount

["Pavel Dovgaluk" <Pavel.Dovgaluk@ispras.ru>]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 07/11/2014 11:32, Pavel Dovgalyuk wrote:
> > This patch is required for deterministic replay to generate an exception
> > by trying executing an instruction without changing icount.
> > It adds new flag to TB for disabling icount while translating it.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  cpu-exec.c              |    6 +++---
> >  include/exec/exec-all.h |    7 ++++---
> >  translate-all.c         |   22 ++++++++++++++++++++--
> >  3 files changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 011f51f..e7206ac 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -198,7 +198,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t
> *tb_ptr)
> >  /* Execute the code without caching the generated code. An interpreter
> >     could be used if available. */
> >  static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
> > -                             TranslationBlock *orig_tb)
> > +                             TranslationBlock *orig_tb, bool ignore_icount)
> >  {
> >      CPUState *cpu = ENV_GET_CPU(env);
> >      TranslationBlock *tb;
> > @@ -214,7 +214,7 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
> >      /* tb_gen_code can flush our orig_tb, invalidate it now */
> >      tb_phys_invalidate(orig_tb, -1);
> >      tb = tb_gen_code(cpu, pc, cs_base, flags,
> > -                     max_cycles);
> > +                     max_cycles | (ignore_icount ? CF_IGNORE_ICOUNT : 0));
> >      cpu->current_tb = tb;
> >      /* execute the generated code */
> >      trace_exec_tb_nocache(tb, tb->pc);
> > @@ -516,7 +516,7 @@ int cpu_exec(CPUArchState *env)
> >                          } else {
> >                              if (insns_left > 0) {
> >                                  /* Execute remaining instructions.  */
> > -                                cpu_exec_nocache(env, insns_left, tb);
> > +                                cpu_exec_nocache(env, insns_left, tb, false);
> >                                  align_clocks(&sc, cpu);
> >                              }
> >                              cpu->exception_index = EXCP_INTERRUPT;
> > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > index 1d17f75..052e8ef 100644
> > --- a/include/exec/exec-all.h
> > +++ b/include/exec/exec-all.h
> > @@ -142,9 +142,10 @@ struct TranslationBlock {
> >      uint64_t flags; /* flags defining in which context the code was generated */
> >      uint16_t size;      /* size of target code for this block (1 <=
> >                             size <= TARGET_PAGE_SIZE) */
> > -    uint16_t cflags;    /* compile flags */
> > -#define CF_COUNT_MASK  0x7fff
> > -#define CF_LAST_IO     0x8000 /* Last insn may be an IO access.  */
> > +    uint32_t cflags;    /* compile flags */
> > +#define CF_COUNT_MASK    0x7fff
> > +#define CF_LAST_IO       0x8000 /* Last insn may be an IO access.  */
> > +#define CF_IGNORE_ICOUNT 0x10000 /* Do not generate icount code */
> >
> >      void *tc_ptr;    /* pointer to the translated code */
> >      /* next matching tb for physical address. */
> > diff --git a/translate-all.c b/translate-all.c
> > index 7177b71..182ea7d 100644
> > --- a/translate-all.c
> > +++ b/translate-all.c
> > @@ -207,6 +207,7 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock
> *tb,
> >      TCGContext *s = &tcg_ctx;
> >      int j;
> >      uintptr_t tc_ptr;
> > +    int old_icount = use_icount;
> >  #ifdef CONFIG_PROFILER
> >      int64_t ti;
> >  #endif
> > @@ -218,6 +219,10 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock
> *tb,
> >
> >      gen_intermediate_code_pc(env, tb);
> >
> > +    if (tb->cflags & CF_IGNORE_ICOUNT) {
> 
> Would it be possible to instead have a CF_USE_ICOUNT tbflag, and check
> it when applicable instead of use_icount?  It seems cleaner, because it
> would avoid playing with global variables, but perhaps it's hard to do it.

Using context-specific flag is better than using global variable like use_icount.
But I could not test such changes because they will affect all of the platforms
(and will be also hard to implement for i386).

Pavel Dovgalyuk