Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions

[James Hogan]

On Thu, Oct 12, 2017 at 03:57:50PM +0200, Aleksandar Markovic wrote:
> 
> > Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions
> > Date: Thursday, October 12, 2017 12:17 CEST
> > From: James Hogan <james.hogan@xxxxxxxx>>@badag02.ba.imgtec.org>
> > > ...
> > > if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E)
> > > ...
> >
> > But just before that condition it does:
> >
> > ctx->fcr31 = (ctx->fcr31 & ~FPU_CSR_ALL_X) | rcsr;
> > I.e. it clears the X bits used in the condition, and overrides them,
> > based on rcsr, which is initialised to 0 and is only set after the
> > copcsr label and in a couple of other cases I don't think we'd be
> > hitting for MADDF.
> >
> 
> The code is odd and deceiving here. Let's see the whole "copcsr label"
> code segment:Âcopcsr:if (ieee754_cxtest(IEEE754_INEXACT)) { MIPS_FPU_EMU_INC_STATS(ieee754_inexact); rcsr |= FPU_CSR_INE_X | FPU_CSR_INE_S;}if (ieee754_cxtest(IEEE754_UNDERFLOW)) { MIPS_FPU_EMU_INC_STATS(ieee754_underflow); rcsr |= FPU_CSR_UDF_X | FPU_CSR_UDF_S;}if (ieee754_cxtest(IEEE754_OVERFLOW)) { MIPS_FPU_EMU_INC_STATS(ieee754_overflow); rcsr |= FPU_CSR_OVF_X | FPU_CSR_OVF_S;} if (ieee754_cxtest(IEEE754_INVALID_OPERATION)) { MIPS_FPU_EMU_INC_STATS(ieee754_invalidop); rcsr |= FPU_CSR_INV_X | FPU_CSR_INV_S;}Âctx->fcr31 = (ctx->fcr31 & ~FPU_CSR_ALL_X) | rcsr;if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E) { /*printk ("SIGFPE: FPU csr = %08x\n", ctx->fcr31); */ return SIGFPE;}

Note: thats the one in fpux_emu(), not fpu_emu() which this patch
modifies. In fpu_emu() the copying of bits from rcsr to fcr32 and the
SIGFPE checking takes place outside of the switch, after other stuff can
modify rcsr.

> 
> 
> Value of rcsr will be dictated by series of invocations to ieee754_cxtest(),
> which, in fact, means that exception bits will be copied from fcr31 to rcsr.
> 
> Then, fcr31 exception bits are cleared and set to the values they had just
> before clearing.
> 
> Obviously, this will not do anything in our scenarios.
> 
> However, the patch is about correct setting of debugfs stats, and this code
> segment correctly does this.

Right, but its not going to even get to copcsr until this patch, so the
SIGFPE handling is I think fixed by this patch, i.e. it isn't just about
the stats.

> 
> May I suggest that we accept my patch as is, and if anybody for any reason
> wants to deal further with related code, this should be done in a separate
> fix/patch?

This patch fixes something, I think it should
a) be clear in the commit message what is fixed
b) be tagged for stable (though that can always be done
retrospectively).

Cheers
James

Attachment: signature.asc
Description: Digital signature