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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/10/2024 9:02 am, Frediano Ziglio wrote:
> diff --git a/.gitignore b/.gitignore
> index d88be80b28..d538c6adb1 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -247,11 +247,10 @@ xen/.config
>  xen/.config.old
>  xen/.xen.elf32
>  xen/System.map
> -xen/arch/x86/boot/mkelf32
> +xen/arch/x86/boot/build32.*.lds
>  xen/arch/x86/boot/cmdline.S
> +xen/arch/x86/boot/mkelf32
>  xen/arch/x86/boot/reloc.S
> -xen/arch/x86/boot/*.bin
> -xen/arch/x86/boot/*.lnk

cmdline.S and reloc.S are stale too.  They're from an even older form of
this bodge (c/s 1ab7c128d9d1).

However, we're trying to move away from one global .gitignore file. 
Would you mind deleting all of these entries, and making a new
xen/arch/x86/boot/.gitignore with just the ones that you need.  (c/s
0a15b7695bd98 is an example of cleaning up the tools/ocaml side of things).

Feel free to make it a prep patch if you want.  This one is complicated
enough.

> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index ff0f965876..da87179fef 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,4 @@
> -obj-bin-y += head.o
> +obj-bin-y += head.o cbundle.o

+obj-bin-y += cbundle.o
 obj-bin-y += head.o

please.  Putting things on a single line is already getting unwieldy by
patch 2.

I have to admit that I'm still not convinced of "cbundle" as a name, but
I can't think of anything better either.

>  
>  head-bin-objs := cmdline.o reloc.o
>  
> @@ -9,7 +9,6 @@ targets   += $(head-bin-objs)
>  head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
>  
>  $(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> -$(obj)/head.o: $(head-bin-objs:.o=.bin)
>  
>  CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
>  $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> @@ -24,10 +23,29 @@ $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
>  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := 
> --no-warn-rwx-segments
>  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
>  
> -%.bin: %.lnk
> -     $(OBJCOPY) -j .text -O binary $< $@
> -
> -%.lnk: %.o $(src)/build32.lds
> -     $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) 
> -o $@ $<
> -
> -clean-files := *.lnk *.bin
> +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S
> +     $(call if_changed_dep,cpp_lds_S)
> +
> +$(obj)/%.x64.o: $(src)/%.c FORCE
> +     $(call if_changed_rule,cc_o_c)
> +
> +$(obj)/cbundle.o: $(head-bin-objs) $(obj)/build32.other.lds 
> $(obj)/build32.final.lds
> +## link all object files together
> +     $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -r -o $(obj)/cbundle.tmp.o 
> $(head-bin-objs)
> +## link twice with 2 different layouts
> +     $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) --orphan-handling=error -N 
> -T $(obj)/build32.other.lds -o $@.1.o $(obj)/cbundle.tmp.o
> +     $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) --orphan-handling=error -N 
> -T $(obj)/build32.final.lds -Map $(obj)/cbundle.map -o $@.2.o 
> $(obj)/cbundle.tmp.o

Looking at this, I think we want an $(LD32) variable.  Also,
--orphan-handling isn't supported by all supported linkers, so you'll
have to make it conditional.  See how we handle =warn in the general case.


As a tangent, I know we have this form in the main Xen link, but I have
the same complaint there.  This is a serial operation that could be less
serial.

AFAICT, you link everything together into cbundle.tmp, then do two
(parallelisable) relink+obcopy, then run the python script.  Would it be
possible to split it into 4(?) rules.

> +## extract binaries from them
> +     $(OBJCOPY) -j .text -O binary $@.1.o $@.1.bin
> +     $(OBJCOPY) -j .text -O binary $@.2.o $@.2.bin
> +## generate final assembly file combining and checking above binaries
> +     $(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 \
> +             --section-header '.section .init.text, "ax", @progbits' \
> +             --output $(obj)/cbundle.s
> +     $(CC) -c $(obj)/cbundle.s -o $@.tmp
> +     rm -f $(obj)/cbundle.tmp.o $@.1.o $@.2.o $@.1.bin $@.2.bin 
> $(obj)/cbundle.map $(obj)/cbundle.s $@
> +     mv $@.tmp $@
> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> similarity index 63%
> rename from xen/arch/x86/boot/build32.lds
> rename to xen/arch/x86/boot/build32.lds.S
> index 56edaa727b..fe422e3d25 100644
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -15,22 +15,54 @@
>   * with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -ENTRY(_start)
> +#undef ENTRY

The need for this undef should have gone away with c/s b102c9f1ce29.

> +
> +#ifdef FINAL
> +# define GAP 0
> +# define MULT 0
> +# define TEXT_START
> +#else
> +# define GAP 0x010200
> +# define MULT 1
> +# define TEXT_START 0x408020
> +#endif
> +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
> +
> +ENTRY(dummy_start)
>  
>  SECTIONS
>  {
> -  /* Merge code and data into one section. */
> -  .text : {
> +  /* Merge code and read-only data into one section. */
> +  .text TEXT_START : {
> +        /* Silence linker warning, we are not going to use it */
> +        dummy_start = .;
> +
> +        /* Declare below any symbol name needed.
> +         * Each symbol should be on its own line.
> +         * It looks like a tedious work but we make sure the things we use.
> +         * Potentially they should be all variables. */
> +        DECLARE_IMPORT(__base_relocs_start);
> +        DECLARE_IMPORT(__base_relocs_end);
> +        . = . + GAP;

I have to admit that I've got lost here trying to follow what's going on.

The linker script is emitting symbols based on line number that are
either adjacent or all the same (depending on MULT), and these names are
also parsed by the python script ?

I also note that running this patch:

$ make -s -j8 build
 __  __            _  _    ____   ___                     _        _    
_      
 \ \/ /___ _ __   | || |  |___ \ / _ \    _   _ _ __  ___| |_ __ _| |__
| | ___
  \  // _ \ '_ \  | || |_   __) | | | |__| | | | '_ \/ __| __/ _` | '_
\| |/ _ \
  /  \  __/ | | | |__   _| / __/| |_| |__| |_| | | | \__ \ || (_| | |_)
| |  __/
 /_/\_\___|_| |_|    |_|(_)_____|\___/    \__,_|_|
|_|___/\__\__,_|_.__/|_|\___|
                                                                              
 
Position 0x70c 50 trampoline_phys
Position 0x87e 51 boot_vid_info
Position 0xade 51 boot_vid_info
Position 0xb92 51 boot_vid_info
Position 0xbae 50 trampoline_phys
Position 0xbb4 46 __trampoline_rel_start
Position 0xbba 47 __trampoline_rel_stop
Position 0xbc6 47 __trampoline_rel_stop
Position 0xbe2 48 __trampoline_seg_start
Position 0xbe8 49 __trampoline_seg_stop
Position 0xbf8 49 __trampoline_seg_stop
$

which looks like debugging leaking out to stderr.

~Andrew