> 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.
On more detailed code inspection, you are right.
> 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)
If you agree, I am going to submit v2 of the series, that would fully
address these concerns.
Additionally, it seems to me that a new round of testing that tests
involved code paths under various scenarios would be appropriate
and I am going to do that.
> Note: thats the one in fpux_emu(), not fpu_emu() which this patch
> modifies.
Yes, my bad, wanting to respond as quickly as possible, I inserted
the segment from fpux_emu(), not fpu_emu() as I should have.
By the way, and not related to this patch, I see only 4 (out of 5)
exceptions are handled in fpux_emu() case (division-by-zero is not
handled), I presume this is fine (probably division-by-zero not
needed), isn't it?
I truly appreciate your analysis and help.
Aleksandar
________________________________________
From: James Hogan [james.ho...@mips.com]
Sent: Thursday, October 12, 2017 7:44 AM
To: Aleksandar Markovic
Cc: Miodrag Dinic; Paul Burton; Petar Jovanovic; Raghu Gandham; Ralf Baechle;
Aleksandar Markovic; linux-m...@linux-mips.org; Douglas Leung; Goran Ferenc;
linux-kernel@vger.kernel.org; Maciej Rozycki; Manuel Lauss; Masahiro Yamada
Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for
certain instructions
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.ho...@mips.com>>@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.
>
> 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
