xen/arch/riscv/riscv64/head.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.2On 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:
> 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 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 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
> >
> > 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
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
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 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 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 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 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 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 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 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
© 2016 - 2024 Red Hat, Inc.