Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Sun, 2016-06-05 at 19:41 +0200, Cédric Le Goater wrote:
> 
> Here is a fix I think. Could you give it a try ? 

This is somewhat wrong...

> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
> introduced an optimisation to flush TLBs only when a context
> synchronizing event is reached (interrupt, rfi). This was done for
> ppc64 but 32bit was forgotten on the way.

No it didn't. That commit only delays flushes on ppc64. ppc32 is
unaffected, unless I missed something. IE. It will delay flushes caused
by slb instructions (which don't exist on 32-bit)
and ppc_tlb_invalidate_one() only in the 64-bit cases.

Also what your patch does in practice is not really change that, though
you seem to try to somewhat extend the batching to 32-bit (but
incompletely), you also introduce something which effectively reverts
part of 9fb044911444fdd09f5f072ad0ca269d7f8b841d (split I/D mode).

I think that's more what's "fixing" your problem, ie, the flush in
IR/DR changes. However it shouldn't be needed.

I suspect all of that is papering over another bug somewhere else which
got exposed by the split I/D mode, since we no longer over-flush on
transitions to/from real-mode. So we must be missing flushes elsewhere,
possibly some G3 specific stuff, or there always was some kind of bug
in the TLB flushing on 32-bit that got somewhat masked by the over-
flushing we used to do.

I need a repro-case.

Cheers,
Ben.

> Tested on mac99 and g3beige with
> 
>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  I think the hunk in powerpc_excp() is needed if we don't generate a
>  context synchronizing event. what is best to do ?
> 
>  target-ppc/cpu.h         |    2 +-
>  target-ppc/excp_helper.c |   10 ++++++++++
>  target-ppc/helper_regs.h |    9 ++++++++-
>  target-ppc/translate.c   |    2 +-
>  4 files changed, 20 insertions(+), 3 deletions(-)
> 
> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>  {
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
> +#if !defined(CONFIG_USER_ONLY)
>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>  {
>      TCGv_i32 t = tcg_temp_new_i32();
> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
> @@ -958,9 +958,9 @@ struct CPUPPCState {
>      /* PowerPC 64 SLB area */
>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>      int32_t slb_nr;
> +#endif
>      /* tcg TLB needs flush (deferred slb inval instruction
> typically) */
>      uint32_t tlb_need_flush;
> -#endif
>      /* segment registers */
>      hwaddr htab_base;
>      /* mask used to normalize hash value to PTEG index */
> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>      }
>      if (((value >> MSR_IR) & 1) != msr_ir ||
>          ((value >> MSR_DR) & 1) != msr_dr) {
> +        /* A change of the instruction relocation bit in the MSR can
> +         * cause an implicit branch in the address space. This
> +         * requires a tlb flush.
> +         */
> +        if (env->mmu_model & POWERPC_MMU_32B) {
> +            env->tlb_need_flush = 1;
> +        }
>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>      }
>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>      return excp;
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
> +#if !defined(CONFIG_USER_ONLY)
>  static inline void check_tlb_flush(CPUPPCState *env)
>  {
>      CPUState *cs = CPU(ppc_env_get_cpu(env));
> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>          }
>      }
>  #endif
> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
> +        /* A change of the instruction relocation bit in the MSR can
> +         * cause an implicit branch in the address space. This
> +         * requires a tlb flush.
> +         */
> +        if (env->mmu_model & POWERPC_MMU_32B) {
> +            env->tlb_need_flush = 1;
> +        }
> +    }
>      /* We don't use hreg_store_msr here as already have treated
>       * any special case that could occur. Just store MSR and update
> hflags
>       *
>