fixes for the SLB shadow buffer

Review

Quoted 2 lines

(quoted diff) +2 -0

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

arch/powerpc/kernel/entry_64.S BEGIN_FTR_SECTION · line 396

Quoted 11 lines

arch/powerpc/kernel/entry_64.S +2 -0

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

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

Quoted 5 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

previous instructions complete and none of the following

instructions

Quoted 4 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

previous instructions complete and none of the following

instructions

Quoted 2 lines

Quoted 1 line

started.

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

Ok, that was missing from your description :-)

Sorry... so ditch the barriers?

Quoted 4 lines

Quoted 2 lines

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.

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

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

arch/powerpc/kernel/entry_64.S +3 -0

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

arch/powerpc/mm/hash_utils_64.c +1 -1

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

arch/powerpc/mm/slb.c +18 -10

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

include/asm-powerpc/mmu-hash64.h +1 -0

@@ -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__ */
 
 /*

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.

Index: linux-2.6-ozlabs/arch/powerpc/mm/hash_utils_64.c
===================================================================

arch/powerpc/mm/hash_utils_64.c +3 -0

@@ -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);

Quoted 6 lines

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.

arch/powerpc/mm/hash_utils_64.c int hash_page(unsigned long ea, unsigned · line 769

Quoted 9 lines

arch/powerpc/mm/hash_utils_64.c +3 -0

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

Quoted 11 lines · Michael Neuling

Quoted 6 lines

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.

Quoted 19 lines

arch/powerpc/mm/hash_utils_64.c int hash_page(unsigned long ea, unsigned · line 769

Quoted 9 lines

arch/powerpc/mm/hash_utils_64.c +3 -0

@@ -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();
        }

Quoted 1 line

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.