[lkml]   [2020]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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

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)
load-ld = $(shell echo "$(load-y)" | sed 's/.\{8\}\(.\{8\}\)$/\1/')

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.


 \ /
  Last update: 2020-04-07 20:11    [W:0.078 / U:3.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site