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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> Why wasn't possible to share functions?
> Why using --orphan-handling option?
> 
> 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.