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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2023 19:25, 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?
> Why don't we should see different code?

I consider it an indication of a problem if assembly code like this
differs depending on the tool chain used. It suggests (and as we can
see this is indeed the case here) that we're depending on some
defaults that we better wouldn't depend on.

> Do you use CONTAINER=riscv64 to build RISC-V Xen?
> If yes, probably, we see different code because you have more up-to-
> date CONTAINER. I am using CONTAINER_NO_PULL=1 for a long time so it
> might happen that we have different gcc version.

Just to clarify: I'm using a compiler I built myself, and I'm doing the
builds locally, without involving any containers.

Jan