[MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Shane McDonald]

In the FPU emulator code of the MIPS, the Cause bits of the FCSR
register are not currently writeable by the ctc1 instruction.
In odd corner cases, this can cause problems.  For example,
a case existed where a divide-by-zero exception was generated
by the FPU, and the signal handler attempted to restore the FPU
registers to their state before the exception occurred.  In this
particular setup, writing the old value to the FCSR register
would cause another divide-by-zero exception to occur immediately.
The solution is to change the ctc1 instruction emulator code to
allow the Cause bits of the FCSR register to be writeable.
This is the behaviour of the hardware that the code is emulating.

This problem was found by Shane McDonald, but the credit for the
fix goes to Kevin Kissell.  In Kevin's words:

I submit that the bug is indeed in that ctc_op:  case of the emulator.  The
Cause bits (17:12) are supposed to be writable by that instruction, but the
CTC1 emulation won't let them be updated by the instruction.  I think that
actually if you just completely removed lines 387-388 [...]
things would work a good deal better.  At least, it would be a more accurate
emulation of the architecturally defined FPU.  If I wanted to be really,
really pedantic (which I sometimes do), I'd also protect the reserved bits
that aren't necessarily writable.

Signed-off-by: Shane McDonald <mcdonald.shane@gmail.com>
---
 arch/mips/math-emu/cp1emu.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/mips/math-emu/cp1emu.c b/arch/mips/math-emu/cp1emu.c
index 8f2f8e9..c756fd9 100644
--- a/arch/mips/math-emu/cp1emu.c
+++ b/arch/mips/math-emu/cp1emu.c
@@ -384,10 +384,11 @@ static int cop1Emulate(struct pt_regs *xcp, struct mips_fpu_struct *ctx)
 					(void *) (xcp->cp0_epc),
 					MIPSInst_RT(ir), value);
 #endif
-				value &= (FPU_CSR_FLUSH | FPU_CSR_ALL_E | FPU_CSR_ALL_S | 0x03);
-				ctx->fcr31 &= ~(FPU_CSR_FLUSH | FPU_CSR_ALL_E | FPU_CSR_ALL_S | 0x03);
-				/* convert to ieee library modes */
-				ctx->fcr31 |= (value & ~0x3) | ieee_rm[value & 0x3];
+
+				/* Don't write reserved bits,
+				   and convert to ieee library modes */
+				ctx->fcr31 = (value & ~0x1c0003) |
+						ieee_rm[value & 0x3];
 			}
 			if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E) {
 				return SIGFPE;
-- 
1.6.2.4

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Kevin D. Kissell]

I'm glad to hear that the patch is functional.  It was pretty clear that
it would solve your problem, but I was concerned that the inability to
write the Cause bits was done as a way around some other problem. 
Someone went to a certain amount of trouble to not accept them as
inputs, in violation of spec.  I just looked back, and the bug was
introduced as part of a patch of Ralf's from April 2005 entitled "Fix
Preemption and SMP problems in the FP emulator".  It's unlikely that
overriding CTC semantics actually fixed any underlying problems, but it
may have masked symptoms of problems that we can only hope have been
fixed in the mean time. Anyone run this patch on an FPUless SMP?   Oh,
for a 34Kf with a bunch of TCs! ;o)

Shane McDonald wrote:
> In the FPU emulator code of the MIPS, the Cause bits of the FCSR
> register are not currently writeable by the ctc1 instruction.
> In odd corner cases, this can cause problems.  For example,
> a case existed where a divide-by-zero exception was generated
> by the FPU, and the signal handler attempted to restore the FPU
> registers to their state before the exception occurred.  In this
> particular setup, writing the old value to the FCSR register
> would cause another divide-by-zero exception to occur immediately.
> The solution is to change the ctc1 instruction emulator code to
> allow the Cause bits of the FCSR register to be writeable.
> This is the behaviour of the hardware that the code is emulating.
>
> This problem was found by Shane McDonald, but the credit for the
> fix goes to Kevin Kissell.  In Kevin's words:
>
> I submit that the bug is indeed in that ctc_op:  case of the emulator.  The
> Cause bits (17:12) are supposed to be writable by that instruction, but the
> CTC1 emulation won't let them be updated by the instruction.  I think that
> actually if you just completely removed lines 387-388 [...]
> things would work a good deal better.  At least, it would be a more accurate
> emulation of the architecturally defined FPU.  If I wanted to be really,
> really pedantic (which I sometimes do), I'd also protect the reserved bits
> that aren't necessarily writable.
>
> Signed-off-by: Shane McDonald <mcdonald.shane@gmail.com>
> ---
>  arch/mips/math-emu/cp1emu.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/math-emu/cp1emu.c b/arch/mips/math-emu/cp1emu.c
> index 8f2f8e9..c756fd9 100644
> --- a/arch/mips/math-emu/cp1emu.c
> +++ b/arch/mips/math-emu/cp1emu.c
> @@ -384,10 +384,11 @@ static int cop1Emulate(struct pt_regs *xcp, struct mips_fpu_struct *ctx)
>  					(void *) (xcp->cp0_epc),
>  					MIPSInst_RT(ir), value);
>  #endif
> -				value &= (FPU_CSR_FLUSH | FPU_CSR_ALL_E | FPU_CSR_ALL_S | 0x03);
> -				ctx->fcr31 &= ~(FPU_CSR_FLUSH | FPU_CSR_ALL_E | FPU_CSR_ALL_S | 0x03);
> -				/* convert to ieee library modes */
> -				ctx->fcr31 |= (value & ~0x3) | ieee_rm[value & 0x3];
> +
> +				/* Don't write reserved bits,
> +				   and convert to ieee library modes */
> +				ctx->fcr31 = (value & ~0x1c0003) |
> +						ieee_rm[value & 0x3];
>  			}
>  			if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E) {
>  				return SIGFPE;
>   

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Ralf Baechle]

On Wed, May 05, 2010 at 12:48:33AM -0700, Kevin D. Kissell wrote:

> I'm glad to hear that the patch is functional.  It was pretty clear that
> it would solve your problem, but I was concerned that the inability to
> write the Cause bits was done as a way around some other problem. 
> Someone went to a certain amount of trouble to not accept them as
> inputs, in violation of spec.  I just looked back, and the bug was
> introduced as part of a patch of Ralf's from April 2005 entitled "Fix
> Preemption and SMP problems in the FP emulator".  It's unlikely that
> overriding CTC semantics actually fixed any underlying problems, but it
> may have masked symptoms of problems that we can only hope have been
> fixed in the mean time. Anyone run this patch on an FPUless SMP?   Oh,
> for a 34Kf with a bunch of TCs! ;o)

That's commit ID 72402ec9efdb2ea7e0ec6223cf20e7301a57da02 and the patch
was comitted during the CVS days which only records the comitter but not
the author.  The actual author is Atsushi Nemoto.

  Ralf

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Kevin D. Kissell]

Ralf Baechle wrote:
>> I'm glad to hear that the patch is functional.  It was pretty clear that
>> it would solve your problem, but I was concerned that the inability to
>> write the Cause bits was done as a way around some other problem. 
>> Someone went to a certain amount of trouble to not accept them as
>> inputs, in violation of spec.  I just looked back, and the bug was
>> introduced as part of a patch of Ralf's from April 2005 entitled "Fix
>> Preemption and SMP problems in the FP emulator".  It's unlikely that
>> overriding CTC semantics actually fixed any underlying problems, but it
>> may have masked symptoms of problems that we can only hope have been
>> fixed in the mean time. Anyone run this patch on an FPUless SMP?   Oh,
>> for a 34Kf with a bunch of TCs! ;o)
>>     
>
> That's commit ID 72402ec9efdb2ea7e0ec6223cf20e7301a57da02 and the patch
> was comitted during the CVS days which only records the comitter but not
> the author.  The actual author is Atsushi Nemoto.
>   
Well,. I certainly understood that you were simply the guy who did the
commit.  Didn't mean to make it sound like an accusation, but it was
marginally easier to type your name and date than to find, cut, and
paste the commit ID.  Sorry if it came off wrong.  It would be cool if
Atsushi remembered the reasoning behind the change, but empirical proof
that undoing it doesn't create a subtle problem for current SMP kernels
would be better still.

          Regards,

          Kevin K.

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Atsushi Nemoto]

On Wed, 05 May 2010 08:43:16 -0700, "Kevin D. Kissell" <kevink@paralogos.com> wrote:
> >> I'm glad to hear that the patch is functional.  It was pretty clear that
> >> it would solve your problem, but I was concerned that the inability to
> >> write the Cause bits was done as a way around some other problem. 
> >> Someone went to a certain amount of trouble to not accept them as
> >> inputs, in violation of spec.  I just looked back, and the bug was
> >> introduced as part of a patch of Ralf's from April 2005 entitled "Fix
> >> Preemption and SMP problems in the FP emulator".  It's unlikely that
> >> overriding CTC semantics actually fixed any underlying problems, but it
> >> may have masked symptoms of problems that we can only hope have been
> >> fixed in the mean time. Anyone run this patch on an FPUless SMP?   Oh,
> >> for a 34Kf with a bunch of TCs! ;o)
> >>     
> >
> > That's commit ID 72402ec9efdb2ea7e0ec6223cf20e7301a57da02 and the patch
> > was comitted during the CVS days which only records the comitter but not
> > the author.  The actual author is Atsushi Nemoto.
> >   
> Well,. I certainly understood that you were simply the guy who did the
> commit.  Didn't mean to make it sound like an accusation, but it was
> marginally easier to type your name and date than to find, cut, and
> paste the commit ID.  Sorry if it came off wrong.  It would be cool if
> Atsushi remembered the reasoning behind the change, but empirical proof
> that undoing it doesn't create a subtle problem for current SMP kernels
> would be better still.

Yes, that's my patch.  Though I cannot remember precisely, maybe I
just had (mis)thought Cause bits in FCSR are read-only at that time.
I have never used real SMP MIPS platforms, so there must be no
SMP-related issues.

I'm OK with Kevin's fix.  Thank you very much for reporting and
investigating.

---
Atsushi Nemoto

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Kevin D. Kissell]

Atsushi Nemoto wrote:
> On Wed, 05 May 2010 08:43:16 -0700, "Kevin D. Kissell" <kevink@paralogos.com> wrote:
>   
>>>> I'm glad to hear that the patch is functional.  It was pretty clear that
>>>> it would solve your problem, but I was concerned that the inability to
>>>> write the Cause bits was done as a way around some other problem. 
>>>> Someone went to a certain amount of trouble to not accept them as
>>>> inputs, in violation of spec.  I just looked back, and the bug was
>>>> introduced as part of a patch of Ralf's from April 2005 entitled "Fix
>>>> Preemption and SMP problems in the FP emulator".  It's unlikely that
>>>> overriding CTC semantics actually fixed any underlying problems, but it
>>>> may have masked symptoms of problems that we can only hope have been
>>>> fixed in the mean time. Anyone run this patch on an FPUless SMP?   Oh,
>>>> for a 34Kf with a bunch of TCs! ;o)
>>>>         
>>> That's commit ID 72402ec9efdb2ea7e0ec6223cf20e7301a57da02 and the patch
>>> was comitted during the CVS days which only records the comitter but not
>>> the author.  The actual author is Atsushi Nemoto.  
>>>       
>> Well,. I certainly understood that you were simply the guy who did the
>> commit.  Didn't mean to make it sound like an accusation, but it was
>> marginally easier to type your name and date than to find, cut, and
>> paste the commit ID.  Sorry if it came off wrong.  It would be cool if
>> Atsushi remembered the reasoning behind the change, but empirical proof
>> that undoing it doesn't create a subtle problem for current SMP kernels
>> would be better still.
>>     
>
> Yes, that's my patch.  Though I cannot remember precisely, maybe I
> just had (mis)thought Cause bits in FCSR are read-only at that time.
> I have never used real SMP MIPS platforms, so there must be no
> SMP-related issues.
>
> I'm OK with Kevin's fix.  Thank you very much for reporting and
> investigating.
>   
The patch was labeled for "preemption and SMP problems", and if you 
weren't working on an SMP system, I'd guess that you were working with 
full preemption and seeing a problem that you might have assumed was 
also relevant to SMP.  The FPU emulator *shouldn't* have pre-emption 
issues, since it works off of data structures that are instantiated per 
thread context.  The FCSR seen by thread A is logically independent of 
that seen by thread B, so that even if one emulation was preempted in 
the middle by another, they shouldn't be able to interfere with one 
another.  That was the concept, anyway.

I'm cool with the patch as is, but in the general spirit of regarding 
numeric constants other than 0 and 1 as instruments of Satan, it would 
probably be even better if those reserved bits were defined 
(FPU_CSR_RSVD, or whatever is compatible with existing convention for 
such bits) along with the other FCSR bit masks in mipsregs.h, so that 
the assigment looks like:

          ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
                   ieee_rm[value & 0x3];

That way, in the unlikely event that those reserved bits get 
architecturally assigned, the definition of the field can be changed, 
and that absolute hex mask won't be hardwired and burried in the code to 
screw things up.  If you don't want to touch mipsregs.h, then you should 
consider just using 0x3 as the value mask, and not 0x1c0003 as I 
proposed.  I honestly can't think of any problems that would arise from 
implementing those bits as R/W, even though they aren't in hardware, 
apart from maybe not successfully running AVPs as user-mode Linux binaries.

          Regards,

          Kevin K.

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Sergei Shtylyov]

Hello.

Kevin D. Kissell wrote:

> I'm cool with the patch as is, but in the general spirit of regarding 
> numeric constants other than 0 and 1 as instruments of Satan, it would 
> probably be even better if those reserved bits were defined 
> (FPU_CSR_RSVD, or whatever is compatible with existing convention for 
> such bits) along with the other FCSR bit masks in mipsregs.h, so that 
> the assigment looks like:
>
>          ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
>                   ieee_rm[value & 0x3];

   0x3 is still neither 0 nor 1, and so remains an instrument of Satan. 
How about #defining it also? :-)

WBR, Sergei

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Kevin D. Kissell]

Sergei Shtylyov wrote:
> Kevin D. Kissell wrote:
>
>> I'm cool with the patch as is, but in the general spirit of regarding
>> numeric constants other than 0 and 1 as instruments of Satan, it
>> would probably be even better if those reserved bits were defined
>> (FPU_CSR_RSVD, or whatever is compatible with existing convention for
>> such bits) along with the other FCSR bit masks in mipsregs.h, so that
>> the assigment looks like:
>>
>>          ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
>>                   ieee_rm[value & 0x3];
>
>   0x3 is still neither 0 nor 1, and so remains an instrument of Satan.
> How about #defining it also? :-)
In some software engineering cultures, that would be considered a good
idea or even mandatory.  This being the Linux kernel, I think it's OK,
because, as Shane remarked, it's a mask that's local to the module and
whose value is "obvious", and such things are pretty commonly handled as
numeric constants in the kernel.

There actually *is* a #define for that field, FPU_CSR_RD, which could be
used here in place of the 0x3, but I'm a little torn about its use.  On
one hand "value & ~(FPU_CSR_RSVD | FPU_CSR_RD)" is more clear about what
we're doing, but on the other hand, it's less obvious that "value &
FPU_CSR_RD" is generating an integer in the range 0-3 for an index.  So
I'm absolutely fine with the code as written, but wouldn't complain if
someone wanted to make it use the symbolic constant.

          Yours in excessive geekery,

          Kevin K.

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Ralf Baechle]

On Thu, May 06, 2010 at 12:42:35PM -0600, Shane McDonald wrote:
> Date:   Thu, 6 May 2010 12:42:35 -0600
> From: Shane McDonald <mcdonald.shane@gmail.com>
> To: "Kevin D. Kissell" <kevink@paralogos.com>
> Cc: Sergei Shtylyov <sshtylyov@mvista.com>,
> 	Atsushi Nemoto <anemo@mba.ocn.ne.jp>, ralf@linux-mips.org,
> 	linux-mips@linux-mips.org
> Subject: Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable
>  by
>         ctc1
> Content-Type: text/plain; charset=ISO-8859-1
> 
> On Thu, May 6, 2010 at 9:46 AM, Kevin D. Kissell <kevink@paralogos.com> wrote:
> > Sergei Shtylyov wrote:
> >> Kevin D. Kissell wrote:
> >>
> >>> I'm cool with the patch as is, but in the general spirit of regarding
> >>> numeric constants other than 0 and 1 as instruments of Satan, it
> >>> would probably be even better if those reserved bits were defined
> >>> (FPU_CSR_RSVD, or whatever is compatible with existing convention for
> >>> such bits) along with the other FCSR bit masks in mipsregs.h, so that
> >>> the assigment looks like:
> >>>
> >>>          ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
> >>>                   ieee_rm[value & 0x3];
> >>
> >>   0x3 is still neither 0 nor 1, and so remains an instrument of Satan.
> >> How about #defining it also? :-)
> >
> > In some software engineering cultures, that would be considered a good
> > idea or even mandatory.  This being the Linux kernel, I think it's OK,
> > because, as Shane remarked, it's a mask that's local to the module and
> > whose value is "obvious", and such things are pretty commonly handled as
> > numeric constants in the kernel.
> >
> > There actually *is* a #define for that field, FPU_CSR_RD, which could be
> > used here in place of the 0x3, but I'm a little torn about its use.  On
> > one hand "value & ~(FPU_CSR_RSVD | FPU_CSR_RD)" is more clear about what
> > we're doing, but on the other hand, it's less obvious that "value &
> > FPU_CSR_RD" is generating an integer in the range 0-3 for an index.  So
> > I'm absolutely fine with the code as written, but wouldn't complain if
> > someone wanted to make it use the symbolic constant.
> 
> I'm torn here, which is why I hadn't changed that at all.  I'd rather not
> use FPU_CSR_RD, because that defines a value in the rounding mode
> bits (which happens to have all the bits set), rather than a mask for the
> bits.  I'd prefer to define a new constant FPU_CSR_RM with the
> value 0x00000003 -- a better name might be FPU_CSR_RM_MASK,
> but that's inconsistent with the names of the other FCSR fields.
> And, as Kevin said, it doesn't make it clear that we're trying to generate
> an index in the range of 0 - 3.  I guess we could also define a constant
> for the number of bits to shift to get the RM bits into the low order bits,
> something like
> 
> #define FPU_CSR_RM_SHIFT 0
> 
> at which point our code could look like
> 
> ctx->fcr31 = (value & ~(FPU_CSR_RSVD | FPU_CSR_RM)) |
>                    ieee_rm[(value & FPU_CSR_RM) >> FPU_CSR_RM_SHIFT];
> 
> I'm a little wary of adding the FPU_CSR_RM_SHIFT, because there aren't
> any other SHIFT defines for the FCSR, and because the shift value is 0.
> But, the above code doesn't look as bad as I originally thought it would,
> and it probably is clearer.
> 
> Comments?
> 
> There's also use of the 0x3 magic number in a number of other cases
> in this file.  Do I make a similar change to those cases in this patch,
> or should I create a separate patch for that?  Or would that just be
> one of those minor style change patches that no one likes?

I'd certainly consider it.  It's logically a separate change so I'd
suggest a separate patch for -queue.  Your earlier patch is 2.6.34 and
-stable material however and that's another reason why this should be
separate patches.

  Ralf

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Kevin D. Kissell]

Shane McDonald wrote:
> On Thu, May 6, 2010 at 9:46 AM, Kevin D. Kissell <kevink@paralogos.com> wrote:
>   
>> Sergei Shtylyov wrote:
>>     
>>> Kevin D. Kissell wrote:
>>>
>>>       
>>>> I'm cool with the patch as is, but in the general spirit of regarding
>>>> numeric constants other than 0 and 1 as instruments of Satan, it
>>>> would probably be even better if those reserved bits were defined
>>>> (FPU_CSR_RSVD, or whatever is compatible with existing convention for
>>>> such bits) along with the other FCSR bit masks in mipsregs.h, so that
>>>> the assigment looks like:
>>>>
>>>>          ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
>>>>                   ieee_rm[value & 0x3];
>>>>         
>>>   0x3 is still neither 0 nor 1, and so remains an instrument of Satan.
>>> How about #defining it also? :-)
>>>       
>> In some software engineering cultures, that would be considered a good
>> idea or even mandatory.  This being the Linux kernel, I think it's OK,
>> because, as Shane remarked, it's a mask that's local to the module and
>> whose value is "obvious", and such things are pretty commonly handled as
>> numeric constants in the kernel.
>>
>> There actually *is* a #define for that field, FPU_CSR_RD, which could be
>> used here in place of the 0x3, but I'm a little torn about its use.  On
>> one hand "value & ~(FPU_CSR_RSVD | FPU_CSR_RD)" is more clear about what
>> we're doing, but on the other hand, it's less obvious that "value &
>> FPU_CSR_RD" is generating an integer in the range 0-3 for an index.  So
>> I'm absolutely fine with the code as written, but wouldn't complain if
>> someone wanted to make it use the symbolic constant.
>>     
>
> I'm torn here, which is why I hadn't changed that at all.  I'd rather not
> use FPU_CSR_RD, because that defines a value in the rounding mode
> bits (which happens to have all the bits set), rather than a mask for the
> bits.  I'd prefer to define a new constant FPU_CSR_RM with the
> value 0x00000003 -- a better name might be FPU_CSR_RM_MASK,
> but that's inconsistent with the names of the other FCSR fields.
> And, as Kevin said, it doesn't make it clear that we're trying to generate
> an index in the range of 0 - 3.  I guess we could also define a constant
> for the number of bits to shift to get the RM bits into the low order bits,
> something like
>
> #define FPU_CSR_RM_SHIFT 0
>
> at which point our code could look like
>
> ctx->fcr31 = (value & ~(FPU_CSR_RSVD | FPU_CSR_RM)) |
>                    ieee_rm[(value & FPU_CSR_RM) >> FPU_CSR_RM_SHIFT];
>
> I'm a little wary of adding the FPU_CSR_RM_SHIFT, because there aren't
> any other SHIFT defines for the FCSR, and because the shift value is 0.
> But, the above code doesn't look as bad as I originally thought it would,
> and it probably is clearer.
>
> Comments?
>   
I agree with Ralf's suggestion to make the further cleanup
a separate patch.  Since there isn't a precedent for having
shift counts for the FCSR fields, another option would be
to define a macro, let's call it modeindex() for the sake of
the argument, that's defined wherever ieee_rm is declared,
and which converts an FCSR value into the index:

#define modeindex(v) ((v) & FPU_CSR_RM)

Then the actual code in cop1emu.c could look like:

    ctx->fcr31 = (value & ~(FPU_CSR_RSVD | FPU_CSR_RM)) |
                                   ieee_rm[modeindex(value)];

Which is pretty clear, has no cursed constants, and doesn't
create a universe of shift counts where the only one we
care about is zero.
> There's also use of the 0x3 magic number in a number of other cases
> in this file. 
If all the others treat it as a mask, then your proposed
FPR_CSR_RM should be fine for them.  If they don't...

          Regards,

          Kevin K.

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Atsushi Nemoto]

On Wed, 05 May 2010 12:20:10 -0700, "Kevin D. Kissell" <kevink@paralogos.com> wrote:
> > Yes, that's my patch.  Though I cannot remember precisely, maybe I
> > just had (mis)thought Cause bits in FCSR are read-only at that time.
> > I have never used real SMP MIPS platforms, so there must be no
> > SMP-related issues.
> >
> The patch was labeled for "preemption and SMP problems", and if you 
> weren't working on an SMP system, I'd guess that you were working with 
> full preemption and seeing a problem that you might have assumed was 
> also relevant to SMP.  The FPU emulator *shouldn't* have pre-emption 
> issues, since it works off of data structures that are instantiated per 
> thread context.  The FCSR seen by thread A is logically independent of 
> that seen by thread B, so that even if one emulation was preempted in 
> the middle by another, they shouldn't be able to interfere with one 
> another.  That was the concept, anyway.

Yes, I created the patch for full preemption support. At that time,
ieee754_csr was a global variable, not an alias of
current->thread.fpu.soft.fcr31.

The commmit 72402e was mixture of two patch: "just make FPU emulator
fully-preemptive" and "fix FPU ownership issues on preemption".  The
wrong code is a part of the first one.  So I don't think your fix
causes any regressions on SMP/preemption point of view.

I'm still trying to remember why I masked out some bits on CTC1
emulation, but no success.  Maybe I was just confused.

Anyway, thanks Shane and Kevin!

---
Atsushi Nemoto

Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

[Ralf Baechle]

On Wed, May 05, 2010 at 08:43:16AM -0700, Kevin D. Kissell wrote:

> > That's commit ID 72402ec9efdb2ea7e0ec6223cf20e7301a57da02 and the patch
> > was comitted during the CVS days which only records the comitter but not
> > the author.  The actual author is Atsushi Nemoto.
> >   
> Well,. I certainly understood that you were simply the guy who did the
> commit.  Didn't mean to make it sound like an accusation, but it was
> marginally easier to type your name and date than to find, cut, and
> paste the commit ID.  Sorry if it came off wrong.  It would be cool if
> Atsushi remembered the reasoning behind the change, but empirical proof
> that undoing it doesn't create a subtle problem for current SMP kernels
> would be better still.

Oh, I didn't take it for an accusation but it's interesting if not useful
to get the original author involved.  In the dark past I accepted many
patches that were sent to me directly with none of the usual lists on cc.
In addition there are the limitations of what was recorded by CVS.  Some
history information is only available in my mail folders so occasionally I
have to dig deep.

  Ralf