[PATCH 1/2] powerpc/e6500: hw tablewalk: clear TID in kernel indirect entries

[PATCH 1/2] powerpc/e6500: hw tablewalk: clear TID in kernel indirect entries

[Scott Wood]

Previously TID was being cleared before the tlbsx, but not after.  This
can lead to a multiway hit between a TLB entry with TID=0 (previously
inserted when PID=0) and a TLB entry with TID!=0 that matches PID.
This can theoretically result in undefined behavior, though we probably
get lucky due to the details of the overlap.  It also results in the
inability to use multihit detection to detect other conflicting TLB
entries, as well as poorer TLB utilization due to duplicating kernel
TLB entries.

Rather than try to patch up MAS1 after tlbsx, the entire value is
saved/restored as with MAS2.

I observed a slight improvement in TLB miss performance with this patch
applied.

Signed-off-by: Scott Wood <scottwood@freescale.com>
Reported-by: Ed Swarthout <ed.swarthout@freescale.com>
---
 arch/powerpc/mm/tlb_low_64e.S | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 356e8b4..3298d10 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -322,19 +322,17 @@ tlb_miss_common_e6500:
 	b	1b
 	.previous
 
-	mfspr	r15,SPRN_MAS2
+	mfspr	r15,SPRN_MAS1
+	mfspr	r10,SPRN_MAS2
 
 	tlbsx	0,r16
+	mtspr	SPRN_MAS2,r10
 	mfspr	r10,SPRN_MAS1
+	mtspr	SPRN_MAS1,r15
+
 	andis.	r10,r10,MAS1_VALID@h
 	bne	tlb_miss_done_e6500
 
-	/* Undo MAS-damage from the tlbsx */
-	mfspr	r10,SPRN_MAS1
-	oris	r10,r10,MAS1_VALID@h
-	mtspr	SPRN_MAS1,r10
-	mtspr	SPRN_MAS2,r15
-
 	/* Now, we need to walk the page tables. First check if we are in
 	 * range.
 	 */
-- 
1.9.1

[PATCH 2/2] powerpc/e6500: hw tablewalk: fix recursive tlb lock on cpu 0

[Scott Wood]

Commit 82d86de25b9c99db546e17c6f7ebf9a691da557e "TLB lock recursive"
introduced a bug whereby cpu 0 uses the same value for "lock held" as
is used to indicate that the lock is free.  This means that cpu 1 can
acquire the lock whenever it wants, regardless of whether cpu 0 has it
locked, which in turn means we can get duplicate TLB entries.

Add one to the CPU value to ensure we do not use zero as a "lock held"
value.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/mm/tlb_low_64e.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 3298d10..ba3ba3c 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -309,6 +309,7 @@ tlb_miss_common_e6500:
 	lhz	r10,PACAPACAINDEX(r13)
 	cmpdi	r15,0
 	cmpdi	cr1,r15,1	/* set cr1.eq = 0 for non-recursive */
+	addi	r10,r10,1
 	bne	2f
 	stbcx.	r10,0,r11
 	bne	1b
-- 
1.9.1

RE: [PATCH 2/2] powerpc/e6500: hw tablewalk: fix recursive tlb lock on cpu 0

[mihai.caraman]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBMaW51eHBwYy1kZXYgW21haWx0
bzpsaW51eHBwYy1kZXYtDQo+IGJvdW5jZXMrbWloYWkuY2FyYW1hbj1mcmVlc2NhbGUuY29tQGxp
c3RzLm96bGFicy5vcmddIE9uIEJlaGFsZiBPZiBTY290dA0KPiBXb29kDQo+IFNlbnQ6IEZyaWRh
eSwgTWF5IDIzLCAyMDE0IDEyOjQ1IEFNDQo+IFRvOiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJz
Lm9yZw0KPiBDYzogV29vZCBTY290dC1CMDc0MjENCj4gU3ViamVjdDogW1BBVENIIDIvMl0gcG93
ZXJwYy9lNjUwMDogaHcgdGFibGV3YWxrOiBmaXggcmVjdXJzaXZlIHRsYiBsb2NrDQo+IG9uIGNw
dSAwDQo+IA0KPiBDb21taXQgODJkODZkZTI1YjljOTlkYjU0NmUxN2M2ZjdlYmY5YTY5MWRhNTU3
ZSAiVExCIGxvY2sgcmVjdXJzaXZlIg0KPiBpbnRyb2R1Y2VkIGEgYnVnIHdoZXJlYnkgY3B1IDAg
dXNlcyB0aGUgc2FtZSB2YWx1ZSBmb3IgImxvY2sgaGVsZCIgYXMNCj4gaXMgdXNlZCB0byBpbmRp
Y2F0ZSB0aGF0IHRoZSBsb2NrIGlzIGZyZWUuDQoNCklzbid0IGhpcyB3aGF0IHNwaW4gbG9jayBp
bXBsZW1lbnRhdGlvbiBzb2x2ZXMgYnkgY29tYmluZXMgcGFjYV9pbmRleA0Kd2l0aCBsb2NrX3Rv
a2VuPyBDYW4ndCB3ZSBoYXZlIGEgY29tbW9uIGFwcHJvYWNoPw0KDQo+IEFkZCBvbmUgdG8gdGhl
IENQVSB2YWx1ZSB0byBlbnN1cmUgd2UgZG8gbm90IHVzZSB6ZXJvIGFzIGEgImxvY2sgaGVsZCIN
Cj4gdmFsdWUuDQoNClRoZSBDUFUgdmFsdWUgaXMgbG9hZGVkIGluIHIxMCBmcm9tIHRsYl9taXNz
X2NvbW1vbl9lNjUwMC4gIlRMQiBsb2NrIHJlY3Vyc2l2ZSINCmNvbW1pdCBhbHNvIGludHJvZHVj
ZWQgdGhpcyBtaXNsZWFkaW5nIGNvbW1lbnQ6DQoNCiAgICBXZSBhcmUgZW50ZXJlZCB3aXRoOg0K
ICAgIHIxMCA9IGNwdSBudW1iZXINCg0KLU1pa2UNCg==

Re: [PATCH 2/2] powerpc/e6500: hw tablewalk: fix recursive tlb lock on cpu 0

[Scott Wood]

On Fri, 2014-05-30 at 02:59 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Linuxppc-dev [mailto:linuxppc-dev-
> > bounces+mihai.caraman=freescale.com@lists.ozlabs.org] On Behalf Of Scott
> > Wood
> > Sent: Friday, May 23, 2014 12:45 AM
> > To: linuxppc-dev@lists.ozlabs.org
> > Cc: Wood Scott-B07421
> > Subject: [PATCH 2/2] powerpc/e6500: hw tablewalk: fix recursive tlb lock
> > on cpu 0
> > 
> > Commit 82d86de25b9c99db546e17c6f7ebf9a691da557e "TLB lock recursive"
> > introduced a bug whereby cpu 0 uses the same value for "lock held" as
> > is used to indicate that the lock is free.
> 
> Isn't his what spin lock implementation solves by combines paca_index
> with lock_token? Can't we have a common approach?

That would require expanding the lock to 32 bits, is a more intrusive
fix than needed, and invites breakage in the TLB code if the lock_token
mechanism were to change.

> > Add one to the CPU value to ensure we do not use zero as a "lock held"
> > value.
> 
> The CPU value is loaded in r10 from tlb_miss_common_e6500. "TLB lock recursive"
> commit also introduced this misleading comment:
> 
>     We are entered with:
>     r10 = cpu number

I addressed this in v2 that I posted yesterday.

-Scott