Re: [PATCH v1] xen/riscv: make calculation of stack address PC-relative

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2023 21:16, Oleksii wrote:
> On Tue, 2023-03-14 at 17:09 +0000, Andrew Cooper wrote:
>> On 14/03/2023 4:00 pm, Oleksii Kurochko wrote:
>>> The patch is needed to keep all addresses PC-relative.
>>>
>>> Pseudoinstruction 'la' can be transformed to 'auipc/addi' or
>>> 'auipc/l{w|d}'. It depends on the .option directive: nopic and pic.
>>>
>>> Right now, 'la' transforms to 'auipc/l{w|d}', which in case of
>>> cpu0_boot_stack[] will lead to the usage of _GLOBAL_OFFSET_TABLE_
>>> where all addresses will be without counting that it might happen
>>> that linker address != load address.
>>>
>>> To be sure that SP is loaded always PC-relative address
>>> 'la' should be changed to 'lla', which always transforms to
>>> 'auipc/addi'.
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>> ---
>>>  xen/arch/riscv/riscv64/head.S | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/riscv/riscv64/head.S
>>> b/xen/arch/riscv/riscv64/head.S
>>> index 8887f0cbd4..e12d2a7cf3 100644
>>> --- a/xen/arch/riscv/riscv64/head.S
>>> +++ b/xen/arch/riscv/riscv64/head.S
>>> @@ -27,7 +27,7 @@ ENTRY(start)
>>>          add     t3, t3, __SIZEOF_POINTER__
>>>          bltu    t3, t4, .L_clear_bss
>>>  
>>> -        la      sp, cpu0_boot_stack
>>> +        lla     sp, cpu0_boot_stack
>>
>> I don't think this is the appropriate way forward.  It's very much
>> smells like duct tape hiding the real bug.
>>
> As an option, I thought to add in head.S '.option nopic' directive to
> make la translated to auipc/addi [1] pair.
> As an alternative option, adds to AFLAGS += -fno-PIC... but still...
> 
> I checked in Linux binary how 'la' instruction is transformed, and it
> looks like it is translated as I expect to auipc/addi pair:
> ffffffe000001066: 00027517 auipc a0,0x27
> ffffffe00000106a: f9a50513 addi a0,a0,-102 # ffffffe000028000
> <early_pg_dir>
> 
> I checked compiler flags between Xen and Linux. The difference is in-
> fno-PIE (Linux also adds -mabi and -march to AFLAGS):
> 
> 1. Linux build command of head.S: riscv64-linux-gnu-gcc -Wp,-
> MD,arch/riscv/kernel/.head.o.d -nostdinc -isystem /usr/lib/gcc-
> cross/riscv64-linux-gnu/9/include -I./arch/riscv/include -
> I./arch/riscv/include/generated -I./include -I./arch/riscv/include/uapi
> -I./arch/riscv/include/generated/uapi -I./include/uapi -
> I./include/generated/uapi -include ./include/linux/kconfig.h -
> D__KERNEL__ -D__ASSEMBLY__ -fno-PIE -mabi=lp64 -march=rv64imafdc -c -o
> arch/riscv/kernel/head.o arch/riscv/kernel/head.S
> 
> 2. Xen build command of head.S:riscv64-linux-gnu-gcc -MMD -MP -MF
> arch/riscv/riscv64/.head.o.d -D__ASSEMBLY__ -Wa,--noexecstack -
> DBUILD_ID -fno-strict-aliasing -Wall -Wstrict-prototypes -Wdeclaration-
> after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs
> -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin -fno-common -Werror
> -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ -include
> ./include/xen/config.h -Wa,--strip-local-absolute -g -mabi=lp64 -
> I./include -I./arch/riscv/include -march=rv64gc -mstrict-align -
> mcmodel=medany - -c arch/riscv/riscv64/head.S -o
> arch/riscv/riscv64/head.o
> 
> So can we update AFLAGS in xen/arch/riscv/arch.mk with -fno-PIE or will
> it still be an incorrect fix?

Leaving aside the question of why you and I see different code being
generated, isn't it simply a matter of RISC-V, unlike Arm and x86,
not presently consuming EMBEDDED_EXTRA_CFLAGS in its arch.mk?

Jan