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