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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Fri, Oct 18, 2024 at 12:49 PM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Fri, Oct 18, 2024 at 09:42:48AM +0100, Frediano Ziglio wrote:
> > On Thu, Oct 17, 2024 at 6:13 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> > wrote:
> > >
> > > 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?
> > >
> >
> > You cut half of the explanation, replacing with nothing.
> > Why is a script needed? Why 2 linking? How the new method detects
> > unwanted relocations?
>
> TBH this is not clear to me even with your original commit message.
>

I'm starting to think that either me or Andrew are not the right
persons to write this, there's a lot of assumptions we assume for
granted.

>From what I see, in my message:
----
- 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;
----

in Andrew message:
----
Use differential linking and explicit imports/exports to confirm that
we only have the expected relocations,
----

probably both are cryptic, but getting from "differential linking" you
really need to know in advance what you are explaining.

> > Why wasn't possible to share functions?

mine:
----
- code is compiled separately, it's not possible to share a function
  (like memcpy) between different functions to use;
----

in Andrew message:
----
Link all 32bit objects together first.  This allows for sharing of
logic between translation units.
----

I would agree Andrew comment is clearer here.

> > Why using --orphan-handling option?

mine:
----
- --orphan-handling=error option to linker is used to make sure we
  account for all possible sections from C code;
---

in Andrew message:
----
----

still, Andrew more clear, but not carrying any information.

> >
> > The description has been there for about 2 months without many objections.
>
> IMO it's fine to use lists to describe specific points, but using
> lists exclusively to write a commit message makes the items feel
> disconnected between them.
>
> The format of the commit message by Andrew is clearer to undertsand
> for me.  Could you add what you think it's missing to the proposed
> message by Andrew?
>
> Thanks, Roger.

Probably, as said above, we (me and Andrew) have too many assumptions.
This commit is doing quite some magic that's not easy to grasp.
I can try to explain, and possibly you can suggest something that
makes sense also to people not too involved in this.

There are quite some challenges here. This code is executed during the
boot process and the environment is pretty uncommon. Simply writing a
message is not that easy. We are not sure if we have VGA, serial, BIOS
or UEFI. We are not executing in the location code was compiled/linked
to run so assuming it is wrong and causes issue; in other word the
code/data should be position independent. This code is meant to be
compiled and run in 32 mode, however Xen is 64 bit, so compiler output
can't be linked to the final executable. And obviously you cannot call
64 bit code from 32 bit code. Even if code would be compiled and run
in 64 bit mode, calling functions like printk would probably crash (it
does, we discovered recently) as Xen code assumes some environment
settings (in case of printk, just as example, it was missing per-cpu
info leading to pointer to garbage). In 32 bit mode, you can get code
position independence with -fpic, but this requires linker to generate
GOT table but 64 bit linker would generate that table in a position
not compatible with 32 bit code (and that's not the only issue of
using 64 bit linker on 32 bit code). So, to solve this code/data is
linked to a 32 bit object and converted to binary (note: this is an
invariant, it was done like that before and after this commit). Solved
the code with -fpic, what about data? Say we have something like
"const char *message = "hello";"... a pointer is a pointer, which is
the location of the data pointed. But as said before, we are in an
uncommon environment where code/data could be run at a different
location than compiled/linked! There's no magic option for doing that,
so instead of hoping for the best (as we are doing at the moment) we
check to not have pointers. How? We link code+data at different
locations which will generate different binary in that case and we
compare the 2 binaries to make sure there are no such differences
(well, this is a simplification of the process).


So... somebody willing to translate the above, surely poor and
unclear, explanation is somethig digestible by people less involved in
it?

Frediano