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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2023 19:33, Oleksii wrote:
> On Wed, 2023-03-15 at 08:35 +0100, Jan Beulich wrote:
>> 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?
> No, it doesn't.
> 
> Could we consider cosuming EMBEDDED_EXTRA_CFALGS as a solution?

Well, that what I did suggest. Unless there are RISC-V-specific reasons
not to. (I have to admit that I don't really see why we leave this to
every arch, instead of doing this somewhere in a common makefile. Same
for the passing of -Wnested-externs.)

Jan