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

[Aleksandar Markovic]

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(),
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.

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