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

[Aleksandar Markovic]

(resending since the previous mail was rejected by the mailing list because of html format)

> 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>>
> > ...
> > 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;
}


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?

Regards,
Aleksandar