Re: [PATCH -V5 13/25] powerpc: Update tlbie/tlbiel as per ISA doc

On Thu, Apr 11, 2013 at 10:50:12AM +0530, Aneesh Kumar K.V wrote:

David Gibson [off-list ref] writes:

quoted

On Thu, Apr 04, 2013 at 11:27:51AM +0530, Aneesh Kumar K.V wrote:

quoted

From: "Aneesh Kumar K.V" <redacted>

This make sure we handle multiple page size segment correctly.

This needs a much more detailed message.  In what way was the existing
code not matching the ISA documentation?  What consequences did that
have?

Mostly to make sure we use the right penc values in tlbie. I did test
these changes on PowerNV. 

A vague description like this is not adequate.  Your commit message
needs to explain what was wrong with the existing behaviour.

quoted

quoted

Signed-off-by: Aneesh Kumar K.V <redacted>
---
 arch/powerpc/mm/hash_native_64.c |   30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/hash_native_64.c b/arch/powerpc/mm/hash_native_64.c
index b461b2d..ac84fa6 100644
--- a/arch/powerpc/mm/hash_native_64.c
+++ b/arch/powerpc/mm/hash_native_64.c

@@ -61,7 +61,10 @@ static inline void __tlbie(unsigned long vpn, int psize, int apsize, int ssize)
 
 	switch (psize) {
 	case MMU_PAGE_4K:
+		/* clear out bits after (52) [0....52.....63] */
+		va &= ~((1ul << (64 - 52)) - 1);
 		va |= ssize << 8;
+		va |= mmu_psize_defs[apsize].sllp << 6;

sllp is the per-segment encoding, so it sure must be looked up via
psize, not apsize.

as per ISA doc, for base page size 4K, RB[56:58] must be set to
SLB[L|LP] encoded for the page size corresponding to the actual page
size specified by the PTE that was used to create the the TLB entry to
be invalidated.

Ok, I see.  Wow, our architecture is even more convoluted than I
thought.  This could really do with a comment, because this is a very
surprising aspect of the architecture.

quoted

quoted

 		asm volatile(ASM_FTR_IFCLR("tlbie %0,0", PPC_TLBIE(%1,%0), %2)
 			     : : "r" (va), "r"(0), "i" (CPU_FTR_ARCH_206)
 			     : "memory");

@@ -69,9 +72,19 @@ static inline void __tlbie(unsigned long vpn, int psize, int apsize, int ssize)
 	default:
 		/* We need 14 to 14 + i bits of va */
 		penc = mmu_psize_defs[psize].penc[apsize];
-		va &= ~((1ul << mmu_psize_defs[psize].shift) - 1);
+		va &= ~((1ul << mmu_psize_defs[apsize].shift) - 1);
 		va |= penc << 12;
 		va |= ssize << 8;
+		/* Add AVAL part */
+		if (psize != apsize) {
+			/*
+			 * MPSS, 64K base page size and 16MB parge page size
+			 * We don't need all the bits, but this seems to work.
+			 * vpn cover upto 65 bits of va. (0...65) and we need
+			 * 58..64 bits of va.

"seems to work" is not a comment I like to see in core MMU code...

As per ISA spec, the "other bits" in RB[56:62] must be ignored by the
processor. Hence I didn't bother to do zero it out. Since we only
support one MPSS combination, we could easily zero out using 0xf0. 

Then update the comment to clearly explain why what you're doing is
correct, not just say it "seems to work".

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