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

[Maciej W. Rozycki]

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