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

[Luc Michel]

On 11/06/2017 07:16 AM, David Gibson wrote:
> 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).
Indeed, e500 cores have a two-level TLB. tlbwe writes in L2, and L1 is
handled by the hardware and not visible to the user.

> 
> To invalidate those other caches requires something other than simply
> a tlbwe (tlbie for the ERAT and an isync for the shadow TLBs).
Yes you are right. From "EREF 2.0: A Programmer’s Reference Manual for
Freescale Power Architecture Processors, Rev. 0":

"A context synchronizing instruction is required after a tlbwe
instruction to ensure any subsequent instructions that will use the
updated TLB values execute in the new context."

Linux executes a msync followed by a isync after a tlbwe for BookE MMU
machines.

> 
> 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.
The one sentence from the "PowerPC e500 Core Family Reference Manual,
Rev. 1" document that makes my fix kind of correct is this one:

In 12.4.2 TLB Write Entry (tlbwe) Instruction:

"Note that when an L2 TLB entry is written, it may be displacing an
already valid entry in the same L2 TLB location (a victim). If a valid
L1 TLB entry corresponds to the L2 MMU victim entry, that L1 TLB entry
is automatically invalidated."

At least, it is as correct as the current tlb_flush in
"helper_booke206_tlbwe" function, since it does not wait for isync to
effectively invalidate the page.

> 
>>
>> Signed-off-by: Luc MICHEL <address@hidden>
>> ---
>>  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)
>

signature.asc
Description: OpenPGP digital signature