[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |