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

[Aleksandar Markovic]

> 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.hogan@xxxxxxxx]
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-mips@xxxxxxxxxxxxxx; Douglas Leung; Goran Ferenc; linux-kernel@xxxxxxxxxxxxxxx; 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.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.



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