Re: [Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID
>>> On 19.08.16 at 18:14,wrote: > On Mon, Aug 15, 2016 at 10:15:47AM -0400, Konrad Rzeszutek Wilk wrote: >> On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote: >> > >>> On 15.08.16 at 01:42, wrote: >> > >> --- a/xen/arch/x86/efi/Makefile >> > >> +++ b/xen/arch/x86/efi/Makefile >> > >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte >> > >> efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi > check.o 2>disabled && echo y)) >> > >> efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call > create,boot.init.o); $(call create,runtime.o))) >> > >> >> > >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o >> > >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o >> > >> + >> > >> +%.o: %.ihex >> > >> + $(OBJCOPY) -I ihex -O binary $< $@ >> > > >> > > >> > > Under Ubuntu 14.04.4 this fails compilation: >> > > >> > > >> > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'. >> > >> > That's extremely odd, namely considering that the rule is right >> > there in the quoted text above. Could you double check >> > xen/arch/x86/efi/buildid.ihex is actually there? >> >> >> No it is not. I had issues with your email (git am -s) so I did it by >> hand (patch -p2) and of course forgot to include the new file. And later >> on built it on another OS after pulling from a git tree which missed this > file. >> >> Sorry about the false report! > > About the review: > >> >> x86/EFI: use less crude way of generating the build ID >> >> Recent enough binutils support --build-id also for COFF/PE output, and >> hence we should use that in favor of the original hack when possible. > > Could you mention the exact version that gained this? Looks like that was 2.25; added. >> This gets complicated by the linker requiring at least one COFF object >> file to attach the .buildid section to. Hence the patch introduces a > > Is that requirement spelled out in the manpage of the linker? No. Since it mysteriously failed initially, I had to dig into their sources to figure out that requirement. >> buildid.ihex (in order to avoid introducing binary files into the repo) >> which then gets converted to a binary minimal COFF object (no sections, >> no symbols). > > Otherwise: > > Reviewed-by: Konrad Rzeszutek Wilk Thanks. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID
On Mon, Aug 15, 2016 at 10:15:47AM -0400, Konrad Rzeszutek Wilk wrote: > On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote: > > >>> On 15.08.16 at 01:42,wrote: > > >> --- a/xen/arch/x86/efi/Makefile > > >> +++ b/xen/arch/x86/efi/Makefile > > >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte > > >> efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi > > >> check.o 2>disabled && echo y)) > > >> efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call > > >> create,boot.init.o); $(call create,runtime.o))) > > >> > > >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o > > >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o > > >> + > > >> +%.o: %.ihex > > >> +$(OBJCOPY) -I ihex -O binary $< $@ > > > > > > > > > Under Ubuntu 14.04.4 this fails compilation: > > > > > > > > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'. > > > > That's extremely odd, namely considering that the rule is right > > there in the quoted text above. Could you double check > > xen/arch/x86/efi/buildid.ihex is actually there? > > > No it is not. I had issues with your email (git am -s) so I did it by > hand (patch -p2) and of course forgot to include the new file. And later > on built it on another OS after pulling from a git tree which missed this > file. > > Sorry about the false report! About the review: > > x86/EFI: use less crude way of generating the build ID > > Recent enough binutils support --build-id also for COFF/PE output, and > hence we should use that in favor of the original hack when possible. Could you mention the exact version that gained this? > > This gets complicated by the linker requiring at least one COFF object > file to attach the .buildid section to. Hence the patch introduces a Is that requirement spelled out in the manpage of the linker? > buildid.ihex (in order to avoid introducing binary files into the repo) > which then gets converted to a binary minimal COFF object (no sections, > no symbols). Otherwise: Reviewed-by: Konrad Rzeszutek Wilk Thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID
On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote: > >>> On 15.08.16 at 01:42,wrote: > >> --- a/xen/arch/x86/efi/Makefile > >> +++ b/xen/arch/x86/efi/Makefile > >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte > >> efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi > >> check.o 2>disabled && echo y)) > >> efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call > >> create,boot.init.o); $(call create,runtime.o))) > >> > >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o > >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o > >> + > >> +%.o: %.ihex > >> + $(OBJCOPY) -I ihex -O binary $< $@ > > > > > > Under Ubuntu 14.04.4 this fails compilation: > > > > > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'. > > That's extremely odd, namely considering that the rule is right > there in the quoted text above. Could you double check > xen/arch/x86/efi/buildid.ihex is actually there? No it is not. I had issues with your email (git am -s) so I did it by hand (patch -p2) and of course forgot to include the new file. And later on built it on another OS after pulling from a git tree which missed this file. Sorry about the false report! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID
>>> On 15.08.16 at 01:42,wrote: >> --- a/xen/arch/x86/efi/Makefile >> +++ b/xen/arch/x86/efi/Makefile >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte >> efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi >> check.o 2>disabled && echo y)) >> efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call >> create,boot.init.o); $(call create,runtime.o))) >> >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o >> + >> +%.o: %.ihex >> +$(OBJCOPY) -I ihex -O binary $< $@ > > > Under Ubuntu 14.04.4 this fails compilation: > > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'. That's extremely odd, namely considering that the rule is right there in the quoted text above. Could you double check xen/arch/x86/efi/buildid.ihex is actually there? > The properties of Ubuntu 14.04.4 are: > konrad@ubuntu:~/xen$ gcc --version > gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4 > konrad@ubuntu:~/xen/xen$ ld --version > GNU ld (GNU Binutils for Ubuntu) 2.24 > konrad@ubuntu:~/xen/xen$ ld -mi386pep > ld: no input files > > konrad@ubuntu:~/xen/xen$ cat /etc/debian_version > jessie/sid For the error above the version of make is actually the most relevant one (albeit for this trivial a rule I can't really how the make version could matter). Since you've chopped off other make output - does the error indeed occur in xen/arch/x86/efi/ (and not in xen/arch/x86/)? I agree this should be the case, since in the other directory the printed name would have to be efi/buildid.o - I'm just trying make sure impossible things really are impossible. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID
> --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -158,24 +158,30 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(N > $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A > ALT_START$$,,p') > # Don't use $(wildcard ...) here - at least make 3.80 expands this too early! > $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:) > + > ifneq ($(build_id_linker),) > -$(TARGET).efi: note.o > +ifeq ($(call ld-ver-build-id,$(LD) $(EFI_LDFLAGS)),y) > +CFLAGS += -DBUILD_ID_EFI > +EFI_LDFLAGS += $(build_id_linker) > +note_file := efi/buildid.o > +else > note_file := note.o > +endif > else > note_file := > endif > > -$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o > $(BASEDIR)/common/symbols-dummy.o efi/mkreloc > +$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o > $(BASEDIR)/common/symbols-dummy.o efi/mkreloc > $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ > $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< > efi/relocs-dummy.o \ > - $(BASEDIR)/common/symbols-dummy.o -o > $(@D)/.$(@F).$(base).0 &&) : > + $(BASEDIR)/common/symbols-dummy.o $(note_file) -o > $(@D)/.$(@F).$(base).0 &&) : > $(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) > $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S > $(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \ > | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv > --sort >$(@D)/.$(@F).0s.S > $(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o > $(@D)/.$(@F).0s.o > $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ > $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \ > - $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o > $(@D)/.$(@F).$(base).1 &&) : > + $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file) -o > $(@D)/.$(@F).$(base).1 &&) : > $(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) > $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S > $(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \ > | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv > --sort >$(@D)/.$(@F).1s.S > @@ -185,8 +191,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi > if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi > rm -f $(@D)/.$(@F).[0-9]* > > -efi/boot.init.o efi/runtime.o efi/compat.o: > $(BASEDIR)/arch/x86/efi/built_in.o > -efi/boot.init.o efi/runtime.o efi/compat.o: ; > +efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: > $(BASEDIR)/arch/x86/efi/built_in.o > +efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ; > > asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c > $(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $< > --- a/xen/arch/x86/efi/Makefile > +++ b/xen/arch/x86/efi/Makefile > @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte > efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi > check.o 2>disabled && echo y)) > efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); > $(call create,runtime.o))) > > -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o > +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o > + > +%.o: %.ihex > + $(OBJCOPY) -I ihex -O binary $< $@ > Under Ubuntu 14.04.4 this fails compilation: make[4]: *** No rule to make target `buildid.o', needed by `stub.o'. Stop. The properties of Ubuntu 14.04.4 are: konrad@ubuntu:~/xen$ gcc --version gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4 konrad@ubuntu:~/xen/xen$ ld --version GNU ld (GNU Binutils for Ubuntu) 2.24 konrad@ubuntu:~/xen/xen$ ld -mi386pep ld: no input files konrad@ubuntu:~/xen/xen$ cat /etc/debian_version jessie/sid Hadn't dug in this. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID
Recent enough binutils support --build-id also for COFF/PE output, and hence we should use that in favor of the original hack when possible. This gets complicated by the linker requiring at least one COFF object file to attach the .buildid section to. Hence the patch introduces a buildid.ihex (in order to avoid introducing binary files into the repo) which then gets converted to a binary minimal COFF object (no sections, no symbols). Also (to avoid both code fragment going out of sync) remove an unneeded ALIGN() from xen.lds.S: Adding an equivalent of it to the .buildid section would cause the _erodata symbol to become associated with the wrong section again (see commit 0970299de5 ["x86/EFI + Live Patch: avoid symbol address truncation"]). And it's pointless because the alignment already gets properly set by the input section(s). Signed-off-by: Jan Beulich--- Question of course is whether consumers of the build ID other than Xen itself need to be taught how to find it in an EFI binary. --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -158,24 +158,30 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(N $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p') # Don't use $(wildcard ...) here - at least make 3.80 expands this too early! $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:) + ifneq ($(build_id_linker),) -$(TARGET).efi: note.o +ifeq ($(call ld-ver-build-id,$(LD) $(EFI_LDFLAGS)),y) +CFLAGS += -DBUILD_ID_EFI +EFI_LDFLAGS += $(build_id_linker) +note_file := efi/buildid.o +else note_file := note.o +endif else note_file := endif -$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc +$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \ - $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) : + $(BASEDIR)/common/symbols-dummy.o $(note_file) -o $(@D)/.$(@F).$(base).0 &&) : $(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S $(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \ | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S $(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \ - $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) : + $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file) -o $(@D)/.$(@F).$(base).1 &&) : $(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S $(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \ | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S @@ -185,8 +191,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi rm -f $(@D)/.$(@F).[0-9]* -efi/boot.init.o efi/runtime.o efi/compat.o: $(BASEDIR)/arch/x86/efi/built_in.o -efi/boot.init.o efi/runtime.o efi/compat.o: ; +efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o +efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ; asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $< --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y)) efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o))) -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o + +%.o: %.ihex + $(OBJCOPY) -I ihex -O binary $< $@ stub.o: $(extra-y) --- /dev/null +++ b/xen/arch/x86/efi/buildid.ihex @@ -0,0 +1,3 @@ +:100064864D8DAD57140014 +:04001000EC +:0001FF --- a/xen/arch/x86/efi/mkreloc.c +++ b/xen/arch/x86/efi/mkreloc.c @@ -342,6 +342,7 @@ int main(int argc, char *argv[]) */ if ( memcmp(sec1[i].name, ".initcal", sizeof(sec1[i].name)) == 0 || memcmp(sec1[i].name, ".init.se", sizeof(sec1[i].name)) == 0 || + memcmp(sec1[i].name, ".buildid", sizeof(sec1[i].name)) == 0 || memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 ) continue; --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -91,7 +91,7 @@