Re: [PATCH] fixes for the SLB shadow buffer
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 <[EMAIL PROTECTED]>
Acked-by: Benjamin Herrenschmidt <[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
> ===
> --- 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_ini
Re: [PATCH] fixes for the SLB shadow buffer
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
===
--- 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__ */
/*
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
> > > > 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
> > --- 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
> 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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
> 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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] fixes for the SLB shadow buffer
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] fixes for the SLB shadow buffer
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]>
---
> > + 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,
Re: [PATCH] fixes for the SLB shadow buffer
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] fixes for the SLB shadow buffer
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]>
---
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__ */
/*
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
