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

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

---
 arch/mips/math-emu/cp1emu.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

arch/mips/math-emu/cp1emu.c +5 -4

index 8f2f8e9..c756fd9 100644
@@ -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;

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)

Quoted 12 lines

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.

Quoted 13 lines

Quoted 12 lines

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)

Quoted 4 lines

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.

Quoted 15 lines · Kevin D. Kissell

Quoted 11 lines

Quoted 11 lines

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)

Quoted 4 lines

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.

Quoted 8 lines

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.

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];

Quoted 7 lines · Kevin D. Kissell

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:

Quoted 2 lines

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

Date: Thu, 6 May 2010 12:42:35 -0600
From: Shane McDonald <[email protected]>
To: "Kevin D. Kissell" <[email protected]>
Cc: Sergei Shtylyov <[email protected]>,
Atsushi Nemoto <[email protected]>, [email protected], [email protected]
Subject: Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable
by
ctc1
Content-Type: text/plain; charset=ISO-8859-1

Quoted 22 lines · Kevin D. Kissell

Quoted 9 lines · Sergei Shtylyov

Quoted 7 lines · Kevin D. Kissell

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:

Quoted 5 lines

Quoted 2 lines

         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.

Quoted 7 lines

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?

Quoted 24 lines · Kevin D. Kissell

Quoted 10 lines · Sergei Shtylyov

Quoted 7 lines · Kevin D. Kissell

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:

Quoted 6 lines

Quoted 3 lines

         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.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.