Re: [Qemu-devel] [PATCH 1/1] target-ppc: Fix booke206 tlbwe TLB instruction

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3154 bytes --]

On Thu, Nov 02, 2017 at 11:35:59AM +0100, Luc MICHEL wrote:
> When overwritting a valid TLB entry with a new one, the previous page
> were not flushed in QEMU TLB, leading to incoherent mapping. This commit
> fixes this.

I don't think this is right.  As a rule, overwriting a TLB entry
doesn't necessarily invalidate the previous entry, even on real
hardware.  I don't know exactly what the situation is on the various
FSL BookE chips, but I know various other models have other caches
ahead of the main TLB which can cache mappings that have been removed
from it (e.g. the ERAT on server chips and the shadow TLBs on 4xx).

To invalidate those other caches requires something other than simply
a tlbwe (tlbie for the ERAT and an isync for the shadow TLBs).

The current behaviour won't exactly match what hardware does (and it's
probably not practical to do so), but it should be within what's
permitted by the architecture - and therefore good enough for correct
guests.

It's possible that we do need this for the BookE chips, but it'll need
a more detailed rationale.

> 
> Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
> ---
>  target/ppc/mmu_helper.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 2a1f9902c9..c2c89239b4 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2570,6 +2570,17 @@ void helper_booke_setpid(CPUPPCState *env, uint32_t pidn, target_ulong pid)
>      tlb_flush(CPU(cpu));
>  }
>  
> +static inline void flush_page(CPUPPCState *env, ppcmas_tlb_t *tlb)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +
> +    if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
> +        tlb_flush_page(CPU(cpu), tlb->mas2 & MAS2_EPN_MASK);
> +    } else {
> +        tlb_flush(CPU(cpu));
> +    }
> +}
> +
>  void helper_booke206_tlbwe(CPUPPCState *env)
>  {
>      PowerPCCPU *cpu = ppc_env_get_cpu(env);
> @@ -2628,6 +2639,12 @@ void helper_booke206_tlbwe(CPUPPCState *env)
>      if (msr_gs) {
>          cpu_abort(CPU(cpu), "missing HV implementation\n");
>      }
> +
> +    if (tlb->mas1 & MAS1_VALID) {
> +        /* Invalidate the page in QEMU TLB if it was a valid entry */
> +        flush_page(env, tlb);
> +    }
> +
>      tlb->mas7_3 = ((uint64_t)env->spr[SPR_BOOKE_MAS7] << 32) |
>          env->spr[SPR_BOOKE_MAS3];
>      tlb->mas1 = env->spr[SPR_BOOKE_MAS1];
> @@ -2663,11 +2680,7 @@ void helper_booke206_tlbwe(CPUPPCState *env)
>          tlb->mas1 &= ~MAS1_IPROT;
>      }
>  
> -    if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
> -        tlb_flush_page(CPU(cpu), tlb->mas2 & MAS2_EPN_MASK);
> -    } else {
> -        tlb_flush(CPU(cpu));
> -    }
> +    flush_page(env, tlb);
>  }
>  
>  static inline void booke206_tlb_to_mas(CPUPPCState *env, ppcmas_tlb_t *tlb)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]