On Wed, 11 May 2016, Ralf Baechle wrote:
> > This change is broken, see the description of commit 9b26616c8d9d ("MIPS:
> > Respect the ISA level in FCSR handling") where this code comes from. The
> > very purpose is to probe for the writability of bits 31:18, in particular
> > NAN2008 and ABS2008 stuff, but it applies to vendor bits too. An accurate
> > identification of writable bits is required for the correct presentation
> > of FCSR via ptrace(2) for programs like GDB.
> >
> > You need to fix your simulator instead, the architecture does not permit
> > trapping on optional FCSR bits (there are no reserved bits there anymore
> > with the current architecture revision) especially as access to this
> > register is unprivileged. I don't think we can support arbitrary
> > non-compliant architecture implementations -- if you need to handle an
> > erratum, then please do it on a PRId by PRId basis.
> >
> > As to changing the data type, I'm fine in principle, but then please do
> > so across all our source base where CP1 control registers are handled.
> > Here the `long' type is used for consistency with the rest of code, so
> > changing just this single place seems gratuitous to me.
> >
> > Ralf, please discard this change until it has been corrected.
>
> Using a 32 bit variable made sufficient sense to me to apply the patch
> to me. However I agree, that the simulator's behaviour is overzealous.
> While in violation of the architecture specification this is probably
> similar in spirit as the mode of MIPSsim that was keeping every bit of
> the system as a tristate (0, 1 and uninitialized) which indeed cought a
> number of issues.
The fundamental problem here is this functional change:
> @@ -87,7 +87,7 @@ static inline void cpu_set_fpu_fcsr_mask(struct cpuinfo_mips
> *c)
> write_32bit_cp1_register(CP1_STATUS, fcsr0);
> fcsr0 = read_32bit_cp1_register(CP1_STATUS);
>
> - fcsr1 = fcsr | ~mask;
> + fcsr1 = fcsr | (FPU_CSR_COND | FPU_CSR_FS | FPU_CSR_CONDX);
> write_32bit_cp1_register(CP1_STATUS, fcsr1);
> fcsr1 = read_32bit_cp1_register(CP1_STATUS);
>
which largely defeats the original commit referred above and certainly
regresses 2008-NaN support. And then any type cleanup (which is syntactic
sugar really anyway, which however I'm absolutely fine with if not
enthusiastic as long as applied consistently) should be made as a separate
patch, not along a functional change.
Maciej
