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 <luc.mic...@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)
>
signature.asc
Description: OpenPGP digital signature
