Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 1199291d2b..23ad274c89 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,5 @@
>  obj-bin-y += head.o
> +obj-bin-y += built_in_32.o

I don't quite like that this new object name is "built_in_32.o",
It's really closed to "built_in.*" which is used by Rules.mk to collect
all objects in a subdirectory. I don't really have a better suggestion at
the moment.

> @@ -9,9 +10,6 @@ targets   += $(obj32)
>  
>  obj32 := $(addprefix $(obj)/,$(obj32))
>  
> -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> -$(obj)/head.o: $(obj32:.32.o=.bin)
> -
>  CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
>  $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
>  $(obj)/%.32.o: $(src)/%.c FORCE
>       $(call if_changed_rule,cc_o_c)
>  
> +orphan-handling-$(call ld-option,--orphan-handling=error) := 
> --orphan-handling=error
>  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := 
> --no-warn-rwx-segments
>  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
>  LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
>  
> -%.bin: %.lnk
> -     $(OBJCOPY) -j .text -O binary $< $@
> -
> -%.lnk: %.32.o $(src)/build32.lds
> -     $(LD32) -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 FORCE
> +     $(call if_changed_dep,cpp_lds_S)
> +
> +# link all 32bit objects together
> +$(obj)/built_in_32.tmp.o: $(obj32)
> +     $(LD32) -r -o $@ $^
> +
> +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> +## link bundle with a given layout

Could you put the comment aligned with the rest of the recipe? That way
it won't visually separate the rule into several pieces. You can
prefix it with @ so it doesn't show in build logs:

        @# comments

> +     $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map 
> -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o

I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) 
-o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)

:-(, maybe that's lots of $(patsubst,), not sure which is better between
$(patsubst,) and using the stem $*.

Also, if something tries to use built_in_32.tmp.bin, we have a rule that
remove it's prerequisite.

BTW, everything is kind of temporary in a build system, beside the few
files that we want to install on a machine, so having a target named
with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
remove them before the end of its recipe is fine (or those *tmp* aren't
use outside of this recipe).

> +## extract binaries from object
> +     $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> +     rm -f $(obj)/built_in_32.$(*F).o
> +
> +# generate final object file combining and checking above binaries
> +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin 
> $(obj)/built_in_32.final.bin

So, "other" isn't part of "final", I don't really know what those two
things contains so naming wise I can't suggest anything useful.

But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
it get regenerated if the script changes, or the .lds that --script
option seems to use? What is the "--map" option, an input or output?
But I guess we can ignore the ".map" file because it's part of the
".bin".

Another thing that might be useful to do, is to use the "if_changed"
make macro, that way if the command line of the script changes, make
can remake the output. But maybe it's a bit complicated for this recipe
because it doesn't uses $< or $^.

> +     $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> +             --script $(obj)/build32.final.lds \
> +             --bin1 $(obj)/built_in_32.other.bin \
> +             --bin2 $(obj)/built_in_32.final.bin \
> +             --map $(obj)/built_in_32.final.map \
> +             --exports cmdline_parse_early,reloc \
> +             --output $@
> +
> +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> 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..50c167aef0 100644
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -15,22 +15,47 @@
>   * with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -ENTRY(_start)
> +#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)

Is  ^ this a stray space?


Overall, it's kind of mostly style comment that I had, so feel free to
ignore most of them.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech