Bug in the _save_fp_context.

Bug in the _save_fp_context.

[Carsten Langgaard]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

I posted the following problem on the list almost a year ago, but it
still hasn't made into the SGI CVS.
I think there is a bug in the _save_fp_context function in
arch/mips/kernel/r4k_fpu.S

The problem is the following piece of code:

 jr ra
 .set nomacro
 EX(sw t0,SC_FPC_EIR(a0))
 .set macro

We do not handle entries in the __ex_table which is located in a branch
delay.
So we need to handle the situation where we take a page fault on an
instruction which is located in a brach delay slot, or we don't put the
"potential" faulting instruction in a delay slot.

This situation probably doesn't generally hit people since it hasn't
been fix yet, but if you try run something nasty like Crashme, you will
get hit by this problem.
I suggest the following patch.

/Carsten


--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com



[-- Attachment #2: r4k_fpu.S.patch --]
[-- Type: text/plain, Size: 563 bytes --]

Index: arch/mips/kernel/r4k_fpu.S
===================================================================
RCS file: /cvs/linux/arch/mips/kernel/r4k_fpu.S,v
retrieving revision 1.12
diff -u -r1.12 r4k_fpu.S
--- arch/mips/kernel/r4k_fpu.S	2001/04/11 05:19:46	1.12
+++ arch/mips/kernel/r4k_fpu.S	2002/01/17 14:21:09
@@ -50,11 +50,10 @@
 	EX(sdc1	$f30,(SC_FPREGS+240)(a0))
 	EX(sw	t1,SC_FPC_CSR(a0))
 	cfc1	t0,$0				# implementation/version
+	EX(sw	t0,SC_FPC_EIR(a0))
 
 	jr	ra
-	.set	nomacro
-	 EX(sw	t0,SC_FPC_EIR(a0))
-	.set	macro
+	 nop
 	END(_save_fp_context)
 
 /*

Re: Bug in the _save_fp_context.

[Carsten Langgaard]

"H . J . Lu" wrote:

> On Thu, Jan 17, 2002 at 02:34:37PM +0100, Carsten Langgaard wrote:
> > I posted the following problem on the list almost a year ago, but it
> > still hasn't made into the SGI CVS.
> > I think there is a bug in the _save_fp_context function in
> > arch/mips/kernel/r4k_fpu.S
> >
> > The problem is the following piece of code:
> >
> >  jr ra
> >  .set nomacro
> >  EX(sw t0,SC_FPC_EIR(a0))
> >  .set macro
> >
> > We do not handle entries in the __ex_table which is located in a branch
> > delay.
> > So we need to handle the situation where we take a page fault on an
> > instruction which is located in a brach delay slot, or we don't put the
> > "potential" faulting instruction in a delay slot.
> >
> > This situation probably doesn't generally hit people since it hasn't
> > been fix yet, but if you try run something nasty like Crashme, you will
> > get hit by this problem.
> > I suggest the following patch.
> >
> > /Carsten
> >
> >
> > --
> > _    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
> > |\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
> > | \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
> >   TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
> >                    Denmark             http://www.mips.com
> >
> >
>
> > Index: arch/mips/kernel/r4k_fpu.S
> > ===================================================================
> > RCS file: /cvs/linux/arch/mips/kernel/r4k_fpu.S,v
> > retrieving revision 1.12
> > diff -u -r1.12 r4k_fpu.S
> > --- arch/mips/kernel/r4k_fpu.S        2001/04/11 05:19:46     1.12
> > +++ arch/mips/kernel/r4k_fpu.S        2002/01/17 14:21:09
> > @@ -50,11 +50,10 @@
> >       EX(sdc1 $f30,(SC_FPREGS+240)(a0))
> >       EX(sw   t1,SC_FPC_CSR(a0))
> >       cfc1    t0,$0                           # implementation/version
> > +     EX(sw   t0,SC_FPC_EIR(a0))
> >
> >       jr      ra
> > -     .set    nomacro
> > -      EX(sw  t0,SC_FPC_EIR(a0))
> > -     .set    macro
> > +      nop
> >       END(_save_fp_context)
> >
> >  /*
>
> Do we still need this patch for 2.4 on OSS?
>
> H.J.

No, this fix is not needed any more, as we now handle entries in the
__ex_table which is located in a branch delay.

Bug in the _save_fp_context.

[Carsten Langgaard]

I think there is a bug in the _save_fp_context function in
arch/mips/kernel/r4k_fpu.S

The problem is the following piece of code:

 jr ra
 .set nomacro
 EX(sw t0,SC_FPC_EIR(a0))
 nop
 .set macro

First of all what should the ".set nomacro" do?
If it means that the EX macro shouldn't be used then this entry wouldn't
get into __ex_table, which would be wrong.
But it look like it uses the macro anyway, regardless of the ".set
nomacro", at least with the compiler I use.
Never the less we do not handle entries in the __ex_table which is
located in a branch delay.
So we need to handle the situation where we take a page fault on an
instruction which is located in a brach delay slot, or we don't put the
"potential" faulting instruction in a delay slot.

Any ideas, how we should handle this in a nice and clean way?

/Carsten

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

Re: Bug in the _save_fp_context.

[Kevin D. Kissell]

> I think there is a bug in the _save_fp_context function in
> arch/mips/kernel/r4k_fpu.S
> 
> The problem is the following piece of code:
> 
>  jr ra
>  .set nomacro
>  EX(sw t0,SC_FPC_EIR(a0))
>  nop
>  .set macro
> 
> First of all what should the ".set nomacro" do?
> If it means that the EX macro shouldn't be used then this entry wouldn't
> get into __ex_table, which would be wrong.
> But it look like it uses the macro anyway, regardless of the ".set
> nomacro", at least with the compiler I use.

Not surprising, really.  "EX" is presumably a cpp macro
that gets expanded by gcc from the .S file, based on
some include file.  .set directives affect only the assembler,
and would inhibit assembler-level macros only.  I'm not
sure just what the definition of an assembler macro
would be - it may or may not include pseudo-instructions
like "la" or "li 32_bit_constant".  I *think* that what the
author was trying to do here was to ensure that the
"sw" instruction in the EX expansion was really and
truly a single instruction.

> Never the less we do not handle entries in the __ex_table which is
> located in a branch delay.
> So we need to handle the situation where we take a page fault on an
> instruction which is located in a brach delay slot, or we don't put the
> "potential" faulting instruction in a delay slot.
> 
> Any ideas, how we should handle this in a nice and clean way?

Is the __ex_table really ending up in the delay slot?
Just looking at the source, I have the impression
that the "sw t0,..." instruction should be in the delay
slot, followed by the __ex_table.

On another topic, now that I've patched the kernel to
turn off the stupid stuck interrupt on my Malta board,
I've realized that I can't just connect my old Atlas SCSI
disk.  I'm torn between ordering a Tekram 390 PCI
SCSI card, which should be able to use our "MIPS
safe" NCR driver as-is (I hope) and buying an IDE
disk and going through the network install ritual.
Which do you recommend?  One thing I really never
knew was just what kernel config options I need to
select to build a kernel that can do the NFS-root
bootstrap.  Can you help me there?

            Regards,

            Kevin K.

Re: Bug in the _save_fp_context.

[Kevin D. Kissell]

> I think there is a bug in the _save_fp_context function in
> arch/mips/kernel/r4k_fpu.S
> 
> The problem is the following piece of code:
> 
>  jr ra
>  .set nomacro
>  EX(sw t0,SC_FPC_EIR(a0))
>  nop
>  .set macro
> 
> First of all what should the ".set nomacro" do?
> If it means that the EX macro shouldn't be used then this entry wouldn't
> get into __ex_table, which would be wrong.
> But it look like it uses the macro anyway, regardless of the ".set
> nomacro", at least with the compiler I use.

Not surprising, really.  "EX" is presumably a cpp macro
that gets expanded by gcc from the .S file, based on
some include file.  .set directives affect only the assembler,
and would inhibit assembler-level macros only.  I'm not
sure just what the definition of an assembler macro
would be - it may or may not include pseudo-instructions
like "la" or "li 32_bit_constant".  I *think* that what the
author was trying to do here was to ensure that the
"sw" instruction in the EX expansion was really and
truly a single instruction.

> Never the less we do not handle entries in the __ex_table which is
> located in a branch delay.
> So we need to handle the situation where we take a page fault on an
> instruction which is located in a brach delay slot, or we don't put the
> "potential" faulting instruction in a delay slot.
> 
> Any ideas, how we should handle this in a nice and clean way?

Is the __ex_table really ending up in the delay slot?
Just looking at the source, I have the impression
that the "sw t0,..." instruction should be in the delay
slot, followed by the __ex_table.

On another topic, now that I've patched the kernel to
turn off the stupid stuck interrupt on my Malta board,
I've realized that I can't just connect my old Atlas SCSI
disk.  I'm torn between ordering a Tekram 390 PCI
SCSI card, which should be able to use our "MIPS
safe" NCR driver as-is (I hope) and buying an IDE
disk and going through the network install ritual.
Which do you recommend?  One thing I really never
knew was just what kernel config options I need to
select to build a kernel that can do the NFS-root
bootstrap.  Can you help me there?

            Regards,

            Kevin K.

Re: Bug in the _save_fp_context.

[Kevin D. Kissell]

Oops!  I hadn't noticed that Carsten had copied the
Linux-MIPS mailing list on this, so I treated it as a
point-to-point communication.  The rest of you can
ignore my last paragraph!

            Kevin K.

----- Original Message -----
From: "Kevin D. Kissell" <kevink@mips.com>
To: "Carsten Langgaard" <carstenl@mips.com>; <linux-mips@oss.sgi.com>
Sent: Monday, March 19, 2001 4:43 PM
Subject: Re: Bug in the _save_fp_context.


> > I think there is a bug in the _save_fp_context function in
> > arch/mips/kernel/r4k_fpu.S
> >
> > The problem is the following piece of code:
> >
> >  jr ra
> >  .set nomacro
> >  EX(sw t0,SC_FPC_EIR(a0))
> >  nop
> >  .set macro
> >
> > First of all what should the ".set nomacro" do?
> > If it means that the EX macro shouldn't be used then this entry wouldn't
> > get into __ex_table, which would be wrong.
> > But it look like it uses the macro anyway, regardless of the ".set
> > nomacro", at least with the compiler I use.
>
> Not surprising, really.  "EX" is presumably a cpp macro
> that gets expanded by gcc from the .S file, based on
> some include file.  .set directives affect only the assembler,
> and would inhibit assembler-level macros only.  I'm not
> sure just what the definition of an assembler macro
> would be - it may or may not include pseudo-instructions
> like "la" or "li 32_bit_constant".  I *think* that what the
> author was trying to do here was to ensure that the
> "sw" instruction in the EX expansion was really and
> truly a single instruction.
>
> > Never the less we do not handle entries in the __ex_table which is
> > located in a branch delay.
> > So we need to handle the situation where we take a page fault on an
> > instruction which is located in a brach delay slot, or we don't put the
> > "potential" faulting instruction in a delay slot.
> >
> > Any ideas, how we should handle this in a nice and clean way?
>
> Is the __ex_table really ending up in the delay slot?
> Just looking at the source, I have the impression
> that the "sw t0,..." instruction should be in the delay
> slot, followed by the __ex_table.
>
> On another topic, now that I've patched the kernel to
> turn off the stupid stuck interrupt on my Malta board,
> I've realized that I can't just connect my old Atlas SCSI
> disk.  I'm torn between ordering a Tekram 390 PCI
> SCSI card, which should be able to use our "MIPS
> safe" NCR driver as-is (I hope) and buying an IDE
> disk and going through the network install ritual.
> Which do you recommend?  One thing I really never
> knew was just what kernel config options I need to
> select to build a kernel that can do the NFS-root
> bootstrap.  Can you help me there?
>
>             Regards,
>
>             Kevin K.
>

Re: Bug in the _save_fp_context.

[Kevin D. Kissell]

Oops!  I hadn't noticed that Carsten had copied the
Linux-MIPS mailing list on this, so I treated it as a
point-to-point communication.  The rest of you can
ignore my last paragraph!

            Kevin K.

----- Original Message -----
From: "Kevin D. Kissell" <kevink@mips.com>
To: "Carsten Langgaard" <carstenl@mips.com>; <linux-mips@oss.sgi.com>
Sent: Monday, March 19, 2001 4:43 PM
Subject: Re: Bug in the _save_fp_context.


> > I think there is a bug in the _save_fp_context function in
> > arch/mips/kernel/r4k_fpu.S
> >
> > The problem is the following piece of code:
> >
> >  jr ra
> >  .set nomacro
> >  EX(sw t0,SC_FPC_EIR(a0))
> >  nop
> >  .set macro
> >
> > First of all what should the ".set nomacro" do?
> > If it means that the EX macro shouldn't be used then this entry wouldn't
> > get into __ex_table, which would be wrong.
> > But it look like it uses the macro anyway, regardless of the ".set
> > nomacro", at least with the compiler I use.
>
> Not surprising, really.  "EX" is presumably a cpp macro
> that gets expanded by gcc from the .S file, based on
> some include file.  .set directives affect only the assembler,
> and would inhibit assembler-level macros only.  I'm not
> sure just what the definition of an assembler macro
> would be - it may or may not include pseudo-instructions
> like "la" or "li 32_bit_constant".  I *think* that what the
> author was trying to do here was to ensure that the
> "sw" instruction in the EX expansion was really and
> truly a single instruction.
>
> > Never the less we do not handle entries in the __ex_table which is
> > located in a branch delay.
> > So we need to handle the situation where we take a page fault on an
> > instruction which is located in a brach delay slot, or we don't put the
> > "potential" faulting instruction in a delay slot.
> >
> > Any ideas, how we should handle this in a nice and clean way?
>
> Is the __ex_table really ending up in the delay slot?
> Just looking at the source, I have the impression
> that the "sw t0,..." instruction should be in the delay
> slot, followed by the __ex_table.
>
> On another topic, now that I've patched the kernel to
> turn off the stupid stuck interrupt on my Malta board,
> I've realized that I can't just connect my old Atlas SCSI
> disk.  I'm torn between ordering a Tekram 390 PCI
> SCSI card, which should be able to use our "MIPS
> safe" NCR driver as-is (I hope) and buying an IDE
> disk and going through the network install ritual.
> Which do you recommend?  One thing I really never
> knew was just what kernel config options I need to
> select to build a kernel that can do the NFS-root
> bootstrap.  Can you help me there?
>
>             Regards,
>
>             Kevin K.
>

Re: Bug in the _save_fp_context.

[Jun Sun]

"Kevin D. Kissell" wrote:
> 
> Oops!  I hadn't noticed that Carsten had copied the
> Linux-MIPS mailing list on this, so I treated it as a
> point-to-point communication.  The rest of you can
> ignore my last paragraph!
> 
>             Kevin K.
> 

Oops!  Please ignore my previous reply too. :-)

Jun

Re: Bug in the _save_fp_context.

[Carsten Langgaard]

"Kevin D. Kissell" wrote:

> > I think there is a bug in the _save_fp_context function in
> > arch/mips/kernel/r4k_fpu.S
> >
> > The problem is the following piece of code:
> >
> >  jr ra
> >  .set nomacro
> >  EX(sw t0,SC_FPC_EIR(a0))
> >  nop
> >  .set macro
> >
> > First of all what should the ".set nomacro" do?
> > If it means that the EX macro shouldn't be used then this entry wouldn't
> > get into __ex_table, which would be wrong.
> > But it look like it uses the macro anyway, regardless of the ".set
> > nomacro", at least with the compiler I use.
>
> Not surprising, really.  "EX" is presumably a cpp macro
> that gets expanded by gcc from the .S file, based on
> some include file.  .set directives affect only the assembler,
> and would inhibit assembler-level macros only.  I'm not
> sure just what the definition of an assembler macro
> would be - it may or may not include pseudo-instructions
> like "la" or "li 32_bit_constant".  I *think* that what the
> author was trying to do here was to ensure that the
> "sw" instruction in the EX expansion was really and
> truly a single instruction.
>
> > Never the less we do not handle entries in the __ex_table which is
> > located in a branch delay.
> > So we need to handle the situation where we take a page fault on an
> > instruction which is located in a brach delay slot, or we don't put the
> > "potential" faulting instruction in a delay slot.
> >
> > Any ideas, how we should handle this in a nice and clean way?
>
> Is the __ex_table really ending up in the delay slot?
> Just looking at the source, I have the impression
> that the "sw t0,..." instruction should be in the delay
> slot, followed by the __ex_table.

The problem is that the address of the delay slot is put in the __ex_table
and then we take a page fault EPC is pointing at the jr instruction and not
the delay slot.
This result in a miss match when we try to lookup in __ex_table, resulting in
a kernel crash.

The faulting situation look like this:
EPC = address of delay slot
entry in __ex_table = address of delay slot - 4

Hopes that clarify it a bit more.

>
> On another topic, now that I've patched the kernel to
> turn off the stupid stuck interrupt on my Malta board,
> I've realized that I can't just connect my old Atlas SCSI
> disk.  I'm torn between ordering a Tekram 390 PCI
> SCSI card, which should be able to use our "MIPS
> safe" NCR driver as-is (I hope) and buying an IDE
> disk and going through the network install ritual.
> Which do you recommend?  One thing I really never
> knew was just what kernel config options I need to
> select to build a kernel that can do the NFS-root
> bootstrap.  Can you help me there?
>
>             Regards,
>
>             Kevin K.

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com