[Qemu-devel] [PATCH 0/1] target-ppc: booke206 tlb: fix tlbwe instruction

[Qemu-devel] [PATCH 0/1] target-ppc: booke206 tlb: fix tlbwe instruction

[Luc MICHEL]

Hi,

I was experiencing random segmentation faults of userland applications
in a guest e500 powerpc Linux. After investigating, I found that this
bug appeared with commit 9fb044911444fdd09f5f072ad0ca269d7f8b841d. This
commit introduces more MMU indices to avoid unnecessary TLB flushes when
the CPU changes mode.

It triggers a new bug however, that I finally traced down into the tlbwe
instructions simulation. When replacing a valid TLB entry with a new
one, the previous page was not flushed from QEMU TLB.

This fixes my random crashes in guest Linux. Note that I think there is
a similar issue in booke206_invalidate_ea_tlb but in my case, Linux
never triggers this code so I was not able to test.

Luc MICHEL (1):
  target-ppc: Fix booke206 tlbwe TLB instruction

 target/ppc/mmu_helper.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
2.14.3

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

[Luc MICHEL]

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.

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)
-- 
2.14.3

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

[David Gibson]

[-- 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 --]

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

[Luc Michel]

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

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.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)
> 


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

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

[David Gibson]

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

On Tue, Nov 14, 2017 at 05:28:48PM +0100, Luc Michel wrote:
> 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."

That does seem fairly clear.  If you resend the patch with that
paragraph cited in a comment, I'll apply it.  A link to the reference
manual would also be appreciated.

> 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.



-- 
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 --]