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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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?

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.


~ Oleksii