On Tue, Feb 24, 2015 at 02:26:04PM +0000, Matthew Fortune wrote:
> Måns Rullgård <mans@xxxxxxxxx> writes:
> > Markos Chandras <markos.chandras@xxxxxxxxxx> writes:
> >
> > > Hi,
> > >
> > > On Tue, Feb 24, 2015 at 01:17:33PM +0000, Måns Rullgård wrote:
> > >> This patch (well, the variant that made it into 4.0-rc1) breaks
> > >> MIPS_ABI_FP_DOUBLE (the gcc default) apps on MIPS32.
> > >>
> > >
> > > Thanks for the report.
> > >
> > >> > +void mips_set_personality_fp(struct arch_elf_state *state) {
> > >> > + /*
> > >> > + * This function is only ever called for O32 ELFs so we should
> > >> > + * not be worried about N32/N64 binaries.
> > >> > + */
> > >> >
> > >> > - case MIPS_ABI_FP_XX:
> > >> > - case MIPS_ABI_FP_ANY:
> > >> > - if (!config_enabled(CONFIG_MIPS_O32_FP64_SUPPORT))
> > >> > - set_thread_flag(TIF_32BIT_FPREGS);
> > >> > - else
> > >> > - clear_thread_flag(TIF_32BIT_FPREGS);
> > >> > + if (!config_enabled(CONFIG_MIPS_O32_FP64_SUPPORT))
> > >> > + return;
> > >>
> > >> The problem is here. In a 32-bit configuration,
> > >> MIPS_O32_FP64_SUPPORT is always disabled, so the FP mode doesn't get
> > >> set. Simply deleting those two lines makes things work again, but
> > >> that's probably not the right fix.
>
> I don't recall the final decision on default on/off for this option but
> IIRC it is going to be off for everything except R6 in the first kernel
> version and then turned on by default(/option removed) when the code is
> proven for the following kernel version.
>
> > >>
> > > I had the impression that the loader would have set the FP mode
> > earlier on.
> > > But that only may happen with the latest version of the tools.
> > >
> > > Perhaps instead of dropping these two lines we need a similar check on
> > > the arch_elf_pt_proc so we don't mess with the default FPI abi?
> > >
> > > Having said that, dropping these two lines should be fine, it just
> > > means you do a little bit of extra work when loading your ELF files to
> > > check for ABI compatibility which shouldn't matter in your case.
> >
> > There's another early return like this in arch_check_elf() which should
> > probably go as well, or everything will end up with the default mode.
>
> Ironically I discussed these changes with Markos in an attempt to make
> all the new changes benign when:
>
> !config_enabled(CONFIG_MIPS_O32_FP64_SUPPORT)
>
> Clearly this has backfired. I will have to re-read the version of the code
> in 4.0-rc1 to see what is the root cause. The intention was that without
> the config option then the kernel would blindly continue to assume that
> all O32 binaries would run in the original TIF_32BIT_FPREGS mode. As I
> recall, the callers to mips_set_personality_fp were setting this mode
> which is why the simple early return was added.
>
> Thanks,
> Matthew
I think I can see what is going on. The problem is that
mips_set_personality_fp() (as already mentioned) is not executed for
!CONFIG_MIPS_O32_FP64_SUPPORT. The reason this is a problem
(i think this could only happen in 64-bit) is that SET_PERSONALITY2
clears all the thread flags related to 32-bit and FPU. The 32-bit flags will
be set again by the SET_PERSONALITY32_O32 but the FPU flags are not since
the entire mips_set_personality_fp() is skipped. While removing the if()
conditional in mips_set_personality_fp() will fix the problem, you rely on
state->overall_fp_mode having a good default value for you case. If not, it will
set the wrong FPU mode. Therefore, I believe the correct fix is either to drop
both CONFIG_MIPS_O32_FP64_SUPPORT or drop the one in mips_set_personality_fp()
and add another one in arch_elf_pt_proc() to set a good default ABI just for
this case and then return.
--
markos
