From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Thu, 26 Feb 2015 11:31:45 +0100 (CET) Received: from unicorn.mansr.com ([81.2.72.234]:47947 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27007349AbbBZKbndneQr convert rfc822-to-8bit (ORCPT ); Thu, 26 Feb 2015 11:31:43 +0100 Received: by unicorn.mansr.com (Postfix, from userid 51770) id 798A61538A; Thu, 26 Feb 2015 10:31:37 +0000 (GMT) From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Markos Chandras Cc: Matthew Fortune , "linux-mips\@linux-mips.org" , Paul Burton Subject: Re: [PATCH v3] MIPS: kernel: elf: Improve the overall ABI and FPU mode checks References: <6D39441BF12EF246A7ABCE6654B0235320FBCA7C@LEMAIL01.le.imgtec.org> <1422893593-1291-1-git-send-email-markos.chandras@imgtec.com> <20150224135225.GA23928@mchandras-linux.le.imgtec.org> <6D39441BF12EF246A7ABCE6654B0235320FDEE92@LEMAIL01.le.imgtec.org> <20150226085943.GA26793@mchandras-linux.le.imgtec.org> <20150226092406.GA27701@mchandras-linux.le.imgtec.org> <6D39441BF12EF246A7ABCE6654B0235320FE182B@LEMAIL01.le.imgtec.org> <20150226094445.GA18496@mchandras-linux.le.imgtec.org> Date: Thu, 26 Feb 2015 10:31:37 +0000 In-Reply-To: <20150226094445.GA18496@mchandras-linux.le.imgtec.org> (Markos Chandras's message of "Thu, 26 Feb 2015 09:44:45 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 45995 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: mans@mansr.com Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips Markos Chandras writes: > On Thu, Feb 26, 2015 at 09:31:36AM +0000, Matthew Fortune wrote: >> Markos Chandras writes: >> > On Thu, Feb 26, 2015 at 09:14:41AM +0000, Måns Rullgård wrote: >> > > Markos Chandras writes: >> > > >> > > > On Tue, Feb 24, 2015 at 02:26:04PM +0000, Matthew Fortune wrote: >> > > >> Måns Rullgård writes: >> > > >> > Markos Chandras 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) >> > > >> > > It's definitely causing problems on my 74Kf system. >> > > >> > > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from unicorn.mansr.com ([81.2.72.234]:47947 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27007349AbbBZKbndneQr convert rfc822-to-8bit (ORCPT ); Thu, 26 Feb 2015 11:31:43 +0100 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= Subject: Re: [PATCH v3] MIPS: kernel: elf: Improve the overall ABI and FPU mode checks References: <6D39441BF12EF246A7ABCE6654B0235320FBCA7C@LEMAIL01.le.imgtec.org> <1422893593-1291-1-git-send-email-markos.chandras@imgtec.com> <20150224135225.GA23928@mchandras-linux.le.imgtec.org> <6D39441BF12EF246A7ABCE6654B0235320FDEE92@LEMAIL01.le.imgtec.org> <20150226085943.GA26793@mchandras-linux.le.imgtec.org> <20150226092406.GA27701@mchandras-linux.le.imgtec.org> <6D39441BF12EF246A7ABCE6654B0235320FE182B@LEMAIL01.le.imgtec.org> <20150226094445.GA18496@mchandras-linux.le.imgtec.org> Date: Thu, 26 Feb 2015 10:31:37 +0000 In-Reply-To: <20150226094445.GA18496@mchandras-linux.le.imgtec.org> (Markos Chandras's message of "Thu, 26 Feb 2015 09:44:45 +0000") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Markos Chandras Cc: Matthew Fortune , "linux-mips@linux-mips.org" , Paul Burton Message-ID: <20150226103137.AOJqwfgmkEKuxuHs4cI9LlUO5oRpQcfZqG7l4K2pA7Y@z> Markos Chandras writes: > On Thu, Feb 26, 2015 at 09:31:36AM +0000, Matthew Fortune wrote: >> Markos Chandras writes: >> > On Thu, Feb 26, 2015 at 09:14:41AM +0000, Måns Rullgård wrote: >> > > Markos Chandras writes: >> > > >> > > > On Tue, Feb 24, 2015 at 02:26:04PM +0000, Matthew Fortune wrote: >> > > >> Måns Rullgård writes: >> > > >> > Markos Chandras 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) >> > > >> > > It's definitely causing problems on my 74Kf system. >> > > >> > > > 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