| Message ID | 20220414162739.7251-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Thu, Apr 14, 2022 at 05:27:39PM +0100, Andrew Cooper wrote: > There's no point wasting time converting binaries back to asm source. Just > use .incbin directly. Explain in head.S what these binaries are. > > Also, align the blobs. While there's very little static data in the blobs, > they should have at least 4 byte alignment. There was previously no guarantee > that cmdline_parse_early was aligned, and there is no longer an implicit > 4-byte alignment between cmdline_parse_early and reloc caused by the use of > .long. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile > index a5dd094836f6..0670e03b72e0 100644 > --- a/xen/arch/x86/boot/Makefile > +++ b/xen/arch/x86/boot/Makefile > @@ -10,7 +10,10 @@ head-srcs := $(addprefix $(obj)/, $(head-srcs)) > ifdef building_out_of_srctree > $(obj)/head.o: CFLAGS-y += -iquote $(obj) With this patch, we don't the "-iquote" option above, it was only useful for both "#include" been removed. > endif > -$(obj)/head.o: $(head-srcs) > +# For .incbin - add $(obj) to the include path and add the dependencies > +# manually as they're not included in .d > +$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj) > +$(obj)/head.o: $(head-srcs:.S=.bin) The manual dependencies are needed because `make` needs to know what other target are needed before building "head.o". The .d files wouldn't exist on a first build. I don't think a comment about that isn't really necessary, but if there's one it should be about telling `make` to build cmdline.bin and head.bin first. Otherwise, the patch looks good. Thanks,
On 14.04.2022 18:27, Andrew Cooper wrote: > There's no point wasting time converting binaries back to asm source. Just > use .incbin directly. Explain in head.S what these binaries are. Hmm, yes. Why in the world did we do this all these years? > Also, align the blobs. While there's very little static data in the blobs, > they should have at least 4 byte alignment. There was previously no guarantee > that cmdline_parse_early was aligned, and there is no longer an implicit > 4-byte alignment between cmdline_parse_early and reloc caused by the use of > .long. There's no alignment associated with using .long, so I think you want to re-word this. Jan
On 20/04/2022 13:11, Jan Beulich wrote: > On 14.04.2022 18:27, Andrew Cooper wrote: >> There's no point wasting time converting binaries back to asm source. Just >> use .incbin directly. Explain in head.S what these binaries are. > Hmm, yes. Why in the world did we do this all these years? One of many good questions. Others include "why the gratuitous subsell?" and "who's teaching that 'od | tr | awk | sed | sed | sed' is a clever idea?" Apparently someone was under the impression that forks and userspace pipes were free... >> Also, align the blobs. While there's very little static data in the blobs, >> they should have at least 4 byte alignment. There was previously no guarantee >> that cmdline_parse_early was aligned, and there is no longer an implicit >> 4-byte alignment between cmdline_parse_early and reloc caused by the use of >> .long. > There's no alignment associated with using .long, so I think you > want to re-word this. What I mean is that .long will guarantee to have a 4 byte size, so reloc used to end up with the same alignment that cmdline_parse_early did (as far as internal static data is concerned), whereas now it doesn't. ~Andrew
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index a5dd094836f6..0670e03b72e0 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -10,7 +10,10 @@ head-srcs := $(addprefix $(obj)/, $(head-srcs)) ifdef building_out_of_srctree $(obj)/head.o: CFLAGS-y += -iquote $(obj) endif -$(obj)/head.o: $(head-srcs) +# For .incbin - add $(obj) to the include path and add the dependencies +# manually as they're not included in .d +$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj) +$(obj)/head.o: $(head-srcs:.S=.bin) CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) @@ -24,10 +27,6 @@ CFLAGS_x86_32 += -I$(srctree)/include $(head-srcs:.S=.o): CFLAGS_stack_boundary := $(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic -$(head-srcs): %.S: %.bin - (od -v -t x $< | tr -s ' ' | awk 'NR > 1 {print s} {s=$$0}' | \ - sed 's/ /,0x/g' | sed 's/,0x$$//' | sed 's/^[0-9]*,/ .long /') >$@ - %.bin: %.lnk $(OBJCOPY) -j .text -O binary $< $@ diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 3db47197b841..0fb7dd3029f2 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -777,11 +777,17 @@ trampoline_setup: /* Jump into the relocated trampoline. */ lret + /* + * cmdline and reloc are written in C, and linked to be 32bit PIC with + * entrypoints at 0 and using the stdcall convention. + */ + ALIGN cmdline_parse_early: -#include "cmdline.S" + .incbin "cmdline.bin" + ALIGN reloc: -#include "reloc.S" + .incbin "reloc.bin" ENTRY(trampoline_start) #include "trampoline.S"
There's no point wasting time converting binaries back to asm source. Just use .incbin directly. Explain in head.S what these binaries are. Also, align the blobs. While there's very little static data in the blobs, they should have at least 4 byte alignment. There was previously no guarantee that cmdline_parse_early was aligned, and there is no longer an implicit 4-byte alignment between cmdline_parse_early and reloc caused by the use of .long. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> v1.1: * Rebase over the out-of-tree build work Cleanup to $(head-srcs) deferred to the subsequent patch to make the change legible. --- xen/arch/x86/boot/Makefile | 9 ++++----- xen/arch/x86/boot/head.S | 10 ++++++++-- 2 files changed, 12 insertions(+), 7 deletions(-)