| Message ID | 1458849640-22588-20-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > The version of ld that first implemented --build-id is v2.18. > Hence we check for that or later version - if older version > found we do not build the hypervisor with the build-id > (and the return code is -ENODATA for xen_build_id() call). This appears to be stale. > The EFI binary is more complicated. Having any non-recognizable > sections (.note, .data.note, etc) causes the boot to hang. > Moving the .note in the .data section makes it work. It is also > worth noting that the PE/COFF does not have any "comment" > sections to the author. I'm afraid this is too vague: What exactly is happening there? And is this due to binutils doing something wrong, or an issue with the firmware on the box you've tried? While the placement of .note is not really a big deal, any unusual placement needs a source comment attached explaining the whys, to prevent people down the road moving the section back into the "expected" place. But see also below. > Lastly, we MUST call --binary-id=sha1 on all linker invocation so that > symbol offsets don't changes (which means we have multiple binary > ids - except that the last one is the final one). Without this change, > the symbol table embedded in Xen are incorrect - some of the values it > contains are offset by the size of the included build id. > This obviously causes problems when resolving symbols. Would this also be a problem if you place the notes segment/section last in the binary? > --- a/Config.mk > +++ b/Config.mk > @@ -126,6 +126,17 @@ endef > check-$(gcc) = $(call cc-ver-check,CC,0x040100,"Xen requires at least gcc-4.1") > $(eval $(check-y)) > > +ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \ > + grep -q unrecognized && echo n || echo y) > + > +# binutils 2.18 implement build-id. I don't think this comment is really relevant anymore. > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -22,6 +22,9 @@ OUTPUT_ARCH(FORMAT) > PHDRS > { > text PT_LOAD /* XXX should be AT ( XEN_PHYS_START ) */ ; > +#if defined(BUILD_ID) > + note PT_NOTE ; > +#endif Is this in line with ... > @@ -57,9 +60,18 @@ SECTIONS > *(.lockprofile.data) > __lock_profile_end = .; > #endif > + } :text > > - _erodata = .; /* End of read-only data */ > +#if defined(BUILD_ID) > + .note : { > + __note_gnu_build_id_start = .; > + *(.note.gnu.build-id) > + __note_gnu_build_id_end = .; > + *(.note) > + *(.note.*) > } :text > +#endif ... there not being any :note here? And if so, why the earlier "} :text"? > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -72,9 +72,16 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h > -o \ > -O $(BASEDIR)/include/xen/compile.h ]; then \ > echo '$(TARGET).efi'; fi) > > +ifdef build_id_linker According to Config.mk this is always true. DYM "ifneq ($(build_id_linker),)"? > +build_id.o: $(TARGET)-syms > + $(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin Considering your xen.lds.S changes, won't this potentially copy quite a bit more than just the build ID (i.e. all notes)? This may be okay, and may be even intended, but then the generate file's name should reflect this to not misguide people. > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ > + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ Since you put the notes into .rodata anyway, why name the section .note.*? Just name it .rodata.*, avoiding to mislead others who also think that a section's name has much of a meaning. > @@ -138,6 +151,13 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIR > $(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),:) > +ifdef build_id_linker See above. > @@ -228,21 +257,22 @@ static void do_read(int fd, void *data, int len) > int main(int argc, char **argv) > { > u64 final_exec_addr; > - u32 loadbase, dat_siz, mem_siz; > + u32 loadbase, dat_siz, mem_siz, note_base, note_sz, offset; > char *inimage, *outimage; > int infd, outfd; > char buffer[1024]; > int bytes, todo, i; > + int num_phdrs; > > Elf32_Ehdr in32_ehdr; > > Elf64_Ehdr in64_ehdr; > Elf64_Phdr in64_phdr; > > - if ( argc != 5 ) > + if ( argc != 6 ) > { > fprintf(stderr, "Usage: mkelf32 <in-image> <out-image> " > - "<load-base> <final-exec-addr>\n"); > + "<load-base> <final-exec-addr> <number of program headers>\n"); Even if only an internally used tool, I think this is a bad interface: You want notes copied, and this only happens to result in a second PHDR. Hence I think there should be an option "--notes" or some such. > @@ -299,11 +334,36 @@ int main(int argc, char **argv) > > (void)lseek(infd, in64_phdr.p_offset, SEEK_SET); > dat_siz = (u32)in64_phdr.p_filesz; > - > /* Do not use p_memsz: it does not include BSS alignment padding. */ Stray deletion of a blank line. > @@ -322,6 +382,31 @@ int main(int argc, char **argv) > out_shdr[1].sh_size = dat_siz; > out_shdr[2].sh_offset = RAW_OFFSET + dat_siz + sizeof(out_shdr); > > + if ( num_phdrs > 1 ) > + { > + /* We have two of them! */ > + out_ehdr.e_phnum = num_phdrs; > + /* Extra .note section. */ > + out_ehdr.e_shnum++; > + > + /* Fill out the PT_NOTE program header. */ > + note_phdr.p_vaddr = note_base; > + note_phdr.p_paddr = note_base; > + note_phdr.p_filesz = note_sz; > + note_phdr.p_memsz = note_sz; > + note_phdr.p_offset = offset; > + > + /* Tack on the .note\0 */ > + out_shdr[2].sh_size += sizeof(out_shstrtab_extra); > + /* And move it past the .note section. */ > + out_shdr[2].sh_offset += sizeof(out_shdr_extra); Why "extra" and not "note" or "notes"? > @@ -335,8 +420,15 @@ int main(int argc, char **argv) > > endianadjust_phdr32(&out_phdr); > do_write(outfd, &out_phdr, sizeof(out_phdr)); > - > - if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr)) < 0 ) > + > + if ( num_phdrs > 1 ) > + { > + endianadjust_phdr32(¬e_phdr); > + do_write(outfd, ¬e_phdr, sizeof(note_phdr)); > + } > + > + if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr) - > + ( num_phdrs > 1 ? sizeof(note_phdr) : 0 ) ) < 0 ) Stray blanks. Also with each PHDR being the same size, why not just num_phdrs * sizeof()? > @@ -99,6 +107,21 @@ SECTIONS > _erodata = .; > } :text > > +#if defined(BUILD_ID) && !defined(EFI) > +/* > + * No mechanism to put an PT_NOTE in the EFI file - so put > + * it in .data section. > + */ I think this comment really belongs into the earlier addition. > + . = ALIGN(4); I think this would better be added in the EFI case too. > + .note : { > + __note_gnu_build_id_start = .; > + *(.note.gnu.build-id) > + __note_gnu_build_id_end = .; > + *(.note) > + *(.note.*) The last two don't get dealt with in the EFI case - why? And if you include all notes here and copy them in their entirety into xen.efi, how is __note_gnu_build_id_end expected to be correct then? > --- a/xen/common/version.c > +++ b/xen/common/version.c > @@ -1,5 +1,9 @@ > #include <xen/compile.h> > +#include <xen/errno.h> > +#include <xen/string.h> > +#include <xen/types.h> > #include <xen/version.h> > +#include <xen/elf.h> Please put this last one into its designated slot as well, considering that you arrange for all the rest to be nicely sorted. > +int xen_build_id(const void **p, unsigned int *len) > +{ > + const Elf_Note *n = __note_gnu_build_id_start; > + static bool_t checked = 0; > + > + if ( checked ) > + { > + *len = n->descsz; > + *p = ELFNOTE_DESC(n); > + return 0; > + } > + /* --build-id invoked with wrong parameters. */ > + if ( __note_gnu_build_id_end <= __note_gnu_build_id_start ) Please use the local variable here, just like you do ... > + return -ENODATA; > + > + /* Check for full Note header. */ > + if ( &n[1] > __note_gnu_build_id_end ) ... here. > + return -ENODATA; > + > + /* Check if we really have a build-id. */ > + if ( NT_GNU_BUILD_ID != n->type ) > + return -ENODATA; > + > + /* Sanity check, name should be "GNU" for ld-generated build-id. */ > + if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 ) > + return -ENODATA; > + > + *len = n->descsz; > + *p = ELFNOTE_DESC(n); > + > + checked = 1; All this checking would perhaps better be done in an initcall, at once avoiding it to be done over and over if it fails for some reason. > + return 0; > +} > + > +#else > + > +int xen_build_id(const void **p, unsigned int *len) > +{ > + return -ENODATA; > +} > +#endif Is this #else part really needed? If you omitted the BUILD_ID dependency in xen.lds.S, wouldn't you just find an empty section here when the linker is incapable of --build-id? > --- a/xen/include/xen/version.h > +++ b/xen/include/xen/version.h > @@ -14,4 +14,7 @@ const char *xen_changeset(void); > const char *xen_banner(void); > const char *xen_deny(void); > > +#include <xen/types.h> Why? And if really needed, it doesn't belong in the middle of the file. Jan
On Mon, Apr 04, 2016 at 06:46:24AM -0600, Jan Beulich wrote: > >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > > The version of ld that first implemented --build-id is v2.18. > > Hence we check for that or later version - if older version > > found we do not build the hypervisor with the build-id > > (and the return code is -ENODATA for xen_build_id() call). > > This appears to be stale. > > > The EFI binary is more complicated. Having any non-recognizable > > sections (.note, .data.note, etc) causes the boot to hang. > > Moving the .note in the .data section makes it work. It is also > > worth noting that the PE/COFF does not have any "comment" > > sections to the author. > > I'm afraid this is too vague: What exactly is happening there? And > is this due to binutils doing something wrong, or an issue with the > firmware on the box you've tried? While the placement of .note is > not really a big deal, any unusual placement needs a source > comment attached explaining the whys, to prevent people down > the road moving the section back into the "expected" place. But > see also below. I will have to dig in this more. I know I tried it on TianoCore but that seems to have some other issues at the moment so I will try it out on real hardware. > > > Lastly, we MUST call --binary-id=sha1 on all linker invocation so that > > symbol offsets don't changes (which means we have multiple binary > > ids - except that the last one is the final one). Without this change, > > the symbol table embedded in Xen are incorrect - some of the values it > > contains are offset by the size of the included build id. > > This obviously causes problems when resolving symbols. > > Would this also be a problem if you place the notes segment/section > last in the binary? I am not sure. Ross, you are the one who observed this? > > +build_id.o: $(TARGET)-syms > > + $(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin > > Considering your xen.lds.S changes, won't this potentially copy quite > a bit more than just the build ID (i.e. all notes)? This may be okay, and > may be even intended, but then the generate file's name should > reflect this to not misguide people. I renamed it notes.o > > > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ > > + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ > > Since you put the notes into .rodata anyway, why name the > section .note.*? Just name it .rodata.*, avoiding to mislead > others who also think that a section's name has much of a > meaning. I thought that somebody during review asked for it to be called .note? But I will dig in this next week.
> > +build_id.o: $(TARGET)-syms > > + $(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin > > Considering your xen.lds.S changes, won't this potentially copy quite > a bit more than just the build ID (i.e. all notes)? This may be okay, and > may be even intended, but then the generate file's name should > reflect this to not misguide people. I changed it notes.o > > > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ > > + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ > > Since you put the notes into .rodata anyway, why name the > section .note.*? Just name it .rodata.*, avoiding to mislead > others who also think that a section's name has much of a > meaning. Way back last year: http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01264.html which is where the .note came about. I can put it all in .rodata and not have it for normal ELF builds if you would like.
On Thu, Apr 07, 2016 at 08:18:27PM -0400, Konrad Rzeszutek Wilk wrote: > > > +build_id.o: $(TARGET)-syms > > > + $(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin > > > > Considering your xen.lds.S changes, won't this potentially copy quite > > a bit more than just the build ID (i.e. all notes)? This may be okay, and > > may be even intended, but then the generate file's name should > > reflect this to not misguide people. > > I changed it notes.o > > > > > > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ > > > + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ > > > > Since you put the notes into .rodata anyway, why name the > > section .note.*? Just name it .rodata.*, avoiding to mislead > > others who also think that a section's name has much of a > > meaning. > > Way back last year: > http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01264.html > > which is where the .note came about. I can put it all in .rodata > and not have it for normal ELF builds if you would like. .rodata.notes for both ELF and EFI looks to have done the trick. Now it is just the matter of testing it.
>>> On 08.04.16 at 02:18, <konrad@kernel.org> wrote: >> > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ >> > + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ >> >> Since you put the notes into .rodata anyway, why name the >> section .note.*? Just name it .rodata.*, avoiding to mislead >> others who also think that a section's name has much of a >> meaning. > > Way back last year: > http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01264.html But that wasn't dealing with the EFI case at all back then. > which is where the .note came about. I can put it all in .rodata > and not have it for normal ELF builds if you would like. It would, overall, look more consistent to me. Jan
>>> On 08.04.16 at 03:52, <konrad@kernel.org> wrote: > On Thu, Apr 07, 2016 at 08:18:27PM -0400, Konrad Rzeszutek Wilk wrote: >> > >> > > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ >> > > + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ >> > >> > Since you put the notes into .rodata anyway, why name the >> > section .note.*? Just name it .rodata.*, avoiding to mislead >> > others who also think that a section's name has much of a >> > meaning. >> >> Way back last year: >> http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01264.html >> >> which is where the .note came about. I can put it all in .rodata >> and not have it for normal ELF builds if you would like. > > .rodata.notes for both ELF and EFI looks to have done the trick. > Now it is just the matter of testing it. Why also for ELF? In ELF, these are ordinary notes, so imo belong into .note or .note.*. As opposed to COFF/PE, where the idea of notes doesn't exist. Jan
On 04/07/2016 03:58 AM, Konrad Rzeszutek Wilk wrote: > On Mon, Apr 04, 2016 at 06:46:24AM -0600, Jan Beulich wrote: >>>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >>> The version of ld that first implemented --build-id is v2.18. >>> Hence we check for that or later version - if older version >>> found we do not build the hypervisor with the build-id >>> (and the return code is -ENODATA for xen_build_id() call). >> >> This appears to be stale. >> >>> The EFI binary is more complicated. Having any non-recognizable >>> sections (.note, .data.note, etc) causes the boot to hang. >>> Moving the .note in the .data section makes it work. It is also >>> worth noting that the PE/COFF does not have any "comment" >>> sections to the author. >> >> I'm afraid this is too vague: What exactly is happening there? And >> is this due to binutils doing something wrong, or an issue with the >> firmware on the box you've tried? While the placement of .note is >> not really a big deal, any unusual placement needs a source >> comment attached explaining the whys, to prevent people down >> the road moving the section back into the "expected" place. But >> see also below. > > I will have to dig in this more. I know I tried it on TianoCore but > that seems to have some other issues at the moment so I will > try it out on real hardware. >> >>> Lastly, we MUST call --binary-id=sha1 on all linker invocation so that >>> symbol offsets don't changes (which means we have multiple binary >>> ids - except that the last one is the final one). Without this change, >>> the symbol table embedded in Xen are incorrect - some of the values it >>> contains are offset by the size of the included build id. >>> This obviously causes problems when resolving symbols. >> >> Would this also be a problem if you place the notes segment/section >> last in the binary? > > I am not sure. Ross, you are the one who observed this? Yes, that would probably solve the problem.
On Fri, Apr 08, 2016 at 04:49:00PM +0100, Ross Lagerwall wrote: > On 04/07/2016 03:58 AM, Konrad Rzeszutek Wilk wrote: > >On Mon, Apr 04, 2016 at 06:46:24AM -0600, Jan Beulich wrote: > >>>>>On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > >>>The version of ld that first implemented --build-id is v2.18. > >>>Hence we check for that or later version - if older version > >>>found we do not build the hypervisor with the build-id > >>>(and the return code is -ENODATA for xen_build_id() call). > >> > >>This appears to be stale. > >> > >>>The EFI binary is more complicated. Having any non-recognizable > >>>sections (.note, .data.note, etc) causes the boot to hang. > >>>Moving the .note in the .data section makes it work. It is also > >>>worth noting that the PE/COFF does not have any "comment" > >>>sections to the author. > >> > >>I'm afraid this is too vague: What exactly is happening there? And > >>is this due to binutils doing something wrong, or an issue with the > >>firmware on the box you've tried? While the placement of .note is > >>not really a big deal, any unusual placement needs a source > >>comment attached explaining the whys, to prevent people down > >>the road moving the section back into the "expected" place. But > >>see also below. > > > >I will have to dig in this more. I know I tried it on TianoCore but > >that seems to have some other issues at the moment so I will > >try it out on real hardware. > >> > >>>Lastly, we MUST call --binary-id=sha1 on all linker invocation so that > >>>symbol offsets don't changes (which means we have multiple binary > >>>ids - except that the last one is the final one). Without this change, > >>>the symbol table embedded in Xen are incorrect - some of the values it > >>>contains are offset by the size of the included build id. > >>>This obviously causes problems when resolving symbols. > >> > >>Would this also be a problem if you place the notes segment/section > >>last in the binary? > > > >I am not sure. Ross, you are the one who observed this? > > Yes, that would probably solve the problem. Sadly no. I stuck the .note right before .bss section and I got: relink.o: In function `symbols_expand_symbol': /home/konrad/xen/xen/common/symbols.c:47: undefined reference to `symbols_names' /home/konrad/xen/xen/common/symbols.c:47:(.text+0x30f40): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `symbols_names' /home/konrad/xen/xen/common/symbols.c:58: undefined reference to `symbols_token_table' /home/konrad/xen/xen/common/symbols.c:58:(.text+0x30f6e): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `symbols_token_table' > > -- > Ross Lagerwall
>>>>> Lastly, we MUST call --binary-id=sha1 on all linker invocation so that >>>>> symbol offsets don't changes (which means we have multiple binary >>>>> ids - except that the last one is the final one). Without this change, >>>>> the symbol table embedded in Xen are incorrect - some of the values it >>>>> contains are offset by the size of the included build id. >>>>> This obviously causes problems when resolving symbols. >>>> Would this also be a problem if you place the notes segment/section >>>> last in the binary? >>> I am not sure. Ross, you are the one who observed this? >> Yes, that would probably solve the problem. > Sadly no. I stuck the .note right before .bss section and I got: > > relink.o: In function `symbols_expand_symbol': > /home/konrad/xen/xen/common/symbols.c:47: undefined reference to `symbols_names' > /home/konrad/xen/xen/common/symbols.c:47:(.text+0x30f40): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `symbols_names' > /home/konrad/xen/xen/common/symbols.c:58: undefined reference to `symbols_token_table' > /home/konrad/xen/xen/common/symbols.c:58:(.text+0x30f6e): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `symbols_token_table' Ah yes - our symbol table generation also plays the same trick... ~Andrew
>>> On 08.04.16 at 20:47, <konrad.wilk@oracle.com> wrote: > On Fri, Apr 08, 2016 at 04:49:00PM +0100, Ross Lagerwall wrote: >> On 04/07/2016 03:58 AM, Konrad Rzeszutek Wilk wrote: >> >On Mon, Apr 04, 2016 at 06:46:24AM -0600, Jan Beulich wrote: >> >>>>>On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >> >>>The version of ld that first implemented --build-id is v2.18. >> >>>Hence we check for that or later version - if older version >> >>>found we do not build the hypervisor with the build-id >> >>>(and the return code is -ENODATA for xen_build_id() call). >> >> >> >>This appears to be stale. >> >> >> >>>The EFI binary is more complicated. Having any non-recognizable >> >>>sections (.note, .data.note, etc) causes the boot to hang. >> >>>Moving the .note in the .data section makes it work. It is also >> >>>worth noting that the PE/COFF does not have any "comment" >> >>>sections to the author. >> >> >> >>I'm afraid this is too vague: What exactly is happening there? And >> >>is this due to binutils doing something wrong, or an issue with the >> >>firmware on the box you've tried? While the placement of .note is >> >>not really a big deal, any unusual placement needs a source >> >>comment attached explaining the whys, to prevent people down >> >>the road moving the section back into the "expected" place. But >> >>see also below. >> > >> >I will have to dig in this more. I know I tried it on TianoCore but >> >that seems to have some other issues at the moment so I will >> >try it out on real hardware. >> >> >> >>>Lastly, we MUST call --binary-id=sha1 on all linker invocation so that >> >>>symbol offsets don't changes (which means we have multiple binary >> >>>ids - except that the last one is the final one). Without this change, >> >>>the symbol table embedded in Xen are incorrect - some of the values it >> >>>contains are offset by the size of the included build id. >> >>>This obviously causes problems when resolving symbols. >> >> >> >>Would this also be a problem if you place the notes segment/section >> >>last in the binary? >> > >> >I am not sure. Ross, you are the one who observed this? >> >> Yes, that would probably solve the problem. > > Sadly no. I stuck the .note right before .bss section and I got: > > relink.o: In function `symbols_expand_symbol': > /home/konrad/xen/xen/common/symbols.c:47: undefined reference to > `symbols_names' > /home/konrad/xen/xen/common/symbols.c:47:(.text+0x30f40): relocation > truncated to fit: R_X86_64_PC32 against undefined symbol `symbols_names' > /home/konrad/xen/xen/common/symbols.c:58: undefined reference to > `symbols_token_table' > /home/konrad/xen/xen/common/symbols.c:58:(.text+0x30f6e): relocation > truncated to fit: R_X86_64_PC32 against undefined symbol > `symbols_token_table' Well, "before the .bss section" != "last in the binary", but since we prefer it to be part of r/o data anyway, I guess always passing the option is reasonable. Jan
diff --git a/Config.mk b/Config.mk index 79eb2bd..c8e89fe 100644 --- a/Config.mk +++ b/Config.mk @@ -126,6 +126,17 @@ endef check-$(gcc) = $(call cc-ver-check,CC,0x040100,"Xen requires at least gcc-4.1") $(eval $(check-y)) +ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \ + grep -q unrecognized && echo n || echo y) + +# binutils 2.18 implement build-id. +ifeq ($(call ld-ver-build-id,$(LD)),n) +build_id_linker := +else +CFLAGS += -DBUILD_ID +build_id_linker := --build-id=sha1 +endif + # as-insn: Check whether assembler supports an instruction. # Usage: cflags-y += $(call as-insn "insn",option-yes,option-no) as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \ diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 17e9e3a..a3319ab 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -94,7 +94,7 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1.S $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o - $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ + $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \ $(@D)/.$(@F).1.o -o $@ rm -f $(@D)/.$(@F).[0-9]* diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 9909595..aad26e3 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -22,6 +22,9 @@ OUTPUT_ARCH(FORMAT) PHDRS { text PT_LOAD /* XXX should be AT ( XEN_PHYS_START ) */ ; +#if defined(BUILD_ID) + note PT_NOTE ; +#endif } SECTIONS { @@ -57,9 +60,18 @@ SECTIONS *(.lockprofile.data) __lock_profile_end = .; #endif + } :text - _erodata = .; /* End of read-only data */ +#if defined(BUILD_ID) + .note : { + __note_gnu_build_id_start = .; + *(.note.gnu.build-id) + __note_gnu_build_id_end = .; + *(.note) + *(.note.*) } :text +#endif + _erodata = .; /* End of read-only data */ .data : { /* Data */ . = ALIGN(PAGE_SIZE); diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index d4a8069..7a1f710 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -72,9 +72,16 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h -o \ -O $(BASEDIR)/include/xen/compile.h ]; then \ echo '$(TARGET).efi'; fi) +ifdef build_id_linker +num_phdrs = 2 +else +num_phdrs = 1 +endif + $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 ./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x100000 \ - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` + `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` \ + $(num_phdrs) $(MAKE) -f $(BASEDIR)/Rules.mk -C test install: @@ -110,22 +117,28 @@ $(BASEDIR)/common/symbols-dummy.o: $(MAKE) -f $(BASEDIR)/Rules.mk -C $(BASEDIR)/common symbols-dummy.o $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o - $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ + $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \ $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0 $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ | $(BASEDIR)/tools/symbols --all-symbols --sysv --sort \ >$(@D)/.$(@F).0.S $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o - $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ + $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \ $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ | $(BASEDIR)/tools/symbols --all-symbols --sysv --sort --warn-dup \ >$(@D)/.$(@F).1.S $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o - $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ + $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \ $(@D)/.$(@F).1.o -o $@ rm -f $(@D)/.$(@F).[0-9]* +build_id.o: $(TARGET)-syms + $(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ + --rename-section=.data=.note.gnu.build-id -S $@.bin $@ + rm -f $@.bin + EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(LDFLAGS)) --subsystem=10 EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0 --strip-debug EFI_LDFLAGS += --section-alignment=0x200000 --file-alignment=0x20 @@ -138,6 +151,13 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIR $(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),:) +ifdef build_id_linker +$(TARGET).efi: build_id.o +build_id_file := build_id.o +else +build_id_file := +endif + $(TARGET).efi: prelink-efi.o 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 \ @@ -154,7 +174,7 @@ $(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbol | $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1s.S $(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o $(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \ - $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o -o $@ + $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o $(build_id_file) -o $@ if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi rm -f $(@D)/.$(@F).[0-9]* diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c index 993a7ee..d230e4c 100644 --- a/xen/arch/x86/boot/mkelf32.c +++ b/xen/arch/x86/boot/mkelf32.c @@ -45,9 +45,9 @@ static Elf32_Ehdr out_ehdr = { 0, /* e_flags */ sizeof(Elf32_Ehdr), /* e_ehsize */ sizeof(Elf32_Phdr), /* e_phentsize */ - 1, /* e_phnum */ + 1, /* modify based on num_phdrs */ /* e_phnum */ sizeof(Elf32_Shdr), /* e_shentsize */ - 3, /* e_shnum */ + 3, /* modify based on num_phdrs */ /* e_shnum */ 2 /* e_shstrndx */ }; @@ -61,8 +61,20 @@ static Elf32_Phdr out_phdr = { PF_R|PF_W|PF_X, /* p_flags */ 64 /* p_align */ }; +static Elf32_Phdr note_phdr = { + PT_NOTE, /* p_type */ + DYNAMICALLY_FILLED, /* p_offset */ + DYNAMICALLY_FILLED, /* p_vaddr */ + DYNAMICALLY_FILLED, /* p_paddr */ + DYNAMICALLY_FILLED, /* p_filesz */ + DYNAMICALLY_FILLED, /* p_memsz */ + PF_R, /* p_flags */ + 4 /* p_align */ +}; static u8 out_shstrtab[] = "\0.text\0.shstrtab"; +/* If num_phdrs >= 2, we need to tack the .note. */ +static u8 out_shstrtab_extra[] = ".note\0"; static Elf32_Shdr out_shdr[] = { { 0 }, @@ -90,6 +102,23 @@ static Elf32_Shdr out_shdr[] = { } }; +/* + * The 17 points to the '.note' in the out_shstrtab and out_shstrtab_extra + * laid out in the file. + */ +static Elf32_Shdr out_shdr_extra = { + 17, /* sh_name */ + SHT_NOTE, /* sh_type */ + 0, /* sh_flags */ + DYNAMICALLY_FILLED, /* sh_addr */ + DYNAMICALLY_FILLED, /* sh_offset */ + DYNAMICALLY_FILLED, /* sh_size */ + 0, /* sh_link */ + 0, /* sh_info */ + 4, /* sh_addralign */ + 0 /* sh_entsize */ +}; + /* Some system header files define these macros and pollute our namespace. */ #undef swap16 #undef swap32 @@ -228,21 +257,22 @@ static void do_read(int fd, void *data, int len) int main(int argc, char **argv) { u64 final_exec_addr; - u32 loadbase, dat_siz, mem_siz; + u32 loadbase, dat_siz, mem_siz, note_base, note_sz, offset; char *inimage, *outimage; int infd, outfd; char buffer[1024]; int bytes, todo, i; + int num_phdrs; Elf32_Ehdr in32_ehdr; Elf64_Ehdr in64_ehdr; Elf64_Phdr in64_phdr; - if ( argc != 5 ) + if ( argc != 6 ) { fprintf(stderr, "Usage: mkelf32 <in-image> <out-image> " - "<load-base> <final-exec-addr>\n"); + "<load-base> <final-exec-addr> <number of program headers>\n"); return 1; } @@ -250,7 +280,13 @@ int main(int argc, char **argv) outimage = argv[2]; loadbase = strtoul(argv[3], NULL, 16); final_exec_addr = strtoull(argv[4], NULL, 16); - + num_phdrs = atoi(argv[5]); + if ( num_phdrs > 2 || num_phdrs < 1 ) + { + fprintf(stderr, "Number of program headers MUST be 1 or 2, got %d!\n", + num_phdrs); + return 1; + } infd = open(inimage, O_RDONLY); if ( infd == -1 ) { @@ -285,11 +321,10 @@ int main(int argc, char **argv) (int)in64_ehdr.e_phentsize, (int)sizeof(in64_phdr)); return 1; } - - if ( in64_ehdr.e_phnum != 1 ) + if ( in64_ehdr.e_phnum != num_phdrs ) { - fprintf(stderr, "Expect precisly 1 program header; found %d.\n", - (int)in64_ehdr.e_phnum); + fprintf(stderr, "Expect precisly %d program header; found %d.\n", + num_phdrs, (int)in64_ehdr.e_phnum); return 1; } @@ -299,11 +334,36 @@ int main(int argc, char **argv) (void)lseek(infd, in64_phdr.p_offset, SEEK_SET); dat_siz = (u32)in64_phdr.p_filesz; - /* Do not use p_memsz: it does not include BSS alignment padding. */ /*mem_siz = (u32)in64_phdr.p_memsz;*/ mem_siz = (u32)(final_exec_addr - in64_phdr.p_vaddr); + note_sz = note_base = offset = 0; + if ( num_phdrs > 1 ) + { + offset = in64_phdr.p_offset; + note_base = in64_phdr.p_vaddr; + + (void)lseek(infd, in64_ehdr.e_phoff+sizeof(in64_phdr), SEEK_SET); + do_read(infd, &in64_phdr, sizeof(in64_phdr)); + endianadjust_phdr64(&in64_phdr); + + (void)lseek(infd, offset, SEEK_SET); + + note_sz = in64_phdr.p_memsz; + note_base = in64_phdr.p_vaddr - note_base; + + if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset ) + { + fprintf(stderr, "Expected .note section within .text section!\n" \ + "Offset %ld not within %d!\n", + in64_phdr.p_offset, dat_siz); + return 1; + } + /* Gets us the absolute offset within the .text section. */ + offset = in64_phdr.p_offset - offset; + } + /* * End the image on a page boundary. This gets round alignment bugs * in the boot- or chain-loader (e.g., kexec on the XenoBoot CD). @@ -322,6 +382,31 @@ int main(int argc, char **argv) out_shdr[1].sh_size = dat_siz; out_shdr[2].sh_offset = RAW_OFFSET + dat_siz + sizeof(out_shdr); + if ( num_phdrs > 1 ) + { + /* We have two of them! */ + out_ehdr.e_phnum = num_phdrs; + /* Extra .note section. */ + out_ehdr.e_shnum++; + + /* Fill out the PT_NOTE program header. */ + note_phdr.p_vaddr = note_base; + note_phdr.p_paddr = note_base; + note_phdr.p_filesz = note_sz; + note_phdr.p_memsz = note_sz; + note_phdr.p_offset = offset; + + /* Tack on the .note\0 */ + out_shdr[2].sh_size += sizeof(out_shstrtab_extra); + /* And move it past the .note section. */ + out_shdr[2].sh_offset += sizeof(out_shdr_extra); + + /* Fill out the .note section. */ + out_shdr_extra.sh_size = note_sz; + out_shdr_extra.sh_addr = note_base; + out_shdr_extra.sh_offset = RAW_OFFSET + offset; + } + outfd = open(outimage, O_WRONLY|O_CREAT|O_TRUNC, 0775); if ( outfd == -1 ) { @@ -335,8 +420,15 @@ int main(int argc, char **argv) endianadjust_phdr32(&out_phdr); do_write(outfd, &out_phdr, sizeof(out_phdr)); - - if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr)) < 0 ) + + if ( num_phdrs > 1 ) + { + endianadjust_phdr32(¬e_phdr); + do_write(outfd, ¬e_phdr, sizeof(note_phdr)); + } + + if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr) - + ( num_phdrs > 1 ? sizeof(note_phdr) : 0 ) ) < 0 ) { fprintf(stderr, "Header overflow.\n"); return 1; @@ -355,9 +447,22 @@ int main(int argc, char **argv) endianadjust_shdr32(&out_shdr[i]); do_write(outfd, &out_shdr[0], sizeof(out_shdr)); - do_write(outfd, out_shstrtab, sizeof(out_shstrtab)); - do_write(outfd, buffer, 4-((sizeof(out_shstrtab)+dat_siz)&3)); - + if ( num_phdrs > 1 ) + { + endianadjust_shdr32(&out_shdr_extra); + /* Append the .note section. */ + do_write(outfd, &out_shdr_extra, sizeof(out_shdr_extra)); + /* The normal strings - .text\0.. */ + do_write(outfd, out_shstrtab, sizeof(out_shstrtab)); + /* Our .note */ + do_write(outfd, out_shstrtab_extra, sizeof(out_shstrtab_extra)); + do_write(outfd, buffer, 4-((sizeof(out_shstrtab)+sizeof(out_shstrtab_extra)+dat_siz)&3)); + } + else + { + do_write(outfd, out_shstrtab, sizeof(out_shstrtab)); + do_write(outfd, buffer, 4-((sizeof(out_shstrtab)+dat_siz)&3)); + } close(infd); close(outfd); diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 5eb825e..d8661e3 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -31,6 +31,9 @@ OUTPUT_ARCH(i386:x86-64) PHDRS { text PT_LOAD ; +#if defined(BUILD_ID) && !defined(EFI) + note PT_NOTE ; +#endif } SECTIONS { @@ -78,6 +81,11 @@ SECTIONS *(.rodata) *(.rodata.*) +#if defined(BUILD_ID) && defined(EFI) + __note_gnu_build_id_start = .; + *(.note.gnu.build-id) + __note_gnu_build_id_end = .; +#endif . = ALIGN(8); /* Exception table */ @@ -99,6 +107,21 @@ SECTIONS _erodata = .; } :text +#if defined(BUILD_ID) && !defined(EFI) +/* + * No mechanism to put an PT_NOTE in the EFI file - so put + * it in .data section. + */ + . = ALIGN(4); + .note : { + __note_gnu_build_id_start = .; + *(.note.gnu.build-id) + __note_gnu_build_id_end = .; + *(.note) + *(.note.*) + } :note :text +#endif + #ifdef EFI . = ALIGN(MB(2)); #else diff --git a/xen/common/version.c b/xen/common/version.c index fc9bf42..35078c7 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -1,5 +1,9 @@ #include <xen/compile.h> +#include <xen/errno.h> +#include <xen/string.h> +#include <xen/types.h> #include <xen/version.h> +#include <xen/elf.h> const char *xen_compile_date(void) { @@ -61,6 +65,53 @@ const char *xen_deny(void) return "<denied>"; } +#ifdef BUILD_ID +#define NT_GNU_BUILD_ID 3 +/* Defined in linker script. */ +extern const Elf_Note __note_gnu_build_id_start[], __note_gnu_build_id_end[]; + +int xen_build_id(const void **p, unsigned int *len) +{ + const Elf_Note *n = __note_gnu_build_id_start; + static bool_t checked = 0; + + if ( checked ) + { + *len = n->descsz; + *p = ELFNOTE_DESC(n); + return 0; + } + /* --build-id invoked with wrong parameters. */ + if ( __note_gnu_build_id_end <= __note_gnu_build_id_start ) + return -ENODATA; + + /* Check for full Note header. */ + if ( &n[1] > __note_gnu_build_id_end ) + return -ENODATA; + + /* Check if we really have a build-id. */ + if ( NT_GNU_BUILD_ID != n->type ) + return -ENODATA; + + /* Sanity check, name should be "GNU" for ld-generated build-id. */ + if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 ) + return -ENODATA; + + *len = n->descsz; + *p = ELFNOTE_DESC(n); + + checked = 1; + return 0; +} + +#else + +int xen_build_id(const void **p, unsigned int *len) +{ + return -ENODATA; +} +#endif + /* * Local variables: * mode: C diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h index 2015c0b..4b57e18 100644 --- a/xen/include/xen/version.h +++ b/xen/include/xen/version.h @@ -14,4 +14,7 @@ const char *xen_changeset(void); const char *xen_banner(void); const char *xen_deny(void); +#include <xen/types.h> +int xen_build_id(const void **p, unsigned int *len); + #endif /* __XEN_VERSION_H__ */