Re: [Qemu-devel] [PATCH 05/13] target-openrisc: Remove TLB flush on exception

[Peter Maydell]

On 29 October 2013 19:04, Sebastian Macke <address@hidden> wrote:
> The TLB flush is not necessary as the mmu_index field
> already takes care of correct memory locations.
> Instead the tb flag field must be expanded that
> the exception takes the correct translation block.
>
> Signed-off-by: Sebastian Macke <address@hidden>
> ---
>  target-openrisc/cpu.h       | 4 ++--
>  target-openrisc/interrupt.c | 4 ----
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h
> index 24afe6f..057821d 100644
> --- a/target-openrisc/cpu.h
> +++ b/target-openrisc/cpu.h
> @@ -85,7 +85,7 @@ enum {
>  #define SPR_VR 0xFFFF003F
>
>  /* Internal flags, delay slot flag */
> -#define D_FLAG    1
> +#define D_FLAG    2

Since this set of #defines effectively is the documentation for
what the tb_flags usage is, can you update it to include the
new flag you've added, please?

>  /* Interrupt */
>  #define NR_IRQS  32
> @@ -412,7 +412,7 @@ static inline void cpu_get_tb_cpu_state(CPUOpenRISCState 
> *env,
>      *pc = env->pc;
>      *cs_base = 0;
>      /* D_FLAG -- branch instruction exception */
> -    *flags = (env->flags & D_FLAG);
> +    *flags = (env->flags & D_FLAG) | (env->sr & SR_SM);
>  }
>
>  static inline int cpu_mmu_index(CPUOpenRISCState *env)
> diff --git a/target-openrisc/interrupt.c b/target-openrisc/interrupt.c
> index d1d6ae2..ee98ed3 100644
> --- a/target-openrisc/interrupt.c
> +++ b/target-openrisc/interrupt.c
> @@ -41,10 +41,6 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
>          env->epcr += 4;
>      }
>
> -    /* For machine-state changed between user-mode and supervisor mode,
> -       we need flush TLB when we enter&exit EXCP.  */
> -    tlb_flush(env, 1);
> -
>      env->esr = ENV_GET_SR(env);
>      env->sr &= ~SR_DME;
>      env->sr &= ~SR_IME;

It looks suspicious that this patch doesn't include any change to
translate.c which reads the tb flag you've just added. Either:
 (a) the translated code doesn't actually build in any dependencies
  on the SR_SM flag, in which case it doesn't need to be a tb_flag at all
 (b) the translated code is still referring directly to env->sr somewhere,
  in which case it needs to be changed to use the tb_flags version instead

Also, are you sure that tlb_flush() is needed purely for the change to the
SR_SM flags and not for any of the other CPU state changes that
openrisc_cpu_do_interrupt() is making when it does the user->supervisor
state change?

thanks
-- PMM