Re: [PATCH v6 1/5] x86/boot: create a C bundle for 32 bit boot code and use it

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/10/2024 2:31 pm, Frediano Ziglio wrote:
> The current method to include 32 bit C boot code is:
> - compile each function we want to use into a separate object file;
> - each function is compiled with -fpic option;
> - convert these object files to binary files. This operation removes GOP
>   which we don't want in the executable;
> - a small assembly part in each file add the entry point;
> - code can't have external references, all possible variables are passed
>   by value or pointer;
> - include these binary files in head.S.
>
> There are currently some limitations:
> - code is compiled separately, it's not possible to share a function
>   (like memcpy) between different functions to use;
> - although code is compiled with -fpic there's no certainty there are
>   no relocations, specifically data ones. This can lead into hard to
>   find bugs;
> - it's hard to add a simple function;
> - having to pass external variables makes hard to do multiple things
>   otherwise functions would require a lot of parameters so code would
>   have to be split into multiple functions which is not easy.
>
> Current change extends the current process:
> - all object files are linked together before getting converted making
>   possible to share code between the function we want to call;
> - a single object file is generated with all functions to use and
>   exported symbols to easily call;
> - variables to use are declared in linker script and easily used inside
>   C code. Declaring them manually could be annoying but makes also
>   easier to check them. Using external pointers can be still an issue if
>   they are not fixed. If an external symbol is not declared this gives a
>   link error.
>
> Some details of the implementation:
> - C code is compiled with -fpic flags (as before);
> - object files from C code are linked together;
> - the single bundled object file is linked with 2 slightly different
>   script files to generate 2 bundled object files;
> - the 2 bundled object files are converted to binary removing the need
>   for global offset tables;
> - a Python script is used to generate assembly source from the 2
>   binaries;
> - the single assembly file is compiled to generate final bundled object
>   file;
> - to detect possible unwanted relocation in data/code code is generated
>   with different addresses. This is enforced starting .text section at
>   different positions and adding a fixed "gap" at the beginning.
>   This makes sure code and data is position independent;
> - to detect used symbols in data/code symbols are placed in .text
>   section at different offsets (based on the line in the linker script).
>   This is needed as potentially a reference to a symbol is converted to
>   a reference to the containing section so multiple symbols could be
>   converted to reference to same symbol (section name) and we need to
>   distinguish them;
> - --orphan-handling=error option to linker is used to make sure we
>   account for all possible sections from C code;
>
> Current limitations:
> - the main one is the lack of support for 64 bit code. It would make
>   sure that even the code used for 64 bit (at the moment EFI code) is
>   code and data position independent. We cannot assume that code that
>   came from code compiled for 32 bit and compiled for 64 bit is code and
>   data position independent, different compiler options lead to
>   different code/data.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

This commit message is not particularly easy to follow.  Can I recommend
the following:

---%<---
x86/boot: Rework how 32bit C is linked/included for early boot

Right now, the two functions which were really too complicated to write
in asm are compiled as 32bit PIC, linked to a blob and included
directly, using global asm() to arrange for them to have function semantics.

This is limiting and fragile; the use of data relocations will compile
fine but malfunction when used, creating hard-to-debug bugs.

Furthermore, we would like to increase the amount of C, to
deduplicate/unify Xen's boot logic, as well as making it easier to
follow.  Therefore, rework how the 32bit objects are included.

Link all 32bit objects together first.  This allows for sharing of logic
between translation units.  Use differential linking and explicit
imports/exports to confirm that we only have the expected relocations,
and write the object back out as an assembly file so it can be linked
again as if it were 64bit, to integrate with the rest of Xen.

This allows for the use of external references (e.g. access to global
variables) with reasonable assurance of doing so safely.

No functional change.
---%<---

which I think is an accurate and more concise summary of what's changing?

> diff --git a/xen/arch/x86/boot/.gitignore b/xen/arch/x86/boot/.gitignore
> index a379db7988..7e85549751 100644
> --- a/xen/arch/x86/boot/.gitignore
> +++ b/xen/arch/x86/boot/.gitignore
> @@ -1,3 +1,4 @@
>  /mkelf32
> -/*.bin
> -/*.lnk
> +/build32.*.lds
> +/built-in-32.*.bin
> +/built-in-32.*.map

/built-in-32.S too

And from a glance at the file, this adjustment in the combine script too:

-print('\n\t.section\t.note.GNU-stack,"",@progbits', file=out)
+print('\n\t.section .note.GNU-stack, "", @progbits', file=out)

to have both .section's formatted in the same way.


> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> similarity index 70%
> rename from xen/arch/x86/boot/build32.lds
> rename to xen/arch/x86/boot/build32.lds.S
> index 56edaa727b..e3f5e55261 100644
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds.S
> <snip>
>          *(.text)
>          *(.text.*)
> -        *(.data)
> -        *(.data.*)
>          *(.rodata)
>          *(.rodata.*)
> +        *(.data)
> +        *(.data.*)

Reordering .data and .rodata really isn't necessary.

I'd just drop this part of the diff.  I have some different follow-up
for it anyway, which I've been holding off until after this first change
is sorted.

Everything here I'm happy to fix up on commit, if you're ok with me
doing so.

~Andrew