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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Oct 18, 2024 at 02:04:22PM +0200, Jan Beulich wrote:
> On 18.10.2024 13:41, Roger Pau Monné wrote:
> > On Thu, Oct 17, 2024 at 02:31:19PM +0100, Frediano Ziglio wrote:
> >> @@ -25,14 +23,47 @@ $(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 $< $@
> >> +text_gap := 0x010200
> >> +text_diff := 0x408020
> >> +
> >> +$(obj)/build32.base.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff)
> >> +$(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) -DFINAL
> >> +$(obj)/build32.base.lds $(obj)/build32.offset.lds: $(src)/build32.lds.S FORCE
> >> +	$(call if_changed_dep,cpp_lds_S)
> >> +
> >> +targets += build32.offset.lds build32.base.lds
> >> +
> >> +# link all 32bit objects together
> >> +$(obj)/built-in-32.tmp.o: $(obj32)
> >> +	$(LD32) -r -o $@ $^
> >> +
> >> +# link bundle with a given layout and extract a binary from it
> >> +$(obj)/built-in-32.%.bin: $(obj)/build32.%.lds $(obj)/built-in-32.tmp.o
> >> +	$(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^)
> >> +	$(OBJCOPY) -j .text -O binary $(@:bin=o) $@
> >> +	rm -f $(@:bin=o)
> >> +
> >> +quiet_cmd_combine = GEN     $@
> >> +cmd_combine = \
> >> +	$(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> >> +		--gap=$(text_gap) --text-diff=$(text_diff) \
> >> +		--script $(obj)/build32.offset.lds \
> >> +		--bin1 $(obj)/built-in-32.base.bin \
> >> +		--bin2 $(obj)/built-in-32.offset.bin \
> >> +		--map $(obj)/built-in-32.offset.map \
> >> +		--exports cmdline_parse_early,reloc \
> >> +		--output $@
> > 
> > See xen/Rules.mk, for consistency the indentation should be done with
> > spaces when defining variables.  That would also allow to align the
> > options.
> 
> And ideally also such that the options align with the first program
> argument.

Yup, that's what I was attempting to suggest, sorry if it wasn't
clear.

> >> +
> >> +targets += built-in-32.S
> >>  
> >> -%.lnk: %.32.o $(src)/build32.lds
> >> -	$(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> >> +# generate final object file combining and checking above binaries
> >> +$(obj)/built-in-32.S: $(obj)/built-in-32.base.bin $(obj)/built-in-32.offset.bin \
> >> +		$(srctree)/tools/combine_two_binaries.py FORCE
> > 
> > Can you indent this using spaces also, so it's on the same column as
> > the ':'?
> 
> The first $(obj) you mean, I think / hope?

Indeed, it's one space after the ':':

# Generate final object file combining and checking above binaries
$(obj)/built-in-32.S: $(obj)/built-in-32.base.bin $(obj)/built-in-32.offset.bin \
                      $(srctree)/tools/combine_two_binaries.py FORCE

Preferably comments should also start with an uppercase letter.

Note this already exceeds the 80 char maximum, as the longest line is
81.  I think we have been a bit lax with Makefiles however.

Thanks, Roger.