2020-04-05 08:41:32

by Jiaxun Yang

[permalink] [raw]
Subject: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly

LLD failed to link vmlinux with 64bit load address for 32bit ELF
while bfd will strip 64bit address into 32bit silently.
To fix LLD build, we should supply a 32bit load address for 32bit
kernel.

Signed-off-by: Jiaxun Yang <[email protected]>
Reviewed-by: Fangrui Song <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
---
arch/mips/mti-malta/Platform | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/mips/mti-malta/Platform b/arch/mips/mti-malta/Platform
index 2cc72c9b38e3..f9b49cba1764 100644
--- a/arch/mips/mti-malta/Platform
+++ b/arch/mips/mti-malta/Platform
@@ -6,6 +6,10 @@ cflags-$(CONFIG_MIPS_MALTA) += -I$(srctree)/arch/mips/include/asm/mach-malta
ifdef CONFIG_KVM_GUEST
load-$(CONFIG_MIPS_MALTA) += 0x0000000040100000
else
+ifdef CONFIG_64BIT
load-$(CONFIG_MIPS_MALTA) += 0xffffffff80100000
+else
+ load-$(CONFIG_MIPS_MALTA) += 0x80100000
+endif
endif
all-$(CONFIG_MIPS_MALTA) := $(COMPRESSION_FNAME).bin
--
2.26.0.rc2



2020-04-05 16:48:36

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly

On Sun, 5 Apr 2020, Jiaxun Yang wrote:

> LLD failed to link vmlinux with 64bit load address for 32bit ELF
> while bfd will strip 64bit address into 32bit silently.
> To fix LLD build, we should supply a 32bit load address for 32bit
> kernel.
[...]
> diff --git a/arch/mips/mti-malta/Platform b/arch/mips/mti-malta/Platform
> index 2cc72c9b38e3..f9b49cba1764 100644
> --- a/arch/mips/mti-malta/Platform
> +++ b/arch/mips/mti-malta/Platform
> @@ -6,6 +6,10 @@ cflags-$(CONFIG_MIPS_MALTA) += -I$(srctree)/arch/mips/include/asm/mach-malta
> ifdef CONFIG_KVM_GUEST
> load-$(CONFIG_MIPS_MALTA) += 0x0000000040100000
> else
> +ifdef CONFIG_64BIT
> load-$(CONFIG_MIPS_MALTA) += 0xffffffff80100000
> +else
> + load-$(CONFIG_MIPS_MALTA) += 0x80100000

Given the description above I think it should be done uniformly and
automatically across all platforms by trimming the address supplied with
$(load-y) to low 8 digits in a single place, that is at the place where
the variable is consumed. This will reduce clutter across Makefile
fragments, avoid inconsistencies and extra work to handle individual
platforms as the problem is triggered over and over again, and limit the
risk of mistakes.

Some error checking might be doable for verifying that the 64-bit address
truncated is a sign-extended 32-bit value, but that perhaps would be an
overkill as certainly any 64-bit system that sets the load address to be
outside the sign-extended 32-bit address range does not support a !64BIT
configuration anyway.

Maciej

2020-04-05 16:58:14

by Jiaxun Yang

[permalink] [raw]
Subject: Re: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly



于 2020年4月6日 GMT+08:00 上午12:47:29, "Maciej W. Rozycki" <[email protected]> 写到:
>On Sun, 5 Apr 2020, Jiaxun Yang wrote:
>
>> LLD failed to link vmlinux with 64bit load address for 32bit ELF
>> while bfd will strip 64bit address into 32bit silently.
>> To fix LLD build, we should supply a 32bit load address for 32bit
>> kernel.
>[...]
>> diff --git a/arch/mips/mti-malta/Platform
>b/arch/mips/mti-malta/Platform
>> index 2cc72c9b38e3..f9b49cba1764 100644
>> --- a/arch/mips/mti-malta/Platform
>> +++ b/arch/mips/mti-malta/Platform
>> @@ -6,6 +6,10 @@ cflags-$(CONFIG_MIPS_MALTA) +=
>-I$(srctree)/arch/mips/include/asm/mach-malta
>> ifdef CONFIG_KVM_GUEST
>> load-$(CONFIG_MIPS_MALTA) += 0x0000000040100000
>> else
>> +ifdef CONFIG_64BIT
>> load-$(CONFIG_MIPS_MALTA) += 0xffffffff80100000
>> +else
>> + load-$(CONFIG_MIPS_MALTA) += 0x80100000
>
> Given the description above I think it should be done uniformly and
>automatically across all platforms by trimming the address supplied
>with
>$(load-y) to low 8 digits in a single place, that is at the place where
>
>the variable is consumed. This will reduce clutter across Makefile
>fragments, avoid inconsistencies and extra work to handle individual
>platforms as the problem is triggered over and over again, and limit
>the
>risk of mistakes.

I was intended to do like this but failed to find a proper way.

Makefile isn't designed for any kind of calculation.
And shell variables are 64-bit signed so it can't hold such a huge variable.

Just wish somebody can give me a way to do like:

ifndef CONFIG_64BIT
load-y = $(load-y) & 0xffffffff
endif

In makefiles.

Thanks.
>
>Some error checking might be doable for verifying that the 64-bit
>address
>truncated is a sign-extended 32-bit value, but that perhaps would be an
>
>overkill as certainly any 64-bit system that sets the load address to
>be
>outside the sign-extended 32-bit address range does not support a
>!64BIT
>configuration anyway.
>
> Maciej

--
Jiaxun Yang

2020-04-05 17:23:45

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly

On Mon, 6 Apr 2020, Jiaxun Yang wrote:

> > Given the description above I think it should be done uniformly and
> >automatically across all platforms by trimming the address supplied
> >with
> >$(load-y) to low 8 digits in a single place, that is at the place where
> >
> >the variable is consumed. This will reduce clutter across Makefile
> >fragments, avoid inconsistencies and extra work to handle individual
> >platforms as the problem is triggered over and over again, and limit
> >the
> >risk of mistakes.
>
> I was intended to do like this but failed to find a proper way.
>
> Makefile isn't designed for any kind of calculation.
> And shell variables are 64-bit signed so it can't hold such a huge variable.
>
> Just wish somebody can give me a way to do like:
>
> ifndef CONFIG_64BIT
> load-y = $(load-y) & 0xffffffff
> endif

Use the usual shell tools like `sed', `cut', `awk', or whatever we use in
the kernel build already for other purposes. There's no need to do any
actual calculation here to extract the last 8 characters (and the leading
`0x' prefix). At worst you can write a small C program, compile it with
the build system compiler and run, as we already do for some stuff.

Maciej

2020-04-06 10:58:20

by YunQiang Su

[permalink] [raw]
Subject: Re: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly

Maciej W. Rozycki <[email protected]> 于2020年4月6日周一 上午1:23写道:
>
> On Mon, 6 Apr 2020, Jiaxun Yang wrote:
>
> > > Given the description above I think it should be done uniformly and
> > >automatically across all platforms by trimming the address supplied
> > >with
> > >$(load-y) to low 8 digits in a single place, that is at the place where
> > >
> > >the variable is consumed. This will reduce clutter across Makefile
> > >fragments, avoid inconsistencies and extra work to handle individual
> > >platforms as the problem is triggered over and over again, and limit
> > >the
> > >risk of mistakes.
> >
> > I was intended to do like this but failed to find a proper way.
> >
> > Makefile isn't designed for any kind of calculation.
> > And shell variables are 64-bit signed so it can't hold such a huge variable.
> >
> > Just wish somebody can give me a way to do like:
> >
> > ifndef CONFIG_64BIT
> > load-y = $(load-y) & 0xffffffff
> > endif
>
> Use the usual shell tools like `sed', `cut', `awk', or whatever we use in

perl may be the easiest to use tool here.

ifndef CONFIG_64BIT
load-y := $(shell $(PERL) -e 'print $(load-y) & 0xffffffff')
endif

Note that it is `:=' instead of '='.

> the kernel build already for other purposes. There's no need to do any
> actual calculation here to extract the last 8 characters (and the leading
> `0x' prefix). At worst you can write a small C program, compile it with
> the build system compiler and run, as we already do for some stuff.
>
> Maciej



--
YunQiang Su

2020-04-06 11:12:27

by Jiaxun Yang

[permalink] [raw]
Subject: Re: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly



于 2020年4月6日 GMT+08:00 下午6:57:18, YunQiang Su <[email protected]> 写到:
>Maciej W. Rozycki <[email protected]> 于2020年4月6日周一 上午1:23写道:
>>
>> On Mon, 6 Apr 2020, Jiaxun Yang wrote:
>>
>> > > Given the description above I think it should be done uniformly
>and
>> > >automatically across all platforms by trimming the address
>supplied
>> > >with
>> > >$(load-y) to low 8 digits in a single place, that is at the place
>where
>> > >
>> > >the variable is consumed. This will reduce clutter across
>Makefile
>> > >fragments, avoid inconsistencies and extra work to handle
>individual
>> > >platforms as the problem is triggered over and over again, and
>limit
>> > >the
>> > >risk of mistakes.
>> >
>> > I was intended to do like this but failed to find a proper way.
>> >
>> > Makefile isn't designed for any kind of calculation.
>> > And shell variables are 64-bit signed so it can't hold such a huge
>variable.
>> >
>> > Just wish somebody can give me a way to do like:
>> >
>> > ifndef CONFIG_64BIT
>> > load-y = $(load-y) & 0xffffffff
>> > endif
>>
>> Use the usual shell tools like `sed', `cut', `awk', or whatever we
>use in
>
>perl may be the easiest to use tool here.
>
>ifndef CONFIG_64BIT
> load-y := $(shell $(PERL) -e 'print $(load-y) & 0xffffffff')
>endif
>
>Note that it is `:=' instead of '='.

It seems like perl is not one of kernel's build dependencies.[1]
I'm comsidering a alternative solution,
write a small hostprog in C to deal with that.

Thanks.

[1]: https://www.kernel.org/doc/html/v5.6/process/changes.html


>
>> the kernel build already for other purposes. There's no need to do
>any
>> actual calculation here to extract the last 8 characters (and the
>leading
>> `0x' prefix). At worst you can write a small C program, compile it
>with
>> the build system compiler and run, as we already do for some stuff.
>>
>> Maciej

--
Jiaxun Yang

2020-04-06 16:44:58

by Fangrui Song

[permalink] [raw]
Subject: Re: [PATCH] MIPS: malta: Set load address for 32bit kernel correctly

On 2020-04-06, Jiaxun Yang wrote:
>
>
>于 2020年4月6日 GMT+08:00 下午6:57:18, YunQiang Su <[email protected]> 写到:
>>Maciej W. Rozycki <[email protected]> 于2020年4月6日周一 上午1:23写道:
>>>
>>> On Mon, 6 Apr 2020, Jiaxun Yang wrote:
>>>
>>> > > Given the description above I think it should be done uniformly
>>and
>>> > >automatically across all platforms by trimming the address
>>supplied
>>> > >with
>>> > >$(load-y) to low 8 digits in a single place, that is at the place
>>where
>>> > >
>>> > >the variable is consumed. This will reduce clutter across
>>Makefile
>>> > >fragments, avoid inconsistencies and extra work to handle
>>individual
>>> > >platforms as the problem is triggered over and over again, and
>>limit
>>> > >the
>>> > >risk of mistakes.
>>> >
>>> > I was intended to do like this but failed to find a proper way.
>>> >
>>> > Makefile isn't designed for any kind of calculation.
>>> > And shell variables are 64-bit signed so it can't hold such a huge
>>variable.
>>> >
>>> > Just wish somebody can give me a way to do like:
>>> >
>>> > ifndef CONFIG_64BIT
>>> > load-y = $(load-y) & 0xffffffff
>>> > endif
>>>
>>> Use the usual shell tools like `sed', `cut', `awk', or whatever we
>>use in
>>
>>perl may be the easiest to use tool here.
>>
>>ifndef CONFIG_64BIT
>> load-y := $(shell $(PERL) -e 'print $(load-y) & 0xffffffff')
>>endif
>>
>>Note that it is `:=' instead of '='.
>
>It seems like perl is not one of kernel's build dependencies.[1]
>I'm comsidering a alternative solution,
>write a small hostprog in C to deal with that.
>
>Thanks.
>
>[1]: https://www.kernel.org/doc/html/v5.6/process/changes.html

load-y := 0xffffffff80100000
load-y := 0x$(shell echo "$(load-y)" | rev | head -c 8 | rev)

>>
>>> the kernel build already for other purposes. There's no need to do
>>any
>>> actual calculation here to extract the last 8 characters (and the
>>leading
>>> `0x' prefix). At worst you can write a small C program, compile it
>>with
>>> the build system compiler and run, as we already do for some stuff.
>>>
>>> Maciej
>
>--
>Jiaxun Yang

2020-04-07 08:07:57

by Jiaxun Yang

[permalink] [raw]
Subject: [PATCH v2] MIPS: Truncate load-y into 32bit for 32bit kernel

LLD failed to link vmlinux with 64bit load address for 32bit ELF
while bfd will strip 64bit address into 32bit silently.
To fix LLD build, we should truncate load address provided by platform
into 32bit for 32bit kernel.

Signed-off-by: Jiaxun Yang <[email protected]>
Reviewed-by: Fangrui Song <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>

--
V2: Take MaskRay's shell magic.
---
arch/mips/Makefile | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/mips/Makefile b/arch/mips/Makefile
index e1c44aed8156..f8fd3c39fb55 100644
--- a/arch/mips/Makefile
+++ b/arch/mips/Makefile
@@ -286,6 +286,9 @@ ifdef CONFIG_64BIT
$(error CONFIG_CPU_DADDI_WORKAROUNDS unsupported without -msym32)
endif
endif
+else
+ # Truncate address into 32-bit
+ load-y := 0x$(shell echo "$(load-y)" | rev | head -c 8 | rev)
endif

KBUILD_AFLAGS += $(cflags-y)
--
2.26.0.rc2


2020-04-07 17:23:49

by Nick Desaulniers

[permalink] [raw]
Subject: Re: [PATCH v2] MIPS: Truncate load-y into 32bit for 32bit kernel

On Tue, Apr 7, 2020 at 1:07 AM Jiaxun Yang <[email protected]> wrote:
>
> LLD failed to link vmlinux with 64bit load address for 32bit ELF
> while bfd will strip 64bit address into 32bit silently.
> To fix LLD build, we should truncate load address provided by platform
> into 32bit for 32bit kernel.
>
> Signed-off-by: Jiaxun Yang <[email protected]>
> Reviewed-by: Fangrui Song <[email protected]>
> Tested-by: Nathan Chancellor <[email protected]>
>
> --
> V2: Take MaskRay's shell magic.

V2 is way too clever, V1 was much more readable.

Can this tag be added to the commit to help us track when and where it lands?
Link: https://github.com/ClangBuiltLinux/linux/issues/786

> ---
> arch/mips/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/mips/Makefile b/arch/mips/Makefile
> index e1c44aed8156..f8fd3c39fb55 100644
> --- a/arch/mips/Makefile
> +++ b/arch/mips/Makefile
> @@ -286,6 +286,9 @@ ifdef CONFIG_64BIT
> $(error CONFIG_CPU_DADDI_WORKAROUNDS unsupported without -msym32)
> endif
> endif
> +else
> + # Truncate address into 32-bit
> + load-y := 0x$(shell echo "$(load-y)" | rev | head -c 8 | rev)
> endif
>
> KBUILD_AFLAGS += $(cflags-y)
> --

--
Thanks,
~Nick Desaulniers

2020-04-07 18:05:05

by Fangrui Song

[permalink] [raw]
Subject: Re: [PATCH v2] MIPS: Truncate load-y into 32bit for 32bit kernel

On 2020-04-07, Nick Desaulniers wrote:
>On Tue, Apr 7, 2020 at 1:07 AM Jiaxun Yang <[email protected]> wrote:
>>
>> LLD failed to link vmlinux with 64bit load address for 32bit ELF
>> while bfd will strip 64bit address into 32bit silently.
>> To fix LLD build, we should truncate load address provided by platform
>> into 32bit for 32bit kernel.
>>
>> Signed-off-by: Jiaxun Yang <[email protected]>
>> Reviewed-by: Fangrui Song <[email protected]>
>> Tested-by: Nathan Chancellor <[email protected]>
>>
>> --
>> V2: Take MaskRay's shell magic.
>
>V2 is way too clever, V1 was much more readable.

This is difficult:/
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_04
The POSIX shell is only required to do signed long integer arithmetic.
"signed long" can be 32-bit.

awk may not provide precision more than a double ("... decimal-floating-constant token as specified
by the ISO C standard")

% gawk 'BEGIN {printf("%x", (0xffffffff80101234 % 0x100000000))}' /dev/null
80101000

>Can this tag be added to the commit to help us track when and where it lands?
>Link: https://github.com/ClangBuiltLinux/linux/issues/786

And this tag for GNU ld enhancement:

Link: https://sourceware.org/bugzilla/show_bug.cgi?id=25784

>> ---
>> arch/mips/Makefile | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/mips/Makefile b/arch/mips/Makefile
>> index e1c44aed8156..f8fd3c39fb55 100644
>> --- a/arch/mips/Makefile
>> +++ b/arch/mips/Makefile
>> @@ -286,6 +286,9 @@ ifdef CONFIG_64BIT
>> $(error CONFIG_CPU_DADDI_WORKAROUNDS unsupported without -msym32)
>> endif
>> endif
>> +else
>> + # Truncate address into 32-bit
>> + load-y := 0x$(shell echo "$(load-y)" | rev | head -c 8 | rev)
>> endif
>>
>> KBUILD_AFLAGS += $(cflags-y)
>> --
>
>--
>Thanks,
>~Nick Desaulniers

2020-04-07 18:11:08

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH v2] MIPS: Truncate load-y into 32bit for 32bit kernel

On Tue, 7 Apr 2020, Nick Desaulniers wrote:

> V2 is way too clever, V1 was much more readable.

I think V2 is a step in the right direction except it still has some
issues, and also I'd simplify it as there's surely too much processing
there.

OTOH V1 is going to be a maintenance nightmare, as you need to handle all
platforms individually whether they want different 32-bit and 64-bit load
addresses or not.

> > diff --git a/arch/mips/Makefile b/arch/mips/Makefile
> > index e1c44aed8156..f8fd3c39fb55 100644
> > --- a/arch/mips/Makefile
> > +++ b/arch/mips/Makefile
> > @@ -286,6 +286,9 @@ ifdef CONFIG_64BIT
> > $(error CONFIG_CPU_DADDI_WORKAROUNDS unsupported without -msym32)
> > endif
> > endif
> > +else
> > + # Truncate address into 32-bit
> > + load-y := 0x$(shell echo "$(load-y)" | rev | head -c 8 | rev)

You cannot just truncate `load-y' in place like this as it will break
logic with `expr' used elsewhere in this Makefile (your original change
would too) that does a string comparison on this variable. So you need to
define another variable for the sole linker's use, like `load-ld'.

Then I think there's no need to invoke multiple programs, especially ones
we don't currently rely on (`rev'). How about:

load-ld = $(shell echo "$(load-y)" | sed 's/.\{8\}\(.\{8\}\)$/\1/')

Also this really needs to be placed elsewhere, as it has nothing to do
with KBUILD_SYM32 it has been attached to with this change, and explain
why it is done rather than what (it's obvious from the command it's meant
to truncate the address).

So use something along the lines of:

# When linking a 32-bit executable the LLVM linker cannot cope with a
# 32-bit load address that has been sign-extended to 64 bits. Simply
# remove the upper 32 bits then, as it is safe to do so with other
# linkers.
ifdef CONFIG_64BIT
load-ld = $(load-y)
else
load-ld = $(shell echo "$(load-y)" | sed 's/.\{8\}\(.\{8\}\)$/\1/')
endif

just above the use place, and then adjust the place to refer `load-ld'
rather than `load-y'.

Put the justification for this change (feel free to reuse observations I
made here), like why a new variable, in the change description.

Maciej

2020-04-10 09:08:10

by Jiaxun Yang

[permalink] [raw]
Subject: [PATCH v3] MIPS: Truncate link address into 32bit for 32bit kernel

LLD failed to link vmlinux with 64bit load address for 32bit ELF
while bfd will strip 64bit address into 32bit silently.
To fix LLD build, we should truncate load address provided by platform
into 32bit for 32bit kernel.

Signed-off-by: Jiaxun Yang <[email protected]>
Reviewed-by: Fangrui Song <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>

--
V2: Take MaskRay's shell magic.

V3: After spent an hour on dealing with special character issue in
Makefile, I gave up to do shell hacks and write a util in C instead.
Thanks Maciej for pointing out Makefile variable problem.
---
arch/mips/Makefile | 2 ++
arch/mips/kernel/Makefile | 11 ++++++++++-
arch/mips/kernel/vmlinux.lds.S | 2 +-
arch/mips/tools/.gitignore | 1 +
arch/mips/tools/Makefile | 5 +++++
arch/mips/tools/truncate32.c | 29 +++++++++++++++++++++++++++++
6 files changed, 48 insertions(+), 2 deletions(-)
create mode 100644 arch/mips/tools/truncate32.c

diff --git a/arch/mips/Makefile b/arch/mips/Makefile
index e1c44aed8156..633e9de4d262 100644
--- a/arch/mips/Makefile
+++ b/arch/mips/Makefile
@@ -14,6 +14,7 @@

archscripts: scripts_basic
$(Q)$(MAKE) $(build)=arch/mips/tools elf-entry
+ $(Q)$(MAKE) $(build)=arch/mips/tools truncate32
ifeq ($(CONFIG_CPU_LOONGSON3_WORKAROUNDS),y)
$(Q)$(MAKE) $(build)=arch/mips/tools loongson3-llsc-check
endif
@@ -261,6 +262,7 @@ include arch/mips/Kbuild.platforms
ifdef CONFIG_PHYSICAL_START
load-y = $(CONFIG_PHYSICAL_START)
endif
+export VMLINUX_LOAD_ADDRESS := $(load-y)

entry-y = $(shell $(objtree)/arch/mips/tools/elf-entry vmlinux)
cflags-y += -I$(srctree)/arch/mips/include/asm/mach-generic
diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile
index d6e97df51cfb..0178f7085317 100644
--- a/arch/mips/kernel/Makefile
+++ b/arch/mips/kernel/Makefile
@@ -112,4 +112,13 @@ obj-$(CONFIG_MIPS_CPC) += mips-cpc.o
obj-$(CONFIG_CPU_PM) += pm.o
obj-$(CONFIG_MIPS_CPS_PM) += pm-cps.o

-CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS)
+# When linking a 32-bit executable the LLVM linker cannot cope with a
+# 32-bit load address that has been sign-extended to 64 bits. Simply
+# remove the upper 32 bits then, as it is safe to do so with other
+# linkers.
+ifdef CONFIG_64BIT
+ load-ld = $(VMLINUX_LOAD_ADDRESS)
+else
+ load-ld = $(shell $(objtree)/arch/mips/tools/truncate32 $(VMLINUX_LOAD_ADDRESS))
+endif
+CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS) -DVMLINUX_LINK_ADDRESS=$(load-ld)
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
index a5f00ec73ea6..5226cd8e4bee 100644
--- a/arch/mips/kernel/vmlinux.lds.S
+++ b/arch/mips/kernel/vmlinux.lds.S
@@ -55,7 +55,7 @@ SECTIONS
/* . = 0xa800000000300000; */
. = 0xffffffff80300000;
#endif
- . = VMLINUX_LOAD_ADDRESS;
+ . = VMLINUX_LINK_ADDRESS;
/* read-only */
_text = .; /* Text and read-only data */
.text : {
diff --git a/arch/mips/tools/.gitignore b/arch/mips/tools/.gitignore
index 794817dfb389..58ead412c8d3 100644
--- a/arch/mips/tools/.gitignore
+++ b/arch/mips/tools/.gitignore
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
elf-entry
loongson3-llsc-check
+truncate32
diff --git a/arch/mips/tools/Makefile b/arch/mips/tools/Makefile
index b851e5dcc65a..69debb18bbb4 100644
--- a/arch/mips/tools/Makefile
+++ b/arch/mips/tools/Makefile
@@ -8,3 +8,8 @@ hostprogs += loongson3-llsc-check
PHONY += loongson3-llsc-check
loongson3-llsc-check: $(obj)/loongson3-llsc-check
@:
+
+hostprogs += truncate32
+PHONY += truncate32
+truncate32: $(obj)/truncate32
+ @:
diff --git a/arch/mips/tools/truncate32.c b/arch/mips/tools/truncate32.c
new file mode 100644
index 000000000000..82c19b4c32da
--- /dev/null
+++ b/arch/mips/tools/truncate32.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+__attribute__((noreturn))
+static void die(const char *msg)
+{
+ fputs(msg, stderr);
+ exit(EXIT_FAILURE);
+}
+
+int main(int argc, const char *argv[])
+{
+ unsigned long long val;
+
+ if (argc != 2)
+ die("Usage: truncate32 <address>\n");
+
+ val = strtoull(argv[1], NULL, 0);
+
+ if ((val & 0xffffffff00000000) != 0xffffffff00000000)
+ die("Invalid input\n");
+
+ val = val & 0xffffffff;
+ printf("0x%08llx\n", val);
+
+ return EXIT_SUCCESS;
+}
--
2.26.0.rc2

2020-04-10 20:46:43

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v3] MIPS: Truncate link address into 32bit for 32bit kernel

On Fri, Apr 10, 2020 at 05:06:23PM +0800, Jiaxun Yang wrote:
> LLD failed to link vmlinux with 64bit load address for 32bit ELF
> while bfd will strip 64bit address into 32bit silently.
> To fix LLD build, we should truncate load address provided by platform
> into 32bit for 32bit kernel.
>
> Signed-off-by: Jiaxun Yang <[email protected]>
> Reviewed-by: Fangrui Song <[email protected]>
> Tested-by: Nathan Chancellor <[email protected]>
>
> --
> V2: Take MaskRay's shell magic.
>
> V3: After spent an hour on dealing with special character issue in
> Makefile, I gave up to do shell hacks and write a util in C instead.
> Thanks Maciej for pointing out Makefile variable problem.
> ---
> arch/mips/Makefile | 2 ++
> arch/mips/kernel/Makefile | 11 ++++++++++-
> arch/mips/kernel/vmlinux.lds.S | 2 +-
> arch/mips/tools/.gitignore | 1 +
> arch/mips/tools/Makefile | 5 +++++
> arch/mips/tools/truncate32.c | 29 +++++++++++++++++++++++++++++
> 6 files changed, 48 insertions(+), 2 deletions(-)
> create mode 100644 arch/mips/tools/truncate32.c
>
> diff --git a/arch/mips/Makefile b/arch/mips/Makefile
> index e1c44aed8156..633e9de4d262 100644
> --- a/arch/mips/Makefile
> +++ b/arch/mips/Makefile
> @@ -14,6 +14,7 @@
>
> archscripts: scripts_basic
> $(Q)$(MAKE) $(build)=arch/mips/tools elf-entry
> + $(Q)$(MAKE) $(build)=arch/mips/tools truncate32
> ifeq ($(CONFIG_CPU_LOONGSON3_WORKAROUNDS),y)
> $(Q)$(MAKE) $(build)=arch/mips/tools loongson3-llsc-check
> endif
> @@ -261,6 +262,7 @@ include arch/mips/Kbuild.platforms
> ifdef CONFIG_PHYSICAL_START
> load-y = $(CONFIG_PHYSICAL_START)
> endif
> +export VMLINUX_LOAD_ADDRESS := $(load-y)
>
> entry-y = $(shell $(objtree)/arch/mips/tools/elf-entry vmlinux)
> cflags-y += -I$(srctree)/arch/mips/include/asm/mach-generic
> diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile
> index d6e97df51cfb..0178f7085317 100644
> --- a/arch/mips/kernel/Makefile
> +++ b/arch/mips/kernel/Makefile
> @@ -112,4 +112,13 @@ obj-$(CONFIG_MIPS_CPC) += mips-cpc.o
> obj-$(CONFIG_CPU_PM) += pm.o
> obj-$(CONFIG_MIPS_CPS_PM) += pm-cps.o
>
> -CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS)
> +# When linking a 32-bit executable the LLVM linker cannot cope with a
> +# 32-bit load address that has been sign-extended to 64 bits. Simply
> +# remove the upper 32 bits then, as it is safe to do so with other
> +# linkers.
> +ifdef CONFIG_64BIT
> + load-ld = $(VMLINUX_LOAD_ADDRESS)
> +else
> + load-ld = $(shell $(objtree)/arch/mips/tools/truncate32 $(VMLINUX_LOAD_ADDRESS))

This is major overkill. Just use the Makefile's builtin text
manipulation:

load-ld = $(subst 0xffffffff,0x,$(VMLINUX_LOAD_ADDRESS))

And note that a shell failure here would be entirely ignored, so the use
of die() in the proposed helper wouldn't actually stop a build, etc.

> +endif
> +CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS) -DVMLINUX_LINK_ADDRESS=$(load-ld)
> diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> index a5f00ec73ea6..5226cd8e4bee 100644
> --- a/arch/mips/kernel/vmlinux.lds.S
> +++ b/arch/mips/kernel/vmlinux.lds.S
> @@ -55,7 +55,7 @@ SECTIONS
> /* . = 0xa800000000300000; */
> . = 0xffffffff80300000;
> #endif
> - . = VMLINUX_LOAD_ADDRESS;
> + . = VMLINUX_LINK_ADDRESS;
> /* read-only */
> _text = .; /* Text and read-only data */
> .text : {
> diff --git a/arch/mips/tools/.gitignore b/arch/mips/tools/.gitignore
> index 794817dfb389..58ead412c8d3 100644
> --- a/arch/mips/tools/.gitignore
> +++ b/arch/mips/tools/.gitignore
> @@ -1,3 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0-only
> elf-entry
> loongson3-llsc-check
> +truncate32
> diff --git a/arch/mips/tools/Makefile b/arch/mips/tools/Makefile
> index b851e5dcc65a..69debb18bbb4 100644
> --- a/arch/mips/tools/Makefile
> +++ b/arch/mips/tools/Makefile
> @@ -8,3 +8,8 @@ hostprogs += loongson3-llsc-check
> PHONY += loongson3-llsc-check
> loongson3-llsc-check: $(obj)/loongson3-llsc-check
> @:
> +
> +hostprogs += truncate32
> +PHONY += truncate32

Isn't the special variable named ".PHONY"? (And also is that only for
things the don't get written to disk, but truncate32 is...)

> +truncate32: $(obj)/truncate32
> + @:
> diff --git a/arch/mips/tools/truncate32.c b/arch/mips/tools/truncate32.c
> new file mode 100644
> index 000000000000..82c19b4c32da
> --- /dev/null
> +++ b/arch/mips/tools/truncate32.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +__attribute__((noreturn))
> +static void die(const char *msg)
> +{
> + fputs(msg, stderr);
> + exit(EXIT_FAILURE);
> +}
> +
> +int main(int argc, const char *argv[])
> +{
> + unsigned long long val;
> +
> + if (argc != 2)
> + die("Usage: truncate32 <address>\n");
> +
> + val = strtoull(argv[1], NULL, 0);
> +
> + if ((val & 0xffffffff00000000) != 0xffffffff00000000)
> + die("Invalid input\n");
> +
> + val = val & 0xffffffff;
> + printf("0x%08llx\n", val);
> +
> + return EXIT_SUCCESS;
> +}
> --
> 2.26.0.rc2
>

--
Kees Cook

2020-04-10 23:41:54

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH v3] MIPS: Truncate link address into 32bit for 32bit kernel

On Fri, 10 Apr 2020, Kees Cook wrote:

> > diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile
> > index d6e97df51cfb..0178f7085317 100644
> > --- a/arch/mips/kernel/Makefile
> > +++ b/arch/mips/kernel/Makefile
> > @@ -112,4 +112,13 @@ obj-$(CONFIG_MIPS_CPC) += mips-cpc.o
> > obj-$(CONFIG_CPU_PM) += pm.o
> > obj-$(CONFIG_MIPS_CPS_PM) += pm-cps.o
> >
> > -CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS)
> > +# When linking a 32-bit executable the LLVM linker cannot cope with a
> > +# 32-bit load address that has been sign-extended to 64 bits. Simply
> > +# remove the upper 32 bits then, as it is safe to do so with other
> > +# linkers.
> > +ifdef CONFIG_64BIT
> > + load-ld = $(VMLINUX_LOAD_ADDRESS)
> > +else
> > + load-ld = $(shell $(objtree)/arch/mips/tools/truncate32 $(VMLINUX_LOAD_ADDRESS))
>
> This is major overkill. Just use the Makefile's builtin text
> manipulation:
>
> load-ld = $(subst 0xffffffff,0x,$(VMLINUX_LOAD_ADDRESS))

This looks like the best approach to me, thank you for the hint! And we
only ever want to strip 0xffffffff anyway. (I forgot about this function
of `make', doh!)

Maciej