| Message ID | 20210222034342.13136-1-yunqiang.su@cipunited.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] MIPS: introduce config option to force use FR=0 for FPXX binary | expand |
YunQiang Su <yunqiang.su@cipunited.com> 于2021年2月22日周一 上午11:45写道： > > some binary, for example the output of golang, may be mark as FPXX, > while in fact they are still FP32. > > Since FPXX binary can work with both FR=1 and FR=0, we introduce a > config option CONFIG_MIPS_O32_FPXX_USE_FR0 to force it to use FR=0 here. As the diffination, .gnu.attribution 4,0 is the same as no .gnu.attribution section. Its meaning is that the binary has no float operation at all. I worry about that if we force 4,0 as 4,1, it may cause some compatible problems. > > https://go-review.googlesource.com/c/go/+/239217 > https://go-review.googlesource.com/c/go/+/237058 > > v3->v4: > introduce a config option: CONFIG_MIPS_O32_FPXX_USE_FR0 > > v2->v3: > commit message: add Signed-off-by and Cc to stable. > > v1->v2: > Fix bad commit message: in fact, we are switching to FR=0 > > Signed-off-by: YunQiang Su <yunqiang.su@cipunited.com> > Cc: stable@vger.kernel.org # 4.19+ > --- > arch/mips/Kconfig | 11 +++++++++++ > arch/mips/kernel/elf.c | 13 ++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index 0a17bedf4f0d..442db620636f 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -3100,6 +3100,17 @@ config MIPS_O32_FP64_SUPPORT > > If unsure, say N. > > +config MIPS_O32_FPXX_USE_FR0 > + bool "Use FR=0 mode for O32 FPXX binaries" if !CPU_MIPSR6 > + depends on MIPS_O32_FP64_SUPPORT > + help > + O32 FPXX can works on both FR=0 and FR=1 mode, so by default, the > + mode preferred by hardware is used. > + > + While some binaries may be marked as FPXX by mistake, for example > + output of golang: they are in fact FP32 mode. To compatiable with > + these binaries, we should use FR=0 mode for them. > + > config USE_OF > bool > select OF > diff --git a/arch/mips/kernel/elf.c b/arch/mips/kernel/elf.c > index 7b045d2a0b51..443ced26ee60 100644 > --- a/arch/mips/kernel/elf.c > +++ b/arch/mips/kernel/elf.c > @@ -234,9 +234,10 @@ int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr, > * fpxx case. This is because, in any-ABI (or no-ABI) we have no FPU > * instructions so we don't care about the mode. We will simply use > * the one preferred by the hardware. In fpxx case, that ABI can > - * handle both FR=1 and FR=0, so, again, we simply choose the one > - * preferred by the hardware. Next, if we only use single-precision > - * FPU instructions, and the default ABI FPU mode is not good > + * handle both FR=1 and FR=0. Here, we may need to use FR=0, because > + * some binaries may be mark as FPXX by mistake (ie, output of golang). > + * - If we only use single-precision FPU instructions, > + * and the default ABI FPU mode is not good > * (ie single + any ABI combination), we set again the FPU mode to the > * one is preferred by the hardware. Next, if we know that the code > * will only use single-precision instructions, shown by single being > @@ -248,8 +249,14 @@ int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr, > */ > if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) > state->overall_fp_mode = FP_FRE; > +#if CONFIG_MIPS_O32_FPXX_USE_FR0 > + else if (prog_req.fr1 && prog_req.frdefault) > + state->overall_fp_mode = FP_FR0; > + else if (prog_req.single && !prog_req.frdefault) > +#else > else if ((prog_req.fr1 && prog_req.frdefault) || > (prog_req.single && !prog_req.frdefault)) > +#endif > /* Make sure 64-bit MIPS III/IV/64R1 will not pick FR1 */ > state->overall_fp_mode = ((raw_current_cpu_data.fpu_id & MIPS_FPIR_F64) && > cpu_has_mips_r2_r6) ? > -- > 2.20.1 >
On Tue, 23 Feb 2021, YunQiang Su wrote: > YunQiang Su <yunqiang.su@cipunited.com> 于2021年2月22日周一 上午11:45写道： > > > > some binary, for example the output of golang, may be mark as FPXX, > > while in fact they are still FP32. > > > > Since FPXX binary can work with both FR=1 and FR=0, we introduce a > > config option CONFIG_MIPS_O32_FPXX_USE_FR0 to force it to use FR=0 here. > > As the diffination, .gnu.attribution 4,0 is the same as no > .gnu.attribution section. > Its meaning is that the binary has no float operation at all. Nope, quoting include/elf/mips.h from GNU binutils: /* Not tagged or not using any ABIs affected by the differences. */ Val_GNU_MIPS_ABI_FP_ANY = 0, which means that the object file *either* does not use FP *or* has not been tagged at all, as the GNU linker does not tell these two cases apart internally (yes, quite useless semantics, but there you go; I think the enumeration constant of 0 shouldn't have been chosen for any explicit tag, or possibly for double float instead, so this is an ABI design mistake). Any object produced before mid 2007, specifically this GNU binutils commit: commit 2cf19d5cb991a88bdc71d0852de8206d9510678f Author: Joseph Myers <joseph@codesourcery.com> Date: Fri Jun 29 16:41:32 2007 +0000 will not have been tagged for FP use and therefore the traditional MIPS/Linux psABI of double float has to be assumed. This is also the correct interpretation for objects produced by Golang, which I have concluded are actually just fine according to the traditional psABI definition. It looks to me like the bug is solely in the linker, due to this weird interpretation quoted above and unforeseen consequences for FPXX links invented much later. Yes, the two cases (not tagged vs tagged as 0) ought to be told apart and I outlined a solution (needed for different reasons, but with the same motivation) for the GNU linker in the course of the discussion around NaN interlinking design, but that has never materialised before I effectively left the MIPS world, only remaining active in a limited manner for legacy MIPS platforms (that is ISAs I-IV). Maciej
> -----邮件原件----- > 发件人: Maciej W. Rozycki <macro@orcam.me.uk> > 发送时间: 2021年2月28日 9:48 > 收件人: YunQiang Su <wzssyqa@gmail.com> > 抄送: YunQiang Su <yunqiang.su@cipunited.com>; Thomas Bogendoerfer > <tsbogend@alpha.franken.de>; Jiaxun Yang <jiaxun.yang@flygoat.com>; > linux-mips <linux-mips@vger.kernel.org>; stable@vger.kernel.org > 主题: Re: [PATCH v4] MIPS: introduce config option to force use FR=0 for FPXX > binary > > On Tue, 23 Feb 2021, YunQiang Su wrote: > > > YunQiang Su <yunqiang.su@cipunited.com> 于2021年2月22日周一 上 > 午11:45写道： > > > > > > some binary, for example the output of golang, may be mark as FPXX, > > > while in fact they are still FP32. > > > > > > Since FPXX binary can work with both FR=1 and FR=0, we introduce a > > > config option CONFIG_MIPS_O32_FPXX_USE_FR0 to force it to use FR=0 > here. > > > > As the diffination, .gnu.attribution 4,0 is the same as no > > .gnu.attribution section. > > Its meaning is that the binary has no float operation at all. > > Nope, quoting include/elf/mips.h from GNU binutils: > > /* Not tagged or not using any ABIs affected by the differences. */ > Val_GNU_MIPS_ABI_FP_ANY = 0, > > which means that the object file *either* does not use FP *or* has not been > tagged at all, as the GNU linker does not tell these two cases apart internally > (yes, quite useless semantics, but there you go; I think the enumeration > constant of 0 shouldn't have been chosen for any explicit tag, or possibly for > double float instead, so this is an ABI design mistake). > > Any object produced before mid 2007, specifically this GNU binutils > commit: > > commit 2cf19d5cb991a88bdc71d0852de8206d9510678f > Author: Joseph Myers <joseph@codesourcery.com> > Date: Fri Jun 29 16:41:32 2007 +0000 > > will not have been tagged for FP use and therefore the traditional MIPS/Linux > psABI of double float has to be assumed. > > This is also the correct interpretation for objects produced by Golang, which I > have concluded are actually just fine according to the traditional psABI > definition. It looks to me like the bug is solely in the linker, due to this weird > interpretation quoted above and unforeseen consequences for FPXX links > invented much later. > Yes. This a bug of linker, and we should fix it. While for pre-existing binaries, we need a solution to make it workable, especially for the generic Linux distributions, just like Debian. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=962485 > Yes, the two cases (not tagged vs tagged as 0) ought to be told apart and I > outlined a solution (needed for different reasons, but with the same > motivation) for the GNU linker in the course of the discussion around NaN > interlinking design, but that has never materialised before I effectively left the > MIPS world, only remaining active in a limited manner for legacy MIPS > platforms (that is ISAs I-IV). > > Maciej
On Sun, 28 Feb 2021, Maciej W. Rozycki wrote: > Nope, quoting include/elf/mips.h from GNU binutils: > > /* Not tagged or not using any ABIs affected by the differences. */ > Val_GNU_MIPS_ABI_FP_ANY = 0, > > which means that the object file *either* does not use FP *or* has not > been tagged at all, as the GNU linker does not tell these two cases apart > internally (yes, quite useless semantics, but there you go; I think the > enumeration constant of 0 shouldn't have been chosen for any explicit tag, > or possibly for double float instead, so this is an ABI design mistake). FAOD I think the original intent was to make non-tagged legacy objects link-compatible with any FP ABI under the assumption that the user knows what he's doing. While that is acceptable, it shouldn't have implied the absence of FP code in such legacy objects. Instead legacy properties should have been implied, that is double FP and likewise legacy NaN. It would have been easier if a non-zero enumeration constant was assigned to Val_GNU_MIPS_ABI_FP_ANY, as generic GNU linker code considers the absence of a given tag equivalent to that tag being equal zero. This still can be handled, but complicates matters. Maciej
On Sun, 28 Feb 2021, yunqiang.su@cipunited.com wrote: > > This is also the correct interpretation for objects produced by Golang, which I > > have concluded are actually just fine according to the traditional psABI > > definition. It looks to me like the bug is solely in the linker, due to this weird > > interpretation quoted above and unforeseen consequences for FPXX links > > invented much later. > > > > Yes. This a bug of linker, and we should fix it. > While for pre-existing binaries, we need a solution to make it workable, especially for the > generic Linux distributions, just like Debian. > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=962485 Thanks for the pointer. After a bit of thinking and having fully understood what the issue actually is I conclude a change like your original one (with no configuration option; we've got too many of them already) will be OK so long as it keeps the current arrangement for R6, which has the FR mode hardwired, because, as you say, for genuine FPXX binaries the actual FR setting does not matter, so the change in the fixed form won't break what hasn't been broken already. Please keep the history of changes in the comment section rather that the change description though. Also I think the change description needs to be more elaborate on the motivation, so that someone who looks at it say 10 years from now can figure out what is going on here. You can reuse bits of our discussion for that purpose. Sadly I can see many changes going in where the description hardly says anything, and while the matter may seem obvious right now, it surely won't be for someone trying to unbreak things years from now while keeping the intent of the original change where it did the right thing. Especially as secondary sources of information may not be easily available (anymore) and the test environment may not be easily reproducible. Notice how often I need to refer to changes that were made many years ago and were not always correct. NB the real problem are not programs included with the distribution (which as I say can and ought to be fixed up with a script automatically; a distribution needs to have provisions for such workarounds as problems with the toolchain inevitably do happen from time to time), but programs built by users of the distribution who we cannot reasonably expect to be aware of every single quirk out there. Observe however that this does not solve the issue of a link-time or load-time incompatibility between FP32 modules incorrectly marked FPXX and FP64 or FP64A modules. These will be let through and depending on usage likely eventually fail. You might be able to come up with a wrapper script in place of whatever the Golang invocation command is to postprocess modules produced in user compilations as well, and have it distributed until the linker issue has been fixed upstream and the changes propagated back to the distribution. Maciej
Maciej W. Rozycki <macro@orcam.me.uk> 于2021年2月28日周日 下午9:39写道： > > On Sun, 28 Feb 2021, yunqiang.su@cipunited.com wrote: > > > > This is also the correct interpretation for objects produced by Golang, which I > > > have concluded are actually just fine according to the traditional psABI > > > definition. It looks to me like the bug is solely in the linker, due to this weird > > > interpretation quoted above and unforeseen consequences for FPXX links > > > invented much later. > > > > > > > Yes. This a bug of linker, and we should fix it. > > While for pre-existing binaries, we need a solution to make it workable, especially for the > > generic Linux distributions, just like Debian. > > > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=962485 > > Thanks for the pointer. > > After a bit of thinking and having fully understood what the issue > actually is I conclude a change like your original one (with no > configuration option; we've got too many of them already) will be OK so > long as it keeps the current arrangement for R6, which has the FR mode > hardwired, because, as you say, for genuine FPXX binaries the actual FR > setting does not matter, so the change in the fixed form won't break what > hasn't been broken already. > > Please keep the history of changes in the comment section rather that the > change description though. Also I think the change description needs to > be more elaborate on the motivation, so that someone who looks at it say > 10 years from now can figure out what is going on here. You can reuse > bits of our discussion for that purpose. > > Sadly I can see many changes going in where the description hardly says > anything, and while the matter may seem obvious right now, it surely won't > be for someone trying to unbreak things years from now while keeping the > intent of the original change where it did the right thing. Especially as > secondary sources of information may not be easily available (anymore) and > the test environment may not be easily reproducible. Notice how often I > need to refer to changes that were made many years ago and were not always > correct. > > NB the real problem are not programs included with the distribution > (which as I say can and ought to be fixed up with a script automatically; > a distribution needs to have provisions for such workarounds as problems > with the toolchain inevitably do happen from time to time), but programs > built by users of the distribution who we cannot reasonably expect to be > aware of every single quirk out there. > The "stable" branch of distribution is in the same situation as the user. Normally, we cannot modify the binary in the "stable" branch. > Observe however that this does not solve the issue of a link-time or > load-time incompatibility between FP32 modules incorrectly marked FPXX and > FP64 or FP64A modules. These will be let through and depending on usage > likely eventually fail. > Yes, that's a problem. the patch of golang has been merged now. https://go-review.googlesource.com/c/go/+/239217 https://go-review.googlesource.com/c/go/+/237058 And we will continue to try to fix binutils/llvm etc. > You might be able to come up with a wrapper script in place of whatever > the Golang invocation command is to postprocess modules produced in user > compilations as well, and have it distributed until the linker issue has > been fixed upstream and the changes propagated back to the distribution. > We fixed the golang itself, and the packages has been in Debian bullseye now. > Maciej
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 0a17bedf4f0d..442db620636f 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -3100,6 +3100,17 @@ config MIPS_O32_FP64_SUPPORT If unsure, say N. +config MIPS_O32_FPXX_USE_FR0 + bool "Use FR=0 mode for O32 FPXX binaries" if !CPU_MIPSR6 + depends on MIPS_O32_FP64_SUPPORT + help + O32 FPXX can works on both FR=0 and FR=1 mode, so by default, the + mode preferred by hardware is used. + + While some binaries may be marked as FPXX by mistake, for example + output of golang: they are in fact FP32 mode. To compatiable with + these binaries, we should use FR=0 mode for them. + config USE_OF bool select OF diff --git a/arch/mips/kernel/elf.c b/arch/mips/kernel/elf.c index 7b045d2a0b51..443ced26ee60 100644 --- a/arch/mips/kernel/elf.c +++ b/arch/mips/kernel/elf.c @@ -234,9 +234,10 @@ int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr, * fpxx case. This is because, in any-ABI (or no-ABI) we have no FPU * instructions so we don't care about the mode. We will simply use * the one preferred by the hardware. In fpxx case, that ABI can - * handle both FR=1 and FR=0, so, again, we simply choose the one - * preferred by the hardware. Next, if we only use single-precision - * FPU instructions, and the default ABI FPU mode is not good + * handle both FR=1 and FR=0. Here, we may need to use FR=0, because + * some binaries may be mark as FPXX by mistake (ie, output of golang). + * - If we only use single-precision FPU instructions, + * and the default ABI FPU mode is not good * (ie single + any ABI combination), we set again the FPU mode to the * one is preferred by the hardware. Next, if we know that the code * will only use single-precision instructions, shown by single being @@ -248,8 +249,14 @@ int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr, */ if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) state->overall_fp_mode = FP_FRE; +#if CONFIG_MIPS_O32_FPXX_USE_FR0 + else if (prog_req.fr1 && prog_req.frdefault) + state->overall_fp_mode = FP_FR0; + else if (prog_req.single && !prog_req.frdefault) +#else else if ((prog_req.fr1 && prog_req.frdefault) || (prog_req.single && !prog_req.frdefault)) +#endif /* Make sure 64-bit MIPS III/IV/64R1 will not pick FR1 */ state->overall_fp_mode = ((raw_current_cpu_data.fpu_id & MIPS_FPIR_F64) && cpu_has_mips_r2_r6) ?
some binary, for example the output of golang, may be mark as FPXX, while in fact they are still FP32. Since FPXX binary can work with both FR=1 and FR=0, we introduce a config option CONFIG_MIPS_O32_FPXX_USE_FR0 to force it to use FR=0 here. https://go-review.googlesource.com/c/go/+/239217 https://go-review.googlesource.com/c/go/+/237058 v3->v4: introduce a config option: CONFIG_MIPS_O32_FPXX_USE_FR0 v2->v3: commit message: add Signed-off-by and Cc to stable. v1->v2: Fix bad commit message: in fact, we are switching to FR=0 Signed-off-by: YunQiang Su <yunqiang.su@cipunited.com> Cc: stable@vger.kernel.org # 4.19+ --- arch/mips/Kconfig | 11 +++++++++++ arch/mips/kernel/elf.c | 13 ++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)