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