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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-03-16 at 09:45 +0100, Jan Beulich wrote:
> On 16.03.2023 08:42, Oleksii wrote:
> > On Wed, 2023-03-15 at 21:12 +0000, Andrew Cooper wrote:
> > > On 15/03/2023 7:59 am, Jan Beulich wrote:
> > > > On 14.03.2023 21:16, Oleksii wrote:
> > > > > 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
> > > > Looking into why you see different code generated than I:
> > > > Nothing
> > > > in
> > > > here directs gcc to pass -fpic to gas; in upstream gcc
> > > > (consistent
> > > > from gcc7 through gcc12, which are the versions I've checked;
> > > > the
> > > > actual range may be wider) there is
> > > > 
> > > > #define ASM_SPEC "\
> > > > %(subtarget_asm_debugging_spec) \
> > > > %{" FPIE_OR_FPIC_SPEC ":-fpic} \
> > > > ...
> > > > 
> > > > Can you check whether your gcc passes -fpic to gas even when
> > > > there's
> > > > no -fPIC / -fPIE (or alike) on the gcc command line? Or whether
> > > > your
> > > > gas (unlike upstream's) defaults to PIC mode? (For .S files
> > > > ASM_SPEC
> > > > is all that counts. For .c files gcc is redundantly passing -
> > > > fpic
> > > > along with also emitting ".option pic" or, in the opposite
> > > > case, it
> > > > is omitting -fpic along with emitting ".option nopic".)
> > > > 
> > > > You gcc may have been configured with --enable-default-pie,
> > > > while I
> > > > know mine hasn't been (simply because that's the default).
> > > 
> > > From the thread, the difference is clearly around the pie option,
> > > but
> > > I
> > > have to admit that I'm confused.
> > > 
> > > With GCC 10 from Debian repos and current staging (modulo the
> > > build
> > > fix), we end up with:
> > > 
> > > 0000000080200000 <_start>:
> > >     80200000:   10401073                csrw    sie,zero
> > >     80200004:   00002117                auipc   sp,0x2
> > >     80200008:   00413103                ld      sp,4(sp) #
> > > 80202008
> > > <_GLOBAL_OFFSET_TABLE_+0x8>
> > >     8020000c:   6285                    lui     t0,0x1
> > >     8020000e:   9116                    add     sp,sp,t0
> > >     80200010:   7f10206f                j       80203000
> > > <start_xen>
> > > 
> > > In this case, the auipc/ld pair makes a PC-relative reference
> > > into
> > > the
> > > GOT, but the pointer spilled into the GOT is the link time
> > > address of
> > > cpu0_boot_stack.
> > > 
> > > For the executable as a whole, we've got:
> > > 
> > > [ 6] .got              PROGBITS        0000000080202000 003000
> > > 000010
> > > 08  WA  0   0  8
> > > [ 7] .got.plt          PROGBITS        0000000080202010 003010
> > > 000010
> > > 08  WA  0   0  8
> > > 
> > > i.e. both nonzero in size, so presumably with expectations of
> > > something
> > > else to fix up the references.
> > > 
> > > I suspect we want to extend the x86 section asserts into the
> > > other
> > > architectures too, alongside figuring out how exactly to disable
> > > code
> > > generation of this form.
> > > 
> > But AFAIU it is expected that it will use GOT sections with the
> > link
> > time address of cpu0_boot_stack inside them because of pie option.
> > 
> > If we need to work with pie option that we can fix all address in
> > .got{.plt} somewhere at the start of head.S
> 
> While .got is very sensible in "normal" binaries, I think its use
> should
> be avoided in kernels and alike.
> 
> > but why we can't go with -
> > fno-pie as it is done for other architectures:
> 
> Why do you ask this repeatedly when the suggestion was to actually
> use EMBEDDED_EXTRA_CFLAGS?
Sorry for that.
I will update the current one patch with EMBEDDED_EXTRA_CFLAGS and back
'lla' to 'la'.
> 
> > Config.mk:
> >         EMBEDDED_EXTRA_CFLAGS := -fno-pie -fno-stack-protector -
> > fno-
> > stack-protector-all
> > EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-
> > tables
> > 
> > arch.mk:
> >     $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > 
> > 
> > Could you please explain what is x86 section asserts?
> 
> If you look at the bottom of x86's xen.lds.S you'll find a number of
> assertions, among them one towards .got being empty. Some of the
> sections checked there may indeed not be applicable on arbitrary
> architectures, but I think .got is sufficiently universal. So I agree
> with Andrew that it may be worthwhile making some of this generic.
> 
Thanks for the clarification.

~ Oleksii