[PATCH] fixes for the SLB shadow buffer

[PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
for finding this.

Also added some write barriers to ensure the shadow buffer is always
valid.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
Paulus: unless someone has a problem with my implementation, this should
        go up for 2.6.23. 

 arch/powerpc/kernel/entry_64.S   |    2 ++
 arch/powerpc/mm/hash_utils_64.c  |    7 +++++++
 arch/powerpc/mm/slb.c            |   10 +++++-----
 include/asm-powerpc/mmu-hash64.h |    4 ++++
 4 files changed, 18 insertions(+), 5 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/entry_64.S
+++ linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
@@ -389,7 +389,9 @@ BEGIN_FTR_SECTION
 	ld	r9,PACA_SLBSHADOWPTR(r13)
 	li	r12,0
 	std	r12,SLBSHADOW_STACKESID(r9) /* Clear ESID */
+	eieio
 	std	r7,SLBSHADOW_STACKVSID(r9)  /* Save VSID */
+	eieio
 	std	r0,SLBSHADOW_STACKESID(r9)  /* Save ESID */
 
 	slbie	r6
Index: linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/mm/hash_utils_64.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
@@ -629,6 +629,9 @@ int hash_page(unsigned long ea, unsigned
 	cpumask_t tmp;
 	int rc, user_region = 0, local = 0;
 	int psize;
+#ifdef CONFIG_PPC_64K_PAGES
+	unsigned long vflags;
+#endif
 
 	DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
 		ea, access, trap);
@@ -759,6 +762,10 @@ int hash_page(unsigned long ea, unsigned
 		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
 		get_paca()->vmalloc_sllp =
 			mmu_psize_defs[mmu_vmalloc_psize].sllp;
+		vflags = SLB_VSID_KERNEL |
+			mmu_psize_defs[mmu_vmalloc_psize].sllp;
+		slb_shadow_update(mk_esid_data(VMALLOC_START, 1),
+				  mk_vsid_data(VMALLOC_START, vflags), 1);
 		slb_flush_and_rebolt();
 	}
 #endif /* CONFIG_PPC_64K_PAGES */
Index: linux-2.6-ozlabs/arch/powerpc/mm/slb.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/mm/slb.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/slb.c
@@ -43,17 +43,17 @@ static void slb_allocate(unsigned long e
 	slb_allocate_realmode(ea);
 }
 
-static inline unsigned long mk_esid_data(unsigned long ea, unsigned long slot)
+unsigned long mk_esid_data(unsigned long ea, unsigned long slot)
 {
 	return (ea & ESID_MASK) | SLB_ESID_V | slot;
 }
 
-static inline unsigned long mk_vsid_data(unsigned long ea, unsigned long flags)
+unsigned long mk_vsid_data(unsigned long ea, unsigned long flags)
 {
 	return (get_kernel_vsid(ea) << SLB_VSID_SHIFT) | flags;
 }
 
-static inline void slb_shadow_update(unsigned long esid, unsigned long vsid,
+void slb_shadow_update(unsigned long esid, unsigned long vsid,
 				     unsigned long entry)
 {
 	/*
@@ -61,9 +61,9 @@ static inline void slb_shadow_update(uns
 	 * updating it.
 	 */
 	get_slb_shadow()->save_area[entry].esid = 0;
-	barrier();
+	smp_wmb();
 	get_slb_shadow()->save_area[entry].vsid = vsid;
-	barrier();
+	smp_wmb();
 	get_slb_shadow()->save_area[entry].esid = esid;
 
 }
Index: linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
===================================================================
--- linux-2.6-ozlabs.orig/include/asm-powerpc/mmu-hash64.h
+++ linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
@@ -262,6 +262,10 @@ extern void slb_initialize(void);
 extern void slb_flush_and_rebolt(void);
 extern void stab_initialize(unsigned long stab);
 
+extern unsigned long mk_esid_data(unsigned long ea, unsigned long slot);
+extern unsigned long mk_vsid_data(unsigned long ea, unsigned long flags);
+extern void slb_shadow_update(unsigned long esid, unsigned long vsid,
+			      unsigned long entry);
 #endif /* __ASSEMBLY__ */
 
 /*

Re: [PATCH] fixes for the SLB shadow buffer

[Paul Mackerras]

Michael Neuling writes:

> +		slb_shadow_update(mk_esid_data(VMALLOC_START, 1),
> +				  mk_vsid_data(VMALLOC_START, vflags), 1);

Could you re-jig slb_shadow_update to take ea, slot and vflags, and
call mk_[ev]sid_data itself, rather than exposing mk_esid_data and
mk_vsid_data, please?

Paul.

[PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
for finding this.

Also added some write barriers to ensure the shadow buffer is always
valid.

Signed-off-by: Michael Neuling <mikey@neuling.org>

---
> > +		slb_shadow_update(mk_esid_data(VMALLOC_START, 1),
> > +				  mk_vsid_data(VMALLOC_START, vflags), 1);
> 
> Could you re-jig slb_shadow_update to take ea, slot and vflags, and
> call mk_[ev]sid_data itself, rather than exposing mk_esid_data and
> mk_vsid_data, please?

Sure... this closer to what you want?

 arch/powerpc/kernel/entry_64.S   |    2 ++
 arch/powerpc/mm/hash_utils_64.c  |    6 ++++++
 arch/powerpc/mm/slb.c            |   19 +++++++++----------
 include/asm-powerpc/mmu-hash64.h |    3 +++
 4 files changed, 20 insertions(+), 10 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/entry_64.S
+++ linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
@@ -389,7 +389,9 @@ BEGIN_FTR_SECTION
 	ld	r9,PACA_SLBSHADOWPTR(r13)
 	li	r12,0
 	std	r12,SLBSHADOW_STACKESID(r9) /* Clear ESID */
+	eieio
 	std	r7,SLBSHADOW_STACKVSID(r9)  /* Save VSID */
+	eieio
 	std	r0,SLBSHADOW_STACKESID(r9)  /* Save ESID */
 
 	slbie	r6
Index: linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/mm/hash_utils_64.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
@@ -629,6 +629,9 @@ int hash_page(unsigned long ea, unsigned
 	cpumask_t tmp;
 	int rc, user_region = 0, local = 0;
 	int psize;
+#ifdef CONFIG_PPC_64K_PAGES
+	unsigned long vflags;
+#endif
 
 	DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
 		ea, access, trap);
@@ -759,6 +762,9 @@ int hash_page(unsigned long ea, unsigned
 		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
 		get_paca()->vmalloc_sllp =
 			mmu_psize_defs[mmu_vmalloc_psize].sllp;
+		vflags = SLB_VSID_KERNEL |
+			mmu_psize_defs[mmu_vmalloc_psize].sllp;
+		slb_shadow_update(VMALLOC_START, vflags, 1);
 		slb_flush_and_rebolt();
 	}
 #endif /* CONFIG_PPC_64K_PAGES */
Index: linux-2.6-ozlabs/arch/powerpc/mm/slb.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/mm/slb.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/slb.c
@@ -53,18 +53,19 @@ static inline unsigned long mk_vsid_data
 	return (get_kernel_vsid(ea) << SLB_VSID_SHIFT) | flags;
 }
 
-static inline void slb_shadow_update(unsigned long esid, unsigned long vsid,
-				     unsigned long entry)
+void slb_shadow_update(unsigned long ea,
+		       unsigned long flags,
+		       unsigned long entry)
 {
 	/*
 	 * Clear the ESID first so the entry is not valid while we are
 	 * updating it.
 	 */
 	get_slb_shadow()->save_area[entry].esid = 0;
-	barrier();
-	get_slb_shadow()->save_area[entry].vsid = vsid;
-	barrier();
-	get_slb_shadow()->save_area[entry].esid = esid;
+	smp_wmb();
+	get_slb_shadow()->save_area[entry].vsid = mk_vsid_data(ea, flags);
+	smp_wmb();
+	get_slb_shadow()->save_area[entry].esid = mk_esid_data(ea, entry);
 
 }
 
@@ -76,8 +77,7 @@ static inline void create_shadowed_slbe(
 	 * we don't get a stale entry here if we get preempted by PHYP
 	 * between these two statements.
 	 */
-	slb_shadow_update(mk_esid_data(ea, entry), mk_vsid_data(ea, flags),
-			  entry);
+	slb_shadow_update(ea, flags, entry);
 
 	asm volatile("slbmte  %0,%1" :
 		     : "r" (mk_vsid_data(ea, flags)),
@@ -104,8 +104,7 @@ void slb_flush_and_rebolt(void)
 		ksp_esid_data &= ~SLB_ESID_V;
 
 	/* Only third entry (stack) may change here so only resave that */
-	slb_shadow_update(ksp_esid_data,
-			  mk_vsid_data(ksp_esid_data, lflags), 2);
+	slb_shadow_update(get_paca()->kstack, lflags, 2);
 
 	/* We need to do this all in asm, so we're sure we don't touch
 	 * the stack between the slbia and rebolting it. */
Index: linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
===================================================================
--- linux-2.6-ozlabs.orig/include/asm-powerpc/mmu-hash64.h
+++ linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
@@ -262,6 +262,9 @@ extern void slb_initialize(void);
 extern void slb_flush_and_rebolt(void);
 extern void stab_initialize(unsigned long stab);
 
+extern void slb_shadow_update(unsigned long ea,
+			      unsigned long flags,
+			      unsigned long entry);
 #endif /* __ASSEMBLY__ */
 
 /*

Re: [PATCH] fixes for the SLB shadow buffer

[Will Schmidt]

On Wed, 2007-08-01 at 16:02 +1000, Michael Neuling wrote:
> --- linux-2.6-ozlabs.orig/arch/powerpc/kernel/entry_64.S
> +++ linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
> @@ -389,7 +389,9 @@ BEGIN_FTR_SECTION
>  	ld	r9,PACA_SLBSHADOWPTR(r13)
>  	li	r12,0
>  	std	r12,SLBSHADOW_STACKESID(r9) /* Clear ESID */
> +	eieio
>  	std	r7,SLBSHADOW_STACKVSID(r9)  /* Save VSID */
> +	eieio
>  	std	r0,SLBSHADOW_STACKESID(r9)  /* Save ESID */
> 
Hi Michael, 

I was going to ask if we really needed both of them, but think I
convinced myself that we do.
 
Do we also want/need an eieio after the /* Save ESID */ statement, or is
that somehow handled by the slbie following?

-Will 

Re: [PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

> > --- linux-2.6-ozlabs.orig/arch/powerpc/kernel/entry_64.S
> > +++ linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
> > @@ -389,7 +389,9 @@ BEGIN_FTR_SECTION
> >  	ld	r9,PACA_SLBSHADOWPTR(r13)
> >  	li	r12,0
> >  	std	r12,SLBSHADOW_STACKESID(r9) /* Clear ESID */
> > +	eieio
> >  	std	r7,SLBSHADOW_STACKVSID(r9)  /* Save VSID */
> > +	eieio
> >  	std	r0,SLBSHADOW_STACKESID(r9)  /* Save ESID */
> > 
> Hi Michael, 
> 
> I was going to ask if we really needed both of them, but think I
> convinced myself that we do.
>  
> Do we also want/need an eieio after the /* Save ESID */ statement, or is
> that somehow handled by the slbie following?

Actually, I think we can remove the barriers all together.

The ordering depends on how the buffer is accessed.

If each CPU only access it's own shadow buffer, then we are ok, but this
isn't the case when we take a checkpoint restart, like in POWER6, the
buffer must be read by a separate CPU.  

But even in the case of a checkpoint restart, the ordering will be
preserved as the NIA we get as part of the checkpoint will have all
previous instructions complete and none of the following instructions
started.

So I guess the questions is, does PHYP even need to access the shadow
buffer of another CPU, while that other CPU is in flight.  I'm not sure
that they can as they can't read the entire buffer atomically if the
target CPU is still active.  So PHYP must stop instructions on the
target CPU, before it reads it's shadow buffer.  Hence no ordering
problems.

I should probably talk to some PHYP guys to confirm, but i think we can
remove all the barriers when writing to the shadow buffer

Mikey

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

On Thu, 2007-08-02 at 15:56 +1000, Michael Neuling wrote:
> 
> But even in the case of a checkpoint restart, the ordering will be
> preserved as the NIA we get as part of the checkpoint will have all
> previous instructions complete and none of the following instructions
> started.

Instruction completion isn't enough to ensure storage ordering. The
stores may well be complete but the data still in separate store queues.

> So I guess the questions is, does PHYP even need to access the shadow
> buffer of another CPU, while that other CPU is in flight.  I'm not
> sure
> that they can as they can't read the entire buffer atomically if the
> target CPU is still active.  So PHYP must stop instructions on the
> target CPU, before it reads it's shadow buffer.  Hence no ordering
> problems.
> 
> I should probably talk to some PHYP guys to confirm, but i think we
> can
> remove all the barriers when writing to the shadow buffer

Bah, just keep then in, eieio's won't hurt much and it doesn't look like
a critically hot code path.

Ben.

Re: [PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

> > 
> > But even in the case of a checkpoint restart, the ordering will be
> > preserved as the NIA we get as part of the checkpoint will have all
> > previous instructions complete and none of the following instructions
> > started.
> 
> Instruction completion isn't enough to ensure storage ordering. The
> stores may well be complete but the data still in separate store queues.

POWER6 flushes the store queues when we take a checkpoint.  

> 
> > So I guess the questions is, does PHYP even need to access the shadow
> > buffer of another CPU, while that other CPU is in flight.  I'm not
> > sure
> > that they can as they can't read the entire buffer atomically if the
> > target CPU is still active.  So PHYP must stop instructions on the
> > target CPU, before it reads it's shadow buffer.  Hence no ordering
> > problems.
> > 
> > I should probably talk to some PHYP guys to confirm, but i think we
> > can
> > remove all the barriers when writing to the shadow buffer
> 
> Bah, just keep then in, eieio's won't hurt much and it doesn't look like
> a critically hot code path.

Ok

Mikey

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

On Thu, 2007-08-02 at 18:56 +1000, Michael Neuling wrote:
> > > But even in the case of a checkpoint restart, the ordering will be
> > > preserved as the NIA we get as part of the checkpoint will have
> all
> > > previous instructions complete and none of the following
> instructions
> > > started.
> > 
> > Instruction completion isn't enough to ensure storage ordering. The
> > stores may well be complete but the data still in separate store
> queues.
> 
> POWER6 flushes the store queues when we take a checkpoint.  

Ok, that was missing from your description :-)

Cheers,
Ben.

Re: [PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

> On Thu, 2007-08-02 at 18:56 +1000, Michael Neuling wrote:
> > > > But even in the case of a checkpoint restart, the ordering will be
> > > > preserved as the NIA we get as part of the checkpoint will have
> > all
> > > > previous instructions complete and none of the following
> > instructions
> > > > started.
> > > 
> > > Instruction completion isn't enough to ensure storage ordering. The
> > > stores may well be complete but the data still in separate store
> > queues.
> > 
> > POWER6 flushes the store queues when we take a checkpoint.  
> 
> Ok, that was missing from your description :-)

Sorry... so ditch the barriers?

Mikey

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

On Thu, 2007-08-02 at 19:03 +1000, Michael Neuling wrote:
> > 
> > Ok, that was missing from your description :-)
> 
> Sorry... so ditch the barriers? 

As you like. The reason why you can ditch them is purely because you
know for sure that the only case the firmware will access those shadows
from another CPU is that one and it happens to be just right. What are
the chances that in the future, FW will do something different and
nobody will "fix" the code ?

Considering that eieios are fairly cheap and it's not a very hot code
path as far as I can tell, I'd rather keep them.

Cheers,
Ben.

Re: [PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

> On Thu, 2007-08-02 at 19:03 +1000, Michael Neuling wrote:
> > > 
> > > Ok, that was missing from your description :-)
> > 
> > Sorry... so ditch the barriers? 
> 
> As you like. The reason why you can ditch them is purely because you
> know for sure that the only case the firmware will access those shadows
> from another CPU is that one and it happens to be just right. What are
> the chances that in the future, FW will do something different and
> nobody will "fix" the code ?

Yep, I might chat to some PHYP guys and see what they think.

> Considering that eieios are fairly cheap and it's not a very hot code
> path as far as I can tell, I'd rather keep them.

OK.  I'll keep them in for now and submit an optimisation patch later if
need be.

Mikey

Re: [PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
for finding this.

Also added some write barriers to ensure the shadow buffer is always
valid.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
Integrated more comments from people.

 arch/powerpc/kernel/entry_64.S   |    3 +++
 arch/powerpc/mm/hash_utils_64.c  |    2 +-
 arch/powerpc/mm/slb.c            |   28 ++++++++++++++++++----------
 include/asm-powerpc/mmu-hash64.h |    1 +
 4 files changed, 23 insertions(+), 11 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/entry_64.S
+++ linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
@@ -389,8 +389,11 @@ BEGIN_FTR_SECTION
 	ld	r9,PACA_SLBSHADOWPTR(r13)
 	li	r12,0
 	std	r12,SLBSHADOW_STACKESID(r9) /* Clear ESID */
+	eieio
 	std	r7,SLBSHADOW_STACKVSID(r9)  /* Save VSID */
+	eieio
 	std	r0,SLBSHADOW_STACKESID(r9)  /* Save ESID */
+	eieio
 
 	slbie	r6
 	slbie	r6		/* Workaround POWER5 < DD2.1 issue */
Index: linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/mm/hash_utils_64.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
@@ -759,7 +759,7 @@ int hash_page(unsigned long ea, unsigned
 		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
 		get_paca()->vmalloc_sllp =
 			mmu_psize_defs[mmu_vmalloc_psize].sllp;
-		slb_flush_and_rebolt();
+		slb_vmalloc_update();
 	}
 #endif /* CONFIG_PPC_64K_PAGES */
 
Index: linux-2.6-ozlabs/arch/powerpc/mm/slb.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/mm/slb.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/slb.c
@@ -53,7 +53,8 @@ static inline unsigned long mk_vsid_data
 	return (get_kernel_vsid(ea) << SLB_VSID_SHIFT) | flags;
 }
 
-static inline void slb_shadow_update(unsigned long esid, unsigned long vsid,
+static inline void slb_shadow_update(unsigned long ea,
+				     unsigned long flags,
 				     unsigned long entry)
 {
 	/*
@@ -61,11 +62,11 @@ static inline void slb_shadow_update(uns
 	 * updating it.
 	 */
 	get_slb_shadow()->save_area[entry].esid = 0;
-	barrier();
-	get_slb_shadow()->save_area[entry].vsid = vsid;
-	barrier();
-	get_slb_shadow()->save_area[entry].esid = esid;
-
+	smp_wmb();
+	get_slb_shadow()->save_area[entry].vsid = mk_vsid_data(ea, flags);
+	smp_wmb();
+	get_slb_shadow()->save_area[entry].esid = mk_esid_data(ea, entry);
+	smp_wmb();
 }
 
 static inline void create_shadowed_slbe(unsigned long ea, unsigned long flags,
@@ -76,8 +77,7 @@ static inline void create_shadowed_slbe(
 	 * we don't get a stale entry here if we get preempted by PHYP
 	 * between these two statements.
 	 */
-	slb_shadow_update(mk_esid_data(ea, entry), mk_vsid_data(ea, flags),
-			  entry);
+	slb_shadow_update(ea, flags, entry);
 
 	asm volatile("slbmte  %0,%1" :
 		     : "r" (mk_vsid_data(ea, flags)),
@@ -104,8 +104,7 @@ void slb_flush_and_rebolt(void)
 		ksp_esid_data &= ~SLB_ESID_V;
 
 	/* Only third entry (stack) may change here so only resave that */
-	slb_shadow_update(ksp_esid_data,
-			  mk_vsid_data(ksp_esid_data, lflags), 2);
+	slb_shadow_update(get_paca()->kstack, lflags, 2);
 
 	/* We need to do this all in asm, so we're sure we don't touch
 	 * the stack between the slbia and rebolting it. */
@@ -123,6 +122,15 @@ void slb_flush_and_rebolt(void)
 		     : "memory");
 }
 
+void slb_vmalloc_update(void)
+{
+	unsigned long vflags;
+
+	vflags = SLB_VSID_KERNEL | mmu_psize_defs[mmu_vmalloc_psize].sllp;
+	slb_shadow_update(VMALLOC_START, vflags, 1);
+	slb_flush_and_rebolt();
+}
+
 /* Flush all user entries from the segment table of the current processor. */
 void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
 {
Index: linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
===================================================================
--- linux-2.6-ozlabs.orig/include/asm-powerpc/mmu-hash64.h
+++ linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
@@ -262,6 +262,7 @@ extern void slb_initialize(void);
 extern void slb_flush_and_rebolt(void);
 extern void stab_initialize(unsigned long stab);
 
+extern void slb_vmalloc_update(void);
 #endif /* __ASSEMBLY__ */
 
 /*

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

On Fri, 2007-08-03 at 11:55 +1000, Michael Neuling wrote:
> We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
> never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
> for finding this.
> 
> Also added some write barriers to ensure the shadow buffer is always
> valid.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
> Integrated more comments from people.
> 
>  arch/powerpc/kernel/entry_64.S   |    3 +++
>  arch/powerpc/mm/hash_utils_64.c  |    2 +-
>  arch/powerpc/mm/slb.c            |   28 ++++++++++++++++++----------
>  include/asm-powerpc/mmu-hash64.h |    1 +
>  4 files changed, 23 insertions(+), 11 deletions(-)
> 
> Index: linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
> ===================================================================
> --- linux-2.6-ozlabs.orig/arch/powerpc/kernel/entry_64.S
> +++ linux-2.6-ozlabs/arch/powerpc/kernel/entry_64.S
> @@ -389,8 +389,11 @@ BEGIN_FTR_SECTION
>  	ld	r9,PACA_SLBSHADOWPTR(r13)
>  	li	r12,0
>  	std	r12,SLBSHADOW_STACKESID(r9) /* Clear ESID */
> +	eieio
>  	std	r7,SLBSHADOW_STACKVSID(r9)  /* Save VSID */
> +	eieio
>  	std	r0,SLBSHADOW_STACKESID(r9)  /* Save ESID */
> +	eieio
>  
>  	slbie	r6
>  	slbie	r6		/* Workaround POWER5 < DD2.1 issue */
> Index: linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
> ===================================================================
> --- linux-2.6-ozlabs.orig/arch/powerpc/mm/hash_utils_64.c
> +++ linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
> @@ -759,7 +759,7 @@ int hash_page(unsigned long ea, unsigned
>  		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
>  		get_paca()->vmalloc_sllp =
>  			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> -		slb_flush_and_rebolt();
> +		slb_vmalloc_update();
>  	}
>  #endif /* CONFIG_PPC_64K_PAGES */
>  
> Index: linux-2.6-ozlabs/arch/powerpc/mm/slb.c
> ===================================================================
> --- linux-2.6-ozlabs.orig/arch/powerpc/mm/slb.c
> +++ linux-2.6-ozlabs/arch/powerpc/mm/slb.c
> @@ -53,7 +53,8 @@ static inline unsigned long mk_vsid_data
>  	return (get_kernel_vsid(ea) << SLB_VSID_SHIFT) | flags;
>  }
>  
> -static inline void slb_shadow_update(unsigned long esid, unsigned long vsid,
> +static inline void slb_shadow_update(unsigned long ea,
> +				     unsigned long flags,
>  				     unsigned long entry)
>  {
>  	/*
> @@ -61,11 +62,11 @@ static inline void slb_shadow_update(uns
>  	 * updating it.
>  	 */
>  	get_slb_shadow()->save_area[entry].esid = 0;
> -	barrier();
> -	get_slb_shadow()->save_area[entry].vsid = vsid;
> -	barrier();
> -	get_slb_shadow()->save_area[entry].esid = esid;
> -
> +	smp_wmb();
> +	get_slb_shadow()->save_area[entry].vsid = mk_vsid_data(ea, flags);
> +	smp_wmb();
> +	get_slb_shadow()->save_area[entry].esid = mk_esid_data(ea, entry);
> +	smp_wmb();
>  }
>  
>  static inline void create_shadowed_slbe(unsigned long ea, unsigned long flags,
> @@ -76,8 +77,7 @@ static inline void create_shadowed_slbe(
>  	 * we don't get a stale entry here if we get preempted by PHYP
>  	 * between these two statements.
>  	 */
> -	slb_shadow_update(mk_esid_data(ea, entry), mk_vsid_data(ea, flags),
> -			  entry);
> +	slb_shadow_update(ea, flags, entry);
>  
>  	asm volatile("slbmte  %0,%1" :
>  		     : "r" (mk_vsid_data(ea, flags)),
> @@ -104,8 +104,7 @@ void slb_flush_and_rebolt(void)
>  		ksp_esid_data &= ~SLB_ESID_V;
>  
>  	/* Only third entry (stack) may change here so only resave that */
> -	slb_shadow_update(ksp_esid_data,
> -			  mk_vsid_data(ksp_esid_data, lflags), 2);
> +	slb_shadow_update(get_paca()->kstack, lflags, 2);
>  
>  	/* We need to do this all in asm, so we're sure we don't touch
>  	 * the stack between the slbia and rebolting it. */
> @@ -123,6 +122,15 @@ void slb_flush_and_rebolt(void)
>  		     : "memory");
>  }
>  
> +void slb_vmalloc_update(void)
> +{
> +	unsigned long vflags;
> +
> +	vflags = SLB_VSID_KERNEL | mmu_psize_defs[mmu_vmalloc_psize].sllp;
> +	slb_shadow_update(VMALLOC_START, vflags, 1);
> +	slb_flush_and_rebolt();
> +}
> +
>  /* Flush all user entries from the segment table of the current processor. */
>  void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
>  {
> Index: linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
> ===================================================================
> --- linux-2.6-ozlabs.orig/include/asm-powerpc/mmu-hash64.h
> +++ linux-2.6-ozlabs/include/asm-powerpc/mmu-hash64.h
> @@ -262,6 +262,7 @@ extern void slb_initialize(void);
>  extern void slb_flush_and_rebolt(void);
>  extern void stab_initialize(unsigned long stab);
>  
> +extern void slb_vmalloc_update(void);
>  #endif /* __ASSEMBLY__ */
>  
>  /*

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

On Wed, 2007-08-01 at 16:02 +1000, Michael Neuling wrote:
> We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
> never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
> for finding this.
> 
> Also added some write barriers to ensure the shadow buffer is always
> valid.

The shadow is global or per-cpu ?

Because in the later case, I think you need more than that.

> Index: linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
> ===================================================================
> --- linux-2.6-ozlabs.orig/arch/powerpc/mm/hash_utils_64.c
> +++ linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
> @@ -629,6 +629,9 @@ int hash_page(unsigned long ea, unsigned
>  	cpumask_t tmp;
>  	int rc, user_region = 0, local = 0;
>  	int psize;
> +#ifdef CONFIG_PPC_64K_PAGES
> +	unsigned long vflags;
> +#endif
>  
>  	DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
>  		ea, access, trap);
> @@ -759,6 +762,9 @@ int hash_page(unsigned long ea, unsigned
>  		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
>  		get_paca()->vmalloc_sllp =
>  			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> +		vflags = SLB_VSID_KERNEL |
> +			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> +		slb_shadow_update(VMALLOC_START, vflags, 1);
>  		slb_flush_and_rebolt();
>  	}

Later on:

        } else if (get_paca()->vmalloc_sllp !=
                   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
                get_paca()->vmalloc_sllp =
                        mmu_psize_defs[mmu_vmalloc_psize].sllp;
                slb_flush_and_rebolt();
        }

If your shadow is per-cpu, you need to fix that up too.

I'm tempted to think you should just expose an slb_vmalloc_update()
from slb.c that does the shadow update and calls flush_and_rebolt.
That would also get rid of your ifdef on vflags definition (which
wasn't necessary in the first place if you had put it inside the
if statement anyway).

Ben.

Re: [PATCH] fixes for the SLB shadow buffer

[Michael Neuling]

> On Wed, 2007-08-01 at 16:02 +1000, Michael Neuling wrote:
> > We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
> > never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
> > for finding this.
> > 
> > Also added some write barriers to ensure the shadow buffer is always
> > valid.
> 
> The shadow is global or per-cpu ?
> 
> Because in the later case, I think you need more than that.

It's per CPU.

> > @@ -759,6 +762,9 @@ int hash_page(unsigned long ea, unsigned
> >  		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
> >  		get_paca()->vmalloc_sllp =
> >  			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> > +		vflags = SLB_VSID_KERNEL |
> > +			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> > +		slb_shadow_update(VMALLOC_START, vflags, 1);
> >  		slb_flush_and_rebolt();
> >  	}
> 
> Later on:
> 
>         } else if (get_paca()->vmalloc_sllp !=
>                    mmu_psize_defs[mmu_vmalloc_psize].sllp) {
>                 get_paca()->vmalloc_sllp =
>                         mmu_psize_defs[mmu_vmalloc_psize].sllp;
>                 slb_flush_and_rebolt();
>         }
> 
> If your shadow is per-cpu, you need to fix that up too.

I'm confused... isn't that the same section of code?

> I'm tempted to think you should just expose an slb_vmalloc_update()
> from slb.c that does the shadow update and calls flush_and_rebolt.
> That would also get rid of your ifdef on vflags definition (which
> wasn't necessary in the first place if you had put it inside the
> if statement anyway).

OK, I'll create an slb_vmalloc_update for the next rev.

Mikey

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

On Thu, 2007-08-02 at 09:32 +1000, Michael Neuling wrote:
> > On Wed, 2007-08-01 at 16:02 +1000, Michael Neuling wrote:
> > > We sometimes change the vmalloc segment in slb_flush_and_rebolt but we
> > > never updated with slb shadow buffer.  This fixes it.  Thanks to paulus
> > > for finding this.
> > > 
> > > Also added some write barriers to ensure the shadow buffer is always
> > > valid.
> > 
> > The shadow is global or per-cpu ?
> > 
> > Because in the later case, I think you need more than that.
> 
> It's per CPU.
> 
> > > @@ -759,6 +762,9 @@ int hash_page(unsigned long ea, unsigned
> > >  		   mmu_psize_defs[mmu_vmalloc_psize].sllp) {
> > >  		get_paca()->vmalloc_sllp =
> > >  			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> > > +		vflags = SLB_VSID_KERNEL |
> > > +			mmu_psize_defs[mmu_vmalloc_psize].sllp;
> > > +		slb_shadow_update(VMALLOC_START, vflags, 1);
> > >  		slb_flush_and_rebolt();
> > >  	}
> > 
> > Later on:
> > 
> >         } else if (get_paca()->vmalloc_sllp !=
> >                    mmu_psize_defs[mmu_vmalloc_psize].sllp) {
> >                 get_paca()->vmalloc_sllp =
> >                         mmu_psize_defs[mmu_vmalloc_psize].sllp;
> >                 slb_flush_and_rebolt();
> >         }
> > 
> > If your shadow is per-cpu, you need to fix that up too.
> 
> I'm confused... isn't that the same section of code?

The first block of code detects the need for a demotion and changes the
global mmu_vmalloc_psize, along with changing the value on the local CPU
(and flushing it's SLB). But other CPUs still have the old values.

The second block of code checks if the global value matches the per-CPU
value and if not, proceeds to a local SLB flush. That's how the "other"
CPUs get the new value.

If your shadow is per-CPU, it thus needs to be updated in both cases.

Ben.


Ben.

Re: [PATCH] fixes for the SLB shadow buffer

[Benjamin Herrenschmidt]

> The first block of code detects the need for a demotion and changes the
> global mmu_vmalloc_psize, along with changing the value on the local CPU
> (and flushing it's SLB). But other CPUs still have the old values.
> 
> The second block of code checks if the global value matches the per-CPU
> value and if not, proceeds to a local SLB flush. That's how the "other"
> CPUs get the new value.
> 
> If your shadow is per-CPU, it thus needs to be updated in both cases.

Hrm.. I must have been looking at older sources or lacked caffeine, we
indeed only do the paca update and the flush&rebolt once.

Ben.