Thread (160 messages) 160 messages, 11 authors, 2015-04-20

Re: [PATCH v3] MIPS: kernel: elf: Improve the overall ABI and FPU mode checks

From: Måns Rullgård <hidden>
Date: 2015-02-26 10:31:45

Markos Chandras [off-list ref] writes:
quoted hunk ↗ jump to hunk
On Thu, Feb 26, 2015 at 09:31:36AM +0000, Matthew Fortune wrote:
quoted
Markos Chandras [off-list ref]  writes:
quoted
On Thu, Feb 26, 2015 at 09:14:41AM +0000, Måns Rullgård wrote:
quoted
Markos Chandras [off-list ref] writes:
quoted
On Tue, Feb 24, 2015 at 02:26:04PM +0000, Matthew Fortune wrote:
quoted
Måns Rullgård [off-list ref] writes:
quoted
Markos Chandras [off-list ref] writes:
quoted
Hi,

On Tue, Feb 24, 2015 at 01:17:33PM +0000, Måns Rullgård wrote:
quoted
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.
quoted
quoted
+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.
quoted
quoted
quoted
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)
It's definitely causing problems on my 74Kf system.
quoted
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
OK, so this sounds like what differs from what I remember seeing. I thought
the various SET_PERSONALITY macros were setting up the default FPU flags
(default as in to match the default O32 FP ABI) and then this would be
updated in mips_set_personality_fp(). Iirc then mips_set_personality_fp
is only called for the O32 case so the FPU related flags must be correctly
set for n32/n64 in the macros. Why not just update the O32 macros to
set the mode?
Yes that should be fine too.

Mans, could you try the following patch please since it seems you already have
a suitable environment ready to reproduce this problem
diff --git a/arch/mips/include/asm/elf.h b/arch/mips/include/asm/elf.h
index 535f196ffe02..694925a26924 100644
--- a/arch/mips/include/asm/elf.h
+++ b/arch/mips/include/asm/elf.h
@@ -294,6 +294,9 @@ do {									\
 	if (personality(current->personality) != PER_LINUX)		\
 		set_personality(PER_LINUX);				\
 									\
+	clear_thread_flag(TIF_HYBRID_FPREGS);				\
+	set_thread_flag(TIF_32BIT_FPREGS);				\
+									\
 	mips_set_personality_fp(state);					\
 									\
 	current->thread.abi = &mips_abi;				\
@@ -319,6 +322,8 @@ do {									\
 	do {								\
 		set_thread_flag(TIF_32BIT_REGS);			\
 		set_thread_flag(TIF_32BIT_ADDR);			\
+		clear_thread_flag(TIF_HYBRID_FPREGS);			\
+		set_thread_flag(TIF_32BIT_FPREGS);			\
 									\
 		mips_set_personality_fp(state);				\
 									\
If that works, I will submit a format patch asap.
Seems to work.

-- 
Måns Rullgård
mans@mansr.com
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help