Re: [PATCH 2/3] x86/boot: Reuse code to relocate trampoline

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/10/2024 9:02 am, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> ---
>  xen/arch/x86/boot/Makefile           |  6 +++---
>  xen/arch/x86/boot/build32.lds.S      |  5 +++++
>  xen/arch/x86/boot/head.S             | 23 +--------------------
>  xen/arch/x86/boot/reloc-trampoline.c | 30 ++++++++++++++++++++++++++++
>  xen/arch/x86/efi/efi-boot.h          | 15 ++------------
>  5 files changed, 41 insertions(+), 38 deletions(-)
>  create mode 100644 xen/arch/x86/boot/reloc-trampoline.c
>
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index da87179fef..c16c4a8595 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,6 +1,6 @@
> -obj-bin-y += head.o cbundle.o
> +obj-bin-y += head.o cbundle.o reloc-trampoline.x64.o

Ah.  I think the $(obj)/%.x64.o rule you had in the previous patch wants
introducing here.

That said, x64 is the one name for 64bit that we reliably don't use. 
Also...

> -head-bin-objs := cmdline.o reloc.o
> +head-bin-objs := cmdline.o reloc.o reloc-trampoline.o

... head-bin-objs isn't really correct now seeing as they're not
binaries in head.S.  Also ...

>  nocov-y   += $(head-bin-objs)
>  noubsan-y += $(head-bin-objs)

The no$(foo)'s needs extending to the 64bit objects too.  They're also
used early enough to explode.

In Xen, 64bit objects are the norm, and it's 32bit ones which are the
exception, so how about we special case *.i386.o instead.  Then

obj32 := cmdline.i386.o
obj32 += reloc.i386.o
obj32 += reloc-trampoline.i386.o

and apply no$(foo) over everything.

> @@ -43,7 +43,7 @@ $(obj)/cbundle.o: $(head-bin-objs) $(obj)/build32.other.lds 
> $(obj)/build32.final
>       $(PYTHON) $(srctree)/tools/combine_two_binaries \
>               --script $(obj)/build32.final.lds \
>               --bin1 $@.1.bin --bin2 $@.2.bin \
> -             --map $(obj)/cbundle.map --exports cmdline_parse_early,reloc \
> +             --map $(obj)/cbundle.map --exports 
> cmdline_parse_early,reloc,reloc_trampoline32 \
>               --section-header '.section .init.text, "ax", @progbits' \
>               --output $(obj)/cbundle.s
>       $(CC) -c $(obj)/cbundle.s -o $@.tmp
> diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
> index fe422e3d25..2d10a75fb1 100644
> --- a/xen/arch/x86/boot/build32.lds.S
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -43,6 +43,11 @@ SECTIONS
>           * Potentially they should be all variables. */
>          DECLARE_IMPORT(__base_relocs_start);
>          DECLARE_IMPORT(__base_relocs_end);
> +        DECLARE_IMPORT(__trampoline_rel_start);
> +        DECLARE_IMPORT(__trampoline_rel_stop);
> +        DECLARE_IMPORT(__trampoline_seg_start);
> +        DECLARE_IMPORT(__trampoline_seg_stop);
> +        DECLARE_IMPORT(trampoline_phys);
>          . = . + GAP;
>          *(.text)
>          *(.text.*)
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index e0776e3896..ade2c5c43d 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -706,28 +706,7 @@ trampoline_setup:
>          mov     %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
>  
>          /* Apply relocations to bootstrap trampoline. */
> -        mov     sym_esi(trampoline_phys), %edx
> -        lea     sym_esi(__trampoline_rel_start), %edi
> -        lea     sym_esi(__trampoline_rel_stop), %ecx
> -1:
> -        mov     (%edi), %eax
> -        add     %edx, (%edi, %eax)
> -        add     $4,%edi
> -
> -        cmp     %ecx, %edi
> -        jb      1b
> -
> -        /* Patch in the trampoline segment. */
> -        shr     $4,%edx
> -        lea     sym_esi(__trampoline_seg_start), %edi
> -        lea     sym_esi(__trampoline_seg_stop), %ecx
> -1:
> -        mov     (%edi), %eax
> -        mov     %dx, (%edi, %eax)
> -        add     $4,%edi
> -
> -        cmp     %ecx, %edi
> -        jb      1b
> +        call    reloc_trampoline32
>  
>          /* Do not parse command line on EFI platform here. */
>          cmpb    $0, sym_esi(efi_platform)
> diff --git a/xen/arch/x86/boot/reloc-trampoline.c 
> b/xen/arch/x86/boot/reloc-trampoline.c
> new file mode 100644
> index 0000000000..9509dfa28a
> --- /dev/null
> +++ b/xen/arch/x86/boot/reloc-trampoline.c
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/compiler.h>
> +#include <xen/stdint.h>
> +#include <asm/trampoline.h>
> +
> +extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[];
> +extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[];
> +
> +#if defined(__i386__)
> +void reloc_trampoline32(void)
> +#elif defined (__x86_64__)
> +void reloc_trampoline64(void)
> +#else
> +#error Unknow architecture
> +#endif
> +{
> +    unsigned long phys = trampoline_phys;
> +    const int32_t *trampoline_ptr;
> +
> +    /* Apply relocations to trampoline. */

Please take the opportunity to expand on this.

/*
 * Apply relocations to trampoline.
 *
 * This modifies the trampoline in place within Xen, so that it will
 * operate correctly when copied into place.
 */

This property is entirely non-obvious, and what is is hurting us trying
to make R^X work.

But - it also makes it clean how to fix the problem (later, after you've
unified the relocation paths with this patch).

> +    for ( trampoline_ptr = __trampoline_rel_start;
> +          trampoline_ptr < __trampoline_rel_stop;
> +          ++trampoline_ptr )
> +        *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;

Newline here please.

> +    for ( trampoline_ptr = __trampoline_seg_start;
> +          trampoline_ptr < __trampoline_seg_stop;
> +          ++trampoline_ptr )
> +        *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
> +}

I was looking at the code generation.  32bit looks fine, but 64bit is
outragous.

It turns out I have ubsan enabled in this build, and the code generation
is much better with nocov-y extended to cover reloc-trampoline.x64.o

~Andrew