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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/10/2024 9:52 am, Frediano Ziglio wrote:
> <snip>
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

The makefile changes here are not the easiest to follow, because there
are two related things being done.

I experimented, and came up with the following:

   
https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/fz-32b-v3.1

which splits the obj32 transformation out of the "totally change how we
link things" which is the purpose of this patch.  The end result is much
clearer to follow IMO.

I've got some other comments, which I've included in the branch, but are
presented here for posterity.

> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index ff0f965876..4cf0d7e140 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,15 +1,18 @@
> +obj32 := cmdline.o
> +obj32 += reloc.o
> +
>  obj-bin-y += head.o
> +obj-bin-y += built_in_32.o
>  
> -head-bin-objs := cmdline.o reloc.o
> +obj32 := $(patsubst %.o,%.32.o,$(obj32))

We're already used to writing out foo.init.o, so just have the .32.o in
list.

This makes patch 3 slightly more obvious, where you're not listing the
same .o in two different obj lists.

>  
> -nocov-y   += $(head-bin-objs)
> -noubsan-y += $(head-bin-objs)
> -targets   += $(head-bin-objs)
> +nocov-y   += $(obj32)
> +noubsan-y += $(obj32)
> +targets   += $(obj32)
>  
> -head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
> +obj32 := $(addprefix $(obj)/,$(obj32))
>  
>  $(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> -$(obj)/head.o: $(head-bin-objs:.o=.bin)

The AFLAGS-y was only for the .incbin's, so can go away here too.

>  
>  CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
>  $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> @@ -17,17 +20,46 @@ CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float 
> -mregparm=3
>  CFLAGS_x86_32 += -nostdinc -include $(filter 
> %/include/xen/config.h,$(XEN_CFLAGS))
>  CFLAGS_x86_32 += $(filter -I% -O%,$(XEN_CFLAGS)) -D__XEN__
>  
> +LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))

Because this is :=, it needs to be after ...

> +
>  # override for 32bit binaries
> -$(head-bin-objs): CFLAGS_stack_boundary :=
> -$(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> +$(obj32): CFLAGS_stack_boundary :=
> +$(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
>  
>  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := 
> --no-warn-rwx-segments
>  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)

... this.

>  
> -%.bin: %.lnk
> -     $(OBJCOPY) -j .text -O binary $< $@
> +$(obj)/%.32.o: $(src)/%.c FORCE
> +     $(call if_changed_rule,cc_o_c)
> +
> +$(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)

FORCE needs to be a prerequisite to use $(call if_changed_dep)  (Sorry -
I missed this one in the branch I pushed.)

> +
> +orphan-handling-$(call ld-option,--orphan-handling=error) := 
> --orphan-handling=error
> +
> +# link all object files together

All 32bit objects.  It isn't even all objects today (head.o isn't included).

> +$(obj)/built_in_32.tmp.o: $(obj32)
> +     $(LD32) -r -o $@ $(obj32)

$^

> +
> +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> +## link bundle with a given layout
> +     $(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
> +## extract binaries from object
> +     $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> +     rm -f $(obj)/built_in_32.$(*F).o
>  
> -%.lnk: %.o $(src)/build32.lds
> -     $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) 
> -o $@ $<
> +# generate final object file combining and checking above binaries
> +$(obj)/built_in_32.o: $(obj)/built_in_32.other.bin 
> $(obj)/built_in_32.final.bin
> +     $(PYTHON) $(srctree)/tools/combine_two_binaries \
> +             --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 \
> +             --section-header '.section .init.text, "ax", @progbits' \
> +             --output $(obj)/built_in_32.s
> +     $(CC) -c $(obj)/built_in_32.s -o $@.tmp
> +     rm -f $(obj)/built_in_32.s $@
> +     mv $@.tmp $@

$(obj)/built_in_32.S: and --output $@

The build system already knows how to turn a .S into a .o.  This form
removed an opencoded $(CC), and leaves built_in_32.S around as an
intermediate to be inspected.

~Andrew