| Message ID | ad2249c1b5be01f99ef9c294a3264da0c9715bab.1678809641.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] xen/riscv: make calculation of stack address PC-relative | expand |
On Tue, Mar 14, 2023 at 06:00:41PM +0200, 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@gmail.com> > --- > 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 > li t0, STACK_SIZE > add sp, sp, t0 > > -- > 2.39.2 > > Reviewed-by: Bobby Eshleman <bobbyeshleman@gmail.com>
On 14.03.2023 17:00, 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}', I'm afraid I cannot confirm this. Is it possible that your and my gas are configured differently (i.e. can the default of nopic vs pic vary)? > 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'. While I agree with the adjustment in principle, I'd like to raise the question of the suitablity of such macro insns for purposes like the one here. In principle I would assume it would be better if sp was written only once, with the final value. That would then also remove the dependency on a possibly differing default gas may be using. Jan
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@gmail.com> > --- > 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. ~Andrew
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@gmail.com> > > --- > > 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
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@gmail.com> >>> --- >>> 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? Jan
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). Jan
On Wed, 2023-03-15 at 08:59 +0100, 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? I am not sure that I know how to check specifically if -fpic flag passes to gas. Could you please tell me? > 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".) it looks like it should be by default -fpic because if look at gcc spec file for the RISC-V architecture: [user@49295ae49cbe build]$ riscv64-linux-gnu-gcc -dumpspecs | grep -i pic --traditional-format %(subtarget_asm_debugging_spec) %{fno-pie|fno- PIE|fno-pic|fno-PIC:;:-fpic} %{march=*} %{mabi=*} %{mno-relax} %{mbig- endian} %{mlittle-endian} %(subtarget_asm_spec)%{misa-spec=*} which means that -fpic is enabled if none of the following options are present on the command line: -fno-pie -fno-PIE -fno-pic -fno-PIC > > You gcc may have been configured with --enable-default-pie, while I > know mine hasn't been (simply because that's the default). You are right my gcc is configured with --enable-default-pie: [user@49295ae49cbe build]$ riscv64-linux-gnu-gcc -v Using built-in specs. COLLECT_GCC=riscv64-linux-gnu-gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/riscv64-linux-gnu/12.2.0/lto-wrapper Target: riscv64-linux-gnu Configured with: /build/riscv64-linux-gnu-gcc/src/gcc-12.2.0/configure --prefix=/usr --program-prefix=riscv64-linux-gnu- --with-local- prefix=/usr/riscv64-linux-gnu --with-sysroot=/usr/riscv64-linux-gnu -- with-build-sysroot=/usr/riscv64-linux-gnu --libdir=/usr/lib -- libexecdir=/usr/lib --target=riscv64-linux-gnu --host=x86_64-pc-linux- gnu --build=x86_64-pc-linux-gnu --with-system-zlib --with-isl --with- linker-hash-style=gnu --disable-nls --disable-libunwind-exceptions -- disable-libstdcxx-pch --disable-libssp --disable-multilib --disable- werror --enable-languages=c,c++ --enable-shared --enable-threads=posix --enable-__cxa_atexit --enable-clocale=gnu --enable-gnu-unique-object - -enable-linker-build-id --enable-lto --enable-plugin --enable-install- libiberty --enable-gnu-indirect-function --enable-default-pie --enable- checking=release Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 12.2.0 (GCC) So should we pass to CFLAGS and AFLAGS at least for RISC-V -fno-PIE? ~ Oleksii
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@gmail.com> > > > > --- > > > > 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
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@gmail.com> > > > > --- > > > > 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? ~ Oleksii
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. ~Andrew
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 but why we can't go with - fno-pie as it is done for other architectures: 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? ~ Oleksii
On 15.03.2023 19:23, Oleksii wrote: > On Wed, 2023-03-15 at 08:59 +0100, 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? > I am not sure that I know how to check specifically if -fpic flag > passes to gas. > Could you please tell me? Just to answer this question here (the other aspect fit better elsewhere): You can pass -v to gcc to make it report what options it invokes other tools (including gas) with. Jan
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@gmail.com> >>>>> --- >>>>> 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
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@gmail.com> >>>>> --- >>>>> 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
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? > 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. Jan
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
> > > > 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. I have question about 'SIZEOF(.got.plt) == 3 * 8': #ifndef EFI ASSERT(!SIZEOF(.got), ".got non-empty") /* * At least GNU ld 2.30 and earlier fail to discard the generic part of * .got.plt when no actual entries were allocated. Permit this case alongside * the section being empty. */ ASSERT(!SIZEOF(.got.plt) || SIZEOF(.got.plt) == 3 * 8, "unexpected .got.plt size") ASSERT(!SIZEOF(.igot.plt), ".igot.plt non-empty") ASSERT(!SIZEOF(.iplt), ".iplt non-empty") ASSERT(!SIZEOF(.plt), ".plt non-empty") ASSERT(!SIZEOF(.rela), "leftover relocations") #endif I assume that the check 'SIZEOF(.got.plt) == 3 * 8' was added to verify the case when no real entries in .got.plt are needed but .got.plt still has 3 entries. I commented the code where got entries are produced now: --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -20,6 +20,7 @@ ENTRY(start) csrc CSR_SSTATUS, t0 /* Clear the BSS */ +/* la t3, __bss_start la t4, __bss_end .L_clear_bss: @@ -30,5 +31,6 @@ ENTRY(start) la sp, cpu0_boot_stack li t0, STACK_SIZE add sp, sp, t0 +*/ tail start_xen And I can't see .got.plt with 3 entries: $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i got What am I doing wrong? Or my understanding of the idea of the check is wrong? And I assume that add !SIZEOF(.got) and !SIZEOF(.got.plt) would be enough to RISC-V's xen.lds.S? ~ Oleksii
On 16.03.2023 10:52, Oleksii wrote: >>> >>> 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. > > I have question about 'SIZEOF(.got.plt) == 3 * 8': > > #ifndef EFI > ASSERT(!SIZEOF(.got), ".got non-empty") > /* > * At least GNU ld 2.30 and earlier fail to discard the generic part of > * .got.plt when no actual entries were allocated. Permit this case > alongside > * the section being empty. > */ > ASSERT(!SIZEOF(.got.plt) || SIZEOF(.got.plt) == 3 * 8, > "unexpected .got.plt size") > ASSERT(!SIZEOF(.igot.plt), ".igot.plt non-empty") > ASSERT(!SIZEOF(.iplt), ".iplt non-empty") > ASSERT(!SIZEOF(.plt), ".plt non-empty") > ASSERT(!SIZEOF(.rela), "leftover relocations") > #endif > > I assume that the check 'SIZEOF(.got.plt) == 3 * 8' was added to verify > the case when no real entries in .got.plt are needed but .got.plt still > has 3 entries. > > I commented the code where got entries are produced now: > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -20,6 +20,7 @@ ENTRY(start) > csrc CSR_SSTATUS, t0 > > /* Clear the BSS */ > +/* > la t3, __bss_start > la t4, __bss_end > .L_clear_bss: > @@ -30,5 +31,6 @@ ENTRY(start) > la sp, cpu0_boot_stack > li t0, STACK_SIZE > add sp, sp, t0 > +*/ > > tail start_xen > > And I can't see .got.plt with 3 entries: > $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i got > > What am I doing wrong? Or my understanding of the idea of the check is > wrong? Did you read the comment next to it as to only older binutils being affected? I assume you use something newer than 2.30. Furthermore that specific behavior may very well have been x86-specific in the first place. So ... > And I assume that add !SIZEOF(.got) and !SIZEOF(.got.plt) would be > enough to RISC-V's xen.lds.S? ... quite likely: Yes. Jan
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 li t0, STACK_SIZE add sp, sp, t0
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@gmail.com> --- xen/arch/riscv/riscv64/head.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)