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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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?

[1]
https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md#pseudoinstructions

~ Oleksii