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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
<anthony.perard@xxxxxxxxxx> wrote:
>
> 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.
>

It was cbundle.o before, but people preferred built_in_32.o.
It's a collection of object files like built_in.o so it does not seem
so bad to me.
But seen other replies, some other people prefer "bundle".

> > @@ -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
>

Yes, they look more or less the same but technically pretty different.
The "## XXX" is a comment for make command, the "\t@#comment" is a way
to tell make to not print the command before launching it so make will
launch a shell command "# comment".
Yes, indentation does not make it clear. Not that launching a shell to
execute a comment will take a long time. On the other hand, here that
comment is more for the rule than for the shell. Maybe something like

target:
      command \
           # do something

(personally I found it even more ugly)

> > +     $(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 $*.
>

Not strong about stem or patsubst.
The 2 filters seem good, they suggest lds for the script and objects
for the input, which makes sense.

> 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).
>

Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
So we still don't agree on one name, and now we want to find also
another, tricky.
More or less if it can help, one is a 32 bit object file that bundle
together multiple 32 bits object files while the other is the
conversion of the 32 bits bundle file to 64 bits.
XXX_32.o and XXX_32as64.o ??

> > +## 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?

Both map file and linker script are dependency of the bin files so no problems.
Yes, potentially you want to generate if the python script change.
Potentially also if Makefile, make, ld or any other command change,
but that possibly is kind of "we don't usually care".

> But I guess we can ignore the ".map" file because it's part of the
> ".bin".
>

Yes, I suppose they are, although we can make it explicit both
generating them and using as dependencies.

> 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 $^.
>

I usually simply add Makefile to the dependencies. It works too.

> > +     $(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?
>

Yes, a single ASCII character 32. Surely we don't want tabs. Other
parts of the file use 2 spaces indentation, others 8 spaces. Or are
you suggesting to not indent them?

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

Naming can be cumbersome but usually helps readability.

> Cheers,
>
> --
>
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
>

Frediano