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

[James Hogan]

On Wed, Oct 11, 2017 at 04:18:49PM +0000, Aleksandar Markovic wrote:
> Thanks, James, for the review.
> 
> I've got a couple of points bellow that will, I hope, clarify several issues.
> 
> > ________________________________________
> > From: James Hogan [james.hogan@xxxxxxxx]
> > Sent: Monday, October 09, 2017 2:09 PM
> > To: Aleksandar Markovic
> > Cc: linux-mips@xxxxxxxxxxxxxx; Aleksandar Markovic; Douglas Leung;
> > Goran Ferenc; linux-kernel@xxxxxxxxxxxxxxx; Maciej Rozycki;
> > Manuel Lauss; Masahiro Yamada; Miodrag Dinic; Paul Burton;
> > Petar Jovanovic; Raghu Gandham; Ralf Baechle
> > Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP
> > exception stats for certain instructions
> > 
> > On Fri, Oct 06, 2017 at 07:29:00PM +0200, Aleksandar Markovic wrote:
> > > From: Aleksandar Markovic <aleksandar.markovic@xxxxxxxxxx>
> > >
> > > Fix omission of updating of debugfs FP exception stats for
> > > instructions <CLASS|MADDF|MSUBF|MAX|MIN|MAXA|MINA>.<D|S>.
> > >
> > > CLASS.<D|S> can generate Unimplemented Operation FP exception.
> > > <MADDF|MSUBF|MAX|MIN|MAXA|MINA>>.<D|S> can generate Inexact,
> > 
> > nit: s/>>/>/
> 
> Will be fixed in v2.
> 
> > 
> > > Unimplemented Operation, Invalid Operation, Overflow, and
> > > Underflow FP exceptions. In such cases, and prior to this
> > 
> > Well, according to the manual I'm looking at MAX|MIN|MAXA|MINA can't
> > generate inexact, overflow, or underflow FP exceptions
> >
> 
> The "MIPS64Â Instruction Set Reference Manual" v6.00 mentions that all
> five FP exception are possible for MAX|MIN|MAXA|MINA, but in v6.05, the
> list was reduced to only two, as you hinted. I am going to sync the commit
> message with v6.05 of the document.
> 
> > ... and in practice
> > the only FPE generated by emulating these instructions is invalid
> > operation for MADDF/MSUBF. So presumably that's the only case we really
> > care about.
> > 
> > I.e. the MADDF/MSUBF invalid operation should be counted, but crucially
> > the setting of rcsr bits allows it to generate a SIGFPE which from a
> > glance it doesn't appear to do until this patch. The other changes are
> > redundant and harmless.
> > 
> > Does that sound correct? (appologies if I'm missing something, I'm just
> > reading the code, and reading FPU emulation code late at night is
> > probably asking for trouble).
> >
> 
> You are close, but not quite, I'll explain that in a moment.
> 
> First, just to note that SIGFPE will be generated if the condition
> 
> ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E)
> 
> is met (the code is almost at the end of fpu_emu(), cp1emu.c, line 1557).
> ctx is one of the arguments of fpu_emu(), but, in the current state of the
> code, by analyzing several levels of invocations, it can be concluded
> that ctx will always be equal to &current->thread.fpu. So, the register
> current->thread.fpu->fcr31 is actually important here.
> 
> Now, let's consider, for simplicity, the case of MADDF operation resulting
> in an overflow caused by addition of two large FP numbers. The "special"
> treatment of such case will be done within invocation of ieee754dp_format(),

Ah yes, obviously an MADDF can generate those other exception bits :-)

> which will in turn (in this particular example) execute
> ieee754_setcx(IEEE754_OVERFLOW), which will further execute
> 
> 	ieee754_csr.cx |= flags;
> 	ieee754_csr.sx |= flags;
> 
> and, since ieee754_csr is macro for &current->thread.fpu.fcr31, this will result
> in setting tworelevant and correct bits in current->thread.fpu->fcr31.
> 
> This means that condition from few paragraphs above will be met, and SIGFPE
> will be generated.

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.

Cheers
James

> 
> Whole above scenario happens regardless of inclusion of this patch.
> 
> Actually, setting exception bits within "copcsr label code" seems redundant.
> It though does no harm. I have some theory why is this code here, and why
> we even may want to keep it as is, but this would be too much for this mail.
> 
> > 
> > Given the potential fixing of SIGFPE in that case should this be tagged
> > for stable?
> > 
> 
> As I explained above, SIGFPE behavior is already correct, without this patch.
> This patch fixes only debugfs stats. So, it is not that critical.
> 
> Looking forward to your response.
> 
> Aleksandar

Attachment: signature.asc
Description: Digital signature