Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[Grant Grundler]

On Sat, Feb 24, 2007 at 08:51:29AM -0600, James Bottomley wrote:
> Since we're now changing the %sr3 from an IPI, we have to be careful
> about other places in the kernel where we're using temporary values in %
> sr3.

Isn't sr3 part of the context?
ie if we need to change the sr3 for a given context shouldn't
the code be modifying the value in the struct context instead
the actual register unless we know we are in that context?
The IPI code should know the sr3 was saved and where.

The change to assembly.h has me very nervous since I don't understand it.
You probably have it right and I'm being a bit dense.

I was trying to track down other uses of "mtsp sr3" and schedule()
seems to be one:
	schedule() -> context_switch() -> switch_mm() -> load_context()

I'm not seeing where this path disables local interrupts.
Sounds like it should before calling load_context() or we should
in our switch_mm() or add a prepare_arch_switch() to asm-parisc/system.h.

>   As far as I can tell, this is pretty much only non local cache
> flushing.

ISTR copy and one or two other places use sr3 as well.

> The following patch fixes the IPI to work (by not saving %sr3
> across an interruption) and patches up temporary %sr3 usage.
> 
> The sharp eyed will also notice I've corrected a Protection ID bug with
> the non current flushes.

That's not me :)
Was the fix to place "pgd = mfctl(25)" after the local_irq_disable()?

thanks,
grant

> 
> Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
> 
> diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
> index 00b1641..cb1dd17 100644
> --- a/arch/parisc/kernel/cache.c
> +++ b/arch/parisc/kernel/cache.c
> @@ -303,25 +303,28 @@ flush_user_cache_page_non_current(struct vm_area_struct *vma,
>  				  unsigned long vmaddr)
>  {
>  	/* save the current process space and pgd */
> -	unsigned long space = mfsp(3), pgd = mfctl(25);
> +	unsigned long space, pgd;
>  
> -	/* we don't mind taking interrups since they may not
> -	 * do anything with user space, but we can't
> -	 * be preempted here */
> -	preempt_disable();
> +	/* Have to disable interrupts here, since now %sr3 changes
> +	 * are carried by IPI and we can't have that happen while
> +	 * we're using a temporary %sr3 */
> +	local_irq_disable();
> +
> +	space = mfsp(3);
> +	pgd = mfctl(25);
>  
>  	/* make us current */
>  	mtctl(__pa(vma->vm_mm->pgd), 25);
> -	mtsp(vma->vm_mm->context, 3);
> +	load_context(vma->vm_mm->context);
>  
>  	flush_user_dcache_page(vmaddr);
>  	if(vma->vm_flags & VM_EXEC)
>  		flush_user_icache_page(vmaddr);
>  
>  	/* put the old current process back */
> -	mtsp(space, 3);
> +	load_context(space);
>  	mtctl(pgd, 25);
> -	preempt_enable();
> +	local_irq_enable();
>  }
>  
>  
> diff --git a/include/asm-parisc/assembly.h b/include/asm-parisc/assembly.h
> index 5587f00..efe9ebc 100644
> --- a/include/asm-parisc/assembly.h
> +++ b/include/asm-parisc/assembly.h
> @@ -435,7 +435,8 @@
>  	SAVE_SP  (%sr0, PT_SR0 (\regs))
>  	SAVE_SP  (%sr1, PT_SR1 (\regs))
>  	SAVE_SP  (%sr2, PT_SR2 (\regs))
> -	SAVE_SP  (%sr3, PT_SR3 (\regs))
> +; Can't save and restore %sr3, it may be update from IPI
> +;	SAVE_SP  (%sr3, PT_SR3 (\regs))
>  	SAVE_SP  (%sr4, PT_SR4 (\regs))
>  	SAVE_SP  (%sr5, PT_SR5 (\regs))
>  	SAVE_SP  (%sr6, PT_SR6 (\regs))
> @@ -475,7 +476,8 @@
>  	REST_SP  (%sr0, PT_SR0 (\regs))
>  	REST_SP  (%sr1, PT_SR1 (\regs))
>  	REST_SP  (%sr2, PT_SR2 (\regs))
> -	REST_SP  (%sr3, PT_SR3 (\regs))
> +; Can't save and restore %sr3, it may be updated from IPI
> +;	REST_SP  (%sr3, PT_SR3 (\regs))
>  	REST_SP  (%sr4, PT_SR4 (\regs))
>  	REST_SP  (%sr5, PT_SR5 (\regs))
>  	REST_SP  (%sr6, PT_SR6 (\regs))
> 
> 
> _______________________________________________
> parisc-linux mailing list
> parisc-linux@lists.parisc-linux.org
> http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[James Bottomley]

On Sat, 2007-02-24 at 17:46 -0700, Grant Grundler wrote:
> Isn't sr3 part of the context?

It is the user context above the kernel, yes.

> ie if we need to change the sr3 for a given context shouldn't
> the code be modifying the value in the struct context instead

That's what tausq's change does.  It updates active_mm->context but then
it has to propagate the change on an SMP system, which is what the IPI
is supposed to do.

> the actual register unless we know we are in that context?

That's why the IPI checks current->active_mm against the mm being
changed.  If the CPU isn't executing in that context, nothing happens.
If it has, since we just changed mm->context, the new context gets
loaded into sr3, as it should be.

> The IPI code should know the sr3 was saved and where.

It does: in mm->context.

> The change to assembly.h has me very nervous since I don't understand it.
> You probably have it right and I'm being a bit dense.

Actually, I think it's pointless saving any space register for an
interrupt ... no interrupt should muck with the space registers since no
interrupt should ever be mucking with anything in user context ...
However, sr3 is special ... even if we get interrupts mucking with space
registers, they'll never touch sr3.

> I was trying to track down other uses of "mtsp sr3" and schedule()
> seems to be one:
> 	schedule() -> context_switch() -> switch_mm() -> load_context()
> 
> I'm not seeing where this path disables local interrupts.

It doesn't need to.  The IPI either updates sr3 or it doesn't.  The
load_context() will actually update it anyway, and sr3 isn't used until
the kernel accesses userspace, which is nowhere in that sequence, so
it's all quite safe.

> Sounds like it should before calling load_context() or we should
> in our switch_mm() or add a prepare_arch_switch() to asm-parisc/system.h.

Not really ... like I said, the IPI's effect will be known even if we're
beginning a context switch.

> >   As far as I can tell, this is pretty much only non local cache
> > flushing.
> 
> ISTR copy and one or two other places use sr3 as well.

No, pa_memcpy uses sr1 and sr2 ... it's only the non-local flushing that
uses sr3.

> > The following patch fixes the IPI to work (by not saving %sr3
> > across an interruption) and patches up temporary %sr3 usage.
> > 
> > The sharp eyed will also notice I've corrected a Protection ID bug with
> > the non current flushes.
> 
> That's not me :)
> Was the fix to place "pgd = mfctl(25)" after the local_irq_disable()?

No, it was changing the mtsp(xx, 3) to load_context(xx).  The problem is
that if we don't update %cr8 then the new context in sr3 is completely
ineffective since the protection IDs will mismatch in the TLBs, so, as
far as I can tell, the non local flush was never effective.

James


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[James Bottomley]

Make sure we alter the context on the local CPU, but all possible uses
of the deprecated space on the other cpus.

The sharp eyed will also notice I've corrected a Protection ID bug with
the non current flushes.


Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

---

Here's take II.  It corrects the problem with the prior one not being
agressive enough .. plus sr3 is the wrong register to alter from
userspace ... we really need to do sr4-7.

James

diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index 00b1641..f8d4d92 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -18,6 +18,8 @@
 #include <linux/module.h>
 #include <linux/seq_file.h>
 #include <linux/pagemap.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 
 #include <asm/pdc.h>
 #include <asm/cache.h>
@@ -303,25 +305,28 @@ flush_user_cache_page_non_current(struct vm_area_struct *vma,
 				  unsigned long vmaddr)
 {
 	/* save the current process space and pgd */
-	unsigned long space = mfsp(3), pgd = mfctl(25);
+	unsigned long space, pgd;
 
-	/* we don't mind taking interrups since they may not
-	 * do anything with user space, but we can't
-	 * be preempted here */
-	preempt_disable();
+	/* Have to disable interrupts here, since now %sr3 changes
+	 * are carried by IPI and we can't have that happen while
+	 * we're using a temporary %sr3 */
+	local_irq_disable();
+
+	space = mfsp(3);
+	pgd = mfctl(25);
 
 	/* make us current */
 	mtctl(__pa(vma->vm_mm->pgd), 25);
-	mtsp(vma->vm_mm->context, 3);
+	load_context(vma->vm_mm->context);
 
 	flush_user_dcache_page(vmaddr);
 	if(vma->vm_flags & VM_EXEC)
 		flush_user_icache_page(vmaddr);
 
 	/* put the old current process back */
-	mtsp(space, 3);
+	load_context(space);
 	mtctl(pgd, 25);
-	preempt_enable();
+	local_irq_enable();
 }
 
 
@@ -574,3 +579,34 @@ flush_cache_page(struct vm_area_struct *vma, unsigned long vmaddr, unsigned long
 		__flush_cache_page(vma, vmaddr);
 
 }
+
+void __flush_tlb_mm(void *mmv)
+{
+	struct mm_struct *mm = (struct mm_struct *)mmv;
+	struct pt_regs *regs = get_irq_regs();
+	unsigned long sr3;
+	int i;
+
+	if (mm != current->active_mm)
+		return;
+
+	/* sr3 is alterable by the user, so if we trapped from
+	 * user space, check sr7 instead */
+	if (in_interrupt()) {
+		if (regs->sr[7] != 0)
+			sr3 = regs->sr[7];
+		else
+			sr3 = mfsp(3);
+		/* needed to set up protection ID */
+		load_context(mm->context);
+	} else {
+		/* on_each_cpu makes one non interrupt call,
+		 * this is it */
+		load_context(mm->context);
+		return;
+	}
+
+	for (i = 0; i < 8; i++)
+		if (regs->sr[i] == sr3)
+			regs->sr[i] = mm->context;
+}
diff --git a/include/asm-parisc/assembly.h b/include/asm-parisc/assembly.h
diff --git a/include/asm-parisc/tlbflush.h b/include/asm-parisc/tlbflush.h
index 2e8c2bd..45d04ce 100644
--- a/include/asm-parisc/tlbflush.h
+++ b/include/asm-parisc/tlbflush.h
@@ -39,12 +39,7 @@ extern void flush_tlb_all_local(void *);
  * etc. do not do that).
  */
 
-static inline void __flush_tlb_mm(void *mmv)
-{
-	struct mm_struct *mm = (struct mm_struct *)mmv;
-	if (mm == current->active_mm)
-		load_context(mm->context);
-}
+void __flush_tlb_mm(void *mmv);
 
 static inline void flush_tlb_mm(struct mm_struct *mm)
 {


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[John David Anglin]

> > > Don't like what's going on with fr22 in the above code.  Seems like a
> > > GCC optimization bug since it looks like there are a few general registers
> > > that could be used.
> > 
> > Why a bug? It's a register just like any other.
> 
> It's an optimization bug because copying from a general to a floating
> register has to be done through memory.  A stack local would save
> a read and write.  In this case, all the manipulation is done before
> the syscall and there would appear to be sufficient general registers
> available to do the job.  GCC's register allocator should be smart
> enough to figure this out...

With luck, his problem should be fixed:
http://gcc.gnu.org/ml/gcc-patches/2007-03/msg00335.html

Also, the 64-bit kernel should use fp regs only for xmpyu.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[James Bottomley]

On Sun, 2007-02-25 at 19:33 -0500, John David Anglin wrote:
> This one is less stable your previous change + reversion of tausq's
> SMP TLB opimization patch.  GCC build doesn't get far before wierdness
> occurs (strange characters being read from files):
> 
> gcc -c -g -fkeep-inline-functions      -gnatpg -gnata -I- -I. -Iada -I../../gcc/
> gcc/ada ../../gcc/gcc/ada/exp_ch6.adb -o ada/exp_ch6.o
> /tmp/ccaWBgD1.s: Assembler messages:
> /tmp/ccaWBgD1.s:2436: Error: Undefined register: '%s'.
> /tmp/ccaWBgD1.s:2436: Error: Field out of range [0..31] (-1).
> /tmp/ccaWBgD1.s:2436: Error: Undefined register: '%s'.
> /tmp/ccaWBgD1.s:2436: Error: Undefined register: '%s'.
> /tmp/ccaWBgD1.s:2436: Error: Field out of range [0..31] (-1).
> ...

Hmm ... perhaps there's a reason every other architecture's SMP
flush_tlb_mm() implementation is much more complicated than ours ...

James


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[John David Anglin]

> Here's take II.  It corrects the problem with the prior one not being
> agressive enough .. plus sr3 is the wrong register to alter from
> userspace ... we really need to do sr4-7.

This one is less stable your previous change + reversion of tausq's
SMP TLB opimization patch.  GCC build doesn't get far before wierdness
occurs (strange characters being read from files):

gcc -c -g -fkeep-inline-functions      -gnatpg -gnata -I- -I. -Iada -I../../gcc/
gcc/ada ../../gcc/gcc/ada/exp_ch6.adb -o ada/exp_ch6.o
/tmp/ccaWBgD1.s: Assembler messages:
/tmp/ccaWBgD1.s:2436: Error: Undefined register: '%s'.
/tmp/ccaWBgD1.s:2436: Error: Field out of range [0..31] (-1).
/tmp/ccaWBgD1.s:2436: Error: Undefined register: '%s'.
/tmp/ccaWBgD1.s:2436: Error: Undefined register: '%s'.
/tmp/ccaWBgD1.s:2436: Error: Field out of range [0..31] (-1).
...

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[John David Anglin]

> Because this is non-PIC code the save/restore of r19 has been turned
> into a NOP.

Why a NOP?  Wouldn't a newline be ok?

> > It could be replaced by a mtsp instruction:
> >
> >         mtsp %r0,%sr2
> >
> > This would ensure that sr2 is always set correctly for the branch to
> > the gateway page.
> 
> ... only for people with a recent enough glibc.

Sure!  However, I thought I saw people requesting new syscalls, etc ;)

> > Don't like what's going on with fr22 in the above code.  Seems like a
> > GCC optimization bug since it looks like there are a few general registers
> > that could be used.
> 
> Why a bug? It's a register just like any other.

It's an optimization bug because copying from a general to a floating
register has to be done through memory.  A stack local would save
a read and write.  In this case, all the manipulation is done before
the syscall and there would appear to be sufficient general registers
available to do the job.  GCC's register allocator should be smart
enough to figure this out...

I haven't studied this in detail but GCC on the PA allocates floating
registers first.  It's claimed in the code that this results in better
code.  This allocation scheme was probably based on work done at Utah
in the early 90's.  So, things may have changed.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[John David Anglin]

> > 	ldsid (%r1),%r31
> > 	mtps %r31,%sr3
> > 	be 0(%sr3,%r1)
> > 
> > should work in linux even if it's not efficient.
> 
> I think you'll find that unless %sr3 is zero or your current space,
> you'll get a protection trap (and if it's zero, you'll likely get a
> privilege trap).

At the moment, the following little bit code seems to work
on a UP system:

0x00010444 <main+0>:    ldil L%10000,r1
0x00010448 <main+4>:    ldo 444(r1),r1
0x0001044c <main+8>:    ldsid (r1),r31
0x00010450 <main+12>:   mtsp r31,sr3
0x00010454 <main+16>:   be 0(sr3,r1)
0x00010458 <main+20>:   nop
0x0001045c <main+24>:   bv r0(rp)
0x00010460 <main+28>:   ldi 0,ret0

Of course, sr3 should be the current space.  Whether it is or not is
an interesting question as gdb doesn't seem to be able to figure out
what the space registers contain.  When I run the above under gdb,
it reports that all the space registers are 0.  If I run the program
and signal an abort, I see the following in the core dump:

sr4            0xb3     179
sr0            0x0      0
sr1            0xb3     179
sr2            0x0      0
sr3            0xffff   65535
sr5            0xb3     179
sr6            0xb3     179
sr7            0xb3     179

This is with your patch installed.

> The user space convention on linux says that sr0,1,3-7 should contain
> the current space and sr2 contains zero for gateway operations.
> Theoretically, the user can stash values in sr0-3 as long as they're
> never used; practically, nothing does this.

I can think of a use.  There are certain situation in gcc where
we need to generate a long branch.  We need r1 for this but it
may be live.  So, we stash it in the frame marker and restore
it in the delay slot of the branch.  It might be better to use
a space register to save r1.

I don't think the linux user space convention regarding space
registers and calling conventions is documented.

Regarding the requirement that sr2 always contain zero for
gateway operations, I think this is unnecessary.  For example,
I see the following in libc.a:

00000000 <syscall>:
   0:   0f d9 12 81     stw r25,-10(sp)
   4:   27 c1 10 16     fldw -10(sp),fr22
   8:   6b c2 3f d9     stw rp,-14(sp)
   c:   08 18 02 59     copy r24,r25
  10:   6f c3 00 80     stw,ma r3,40(sp)
  14:   08 17 02 58     copy r23,r24
  18:   08 1a 02 5c     copy r26,ret0
  1c:   4b d5 3f 09     ldw -7c(sp),r21
  20:   4b d6 3f 11     ldw -78(sp),r22
  24:   4b d7 3f 19     ldw -74(sp),r23
  28:   27 c1 12 16     fstw fr22,-10(sp)
  2c:   0f c1 10 9a     ldw -10(sp),r26
  30:   08 00 02 40     nop
  34:   e4 00 82 00     be,l 100(sr2,r0),sr0,r31
  38:   08 1c 02 54     copy ret0,r20

The branch to the gateway page always seems to be preceded by a nop.
It could be replaced by a mtsp instruction:

	mtsp %r0,%sr2

This would ensure that sr2 is always set correctly for the branch to
the gateway page.

Personally, I would like to see sr0 through sr3 available for general
use.  I would also sr4 through sr7 to be stable (i.e., not change during
the lifetime of an application).  Obviously, sr0 through sr3 would
be call clobbered.

Don't like what's going on with fr22 in the above code.  Seems like a
GCC optimization bug since it looks like there are a few general registers
that could be used.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[James Bottomley]

On Sun, 2007-02-25 at 12:04 -0500, John David Anglin wrote:
> > On Sat, 2007-02-24 at 23:27 -0500, John David Anglin wrote:
> > > You have to watch out for "be,l" which mucks with sr0.
> > 
> > But practically, the kernel's not going to link to a new space, and the
> > user can't.
> 
> >From a hardware standpoint, the user can change sr0 through sr4.
> Changing sr4 in user space isn't going to work.  However,
> in the 32-bit hpux runtime, the user is free to change sr0 through
> sr3 in whatever way they want.  At least at one time, there
> was a desire to provide hpux runtime compatibility.  So, a branch
> sequence like the following
> 
> 	ldsid (%r1),%r31
> 	mtps %r31,%sr3
> 	be 0(%sr3,%r1)
> 
> should work in linux even if it's not efficient.

I think you'll find that unless %sr3 is zero or your current space,
you'll get a protection trap (and if it's zero, you'll likely get a
privilege trap).

Linux doesn't use spaces like HPUX, which is why hpux compatibility at
this level just won't work.  In Linux, every user process runs in a
different, isolated space.

>   I picked sr3
> in the example since it's no longer saved on interrupts with your
> patch.  The branch will work if the kernel restores on interrupt
> return the value used in sr4 through sr7.  However, then the user
> can't use sr3 for any other purpose.
> 
> > But the point isn't that we might change them ... it's that by
> > convention we always know what their values should be, so we didn't
> > really need to save them (except sr1 and sr2 from the kernel).  And
> > actually, that's a potential (but I suspect never seen) bug in that this
> > code doesn't set the space registers aggressively enough for a trap from
> > user context.
> 
> I don't see how the kernel can know what should be in sr0 through sr3,
> and I think the user can use these registers for any purpose in the
> 32-bit runtime.  In wide mode, things are different as the OS is allowed
> to change the space registers at any time (see note for ldsid insn).

The user space convention on linux says that sr0,1,3-7 should contain
the current space and sr2 contains zero for gateway operations.
Theoretically, the user can stash values in sr0-3 as long as they're
never used; practically, nothing does this.

However, this is pretty much moot ... the more agressive space shifting
code will have to loop over all of the space registers in the context,
so it will be saving and restoring them all.

James

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[John David Anglin]

> On Sat, 2007-02-24 at 23:27 -0500, John David Anglin wrote:
> > You have to watch out for "be,l" which mucks with sr0.
> 
> But practically, the kernel's not going to link to a new space, and the
> user can't.

>>>From a hardware standpoint, the user can change sr0 through sr4.
Changing sr4 in user space isn't going to work.  However,
in the 32-bit hpux runtime, the user is free to change sr0 through
sr3 in whatever way they want.  At least at one time, there
was a desire to provide hpux runtime compatibility.  So, a branch
sequence like the following

	ldsid (%r1),%r31
	mtps %r31,%sr3
	be 0(%sr3,%r1)

should work in linux even if it's not efficient.  I picked sr3
in the example since it's no longer saved on interrupts with your
patch.  The branch will work if the kernel restores on interrupt
return the value used in sr4 through sr7.  However, then the user
can't use sr3 for any other purpose.

> But the point isn't that we might change them ... it's that by
> convention we always know what their values should be, so we didn't
> really need to save them (except sr1 and sr2 from the kernel).  And
> actually, that's a potential (but I suspect never seen) bug in that this
> code doesn't set the space registers aggressively enough for a trap from
> user context.

I don't see how the kernel can know what should be in sr0 through sr3,
and I think the user can use these registers for any purpose in the
32-bit runtime.  In wide mode, things are different as the OS is allowed
to change the space registers at any time (see note for ldsid insn).

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[James Bottomley]

On Sat, 2007-02-24 at 23:27 -0500, John David Anglin wrote:
> You have to watch out for "be,l" which mucks with sr0.

But practically, the kernel's not going to link to a new space, and the
user can't.

But the point isn't that we might change them ... it's that by
convention we always know what their values should be, so we didn't
really need to save them (except sr1 and sr2 from the kernel).  And
actually, that's a potential (but I suspect never seen) bug in that this
code doesn't set the space registers aggressively enough for a trap from
user context.

James


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] fix SMP TLB optimisations

[John David Anglin]

> Actually, I think it's pointless saving any space register for an
> interrupt ... no interrupt should muck with the space registers since no
> interrupt should ever be mucking with anything in user context ...

You have to watch out for "be,l" which mucks with sr0.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] [PATCH] fix SMP TLB optimisations

[James Bottomley]

Since we're now changing the %sr3 from an IPI, we have to be careful
about other places in the kernel where we're using temporary values in %
sr3.  As far as I can tell, this is pretty much only non local cache
flushing.  The following patch fixes the IPI to work (by not saving %sr3
across an interruption) and patches up temporary %sr3 usage.

The sharp eyed will also notice I've corrected a Protection ID bug with
the non current flushes.

Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index 00b1641..cb1dd17 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -303,25 +303,28 @@ flush_user_cache_page_non_current(struct vm_area_struct *vma,
 				  unsigned long vmaddr)
 {
 	/* save the current process space and pgd */
-	unsigned long space = mfsp(3), pgd = mfctl(25);
+	unsigned long space, pgd;
 
-	/* we don't mind taking interrups since they may not
-	 * do anything with user space, but we can't
-	 * be preempted here */
-	preempt_disable();
+	/* Have to disable interrupts here, since now %sr3 changes
+	 * are carried by IPI and we can't have that happen while
+	 * we're using a temporary %sr3 */
+	local_irq_disable();
+
+	space = mfsp(3);
+	pgd = mfctl(25);
 
 	/* make us current */
 	mtctl(__pa(vma->vm_mm->pgd), 25);
-	mtsp(vma->vm_mm->context, 3);
+	load_context(vma->vm_mm->context);
 
 	flush_user_dcache_page(vmaddr);
 	if(vma->vm_flags & VM_EXEC)
 		flush_user_icache_page(vmaddr);
 
 	/* put the old current process back */
-	mtsp(space, 3);
+	load_context(space);
 	mtctl(pgd, 25);
-	preempt_enable();
+	local_irq_enable();
 }
 
 
diff --git a/include/asm-parisc/assembly.h b/include/asm-parisc/assembly.h
index 5587f00..efe9ebc 100644
--- a/include/asm-parisc/assembly.h
+++ b/include/asm-parisc/assembly.h
@@ -435,7 +435,8 @@
 	SAVE_SP  (%sr0, PT_SR0 (\regs))
 	SAVE_SP  (%sr1, PT_SR1 (\regs))
 	SAVE_SP  (%sr2, PT_SR2 (\regs))
-	SAVE_SP  (%sr3, PT_SR3 (\regs))
+; Can't save and restore %sr3, it may be update from IPI
+;	SAVE_SP  (%sr3, PT_SR3 (\regs))
 	SAVE_SP  (%sr4, PT_SR4 (\regs))
 	SAVE_SP  (%sr5, PT_SR5 (\regs))
 	SAVE_SP  (%sr6, PT_SR6 (\regs))
@@ -475,7 +476,8 @@
 	REST_SP  (%sr0, PT_SR0 (\regs))
 	REST_SP  (%sr1, PT_SR1 (\regs))
 	REST_SP  (%sr2, PT_SR2 (\regs))
-	REST_SP  (%sr3, PT_SR3 (\regs))
+; Can't save and restore %sr3, it may be updated from IPI
+;	REST_SP  (%sr3, PT_SR3 (\regs))
 	REST_SP  (%sr4, PT_SR4 (\regs))
 	REST_SP  (%sr5, PT_SR5 (\regs))
 	REST_SP  (%sr6, PT_SR6 (\regs))


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux