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

[Jan Beulich <jbeulich@xxxxxxxx>]

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