| Message ID | 20170912003726.368-7-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Konrad, On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote: > By default when using objcopy we lose the alignment when we copy it from xen-syms - > with the result that alignment (on ARM32 for example) can be 1: > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > .. > [ 6] .livepatch.depend PROGBITS 00000000 000093 000024 00 A 0 0 1 > > That, combined with wacky offset means it will be loaded in > memory with the wrong alignment: > > (XEN) livepatch.c:425: livepatch: xen_bye_world: Loaded .livepatch.depends at 000a08043 > > And later we get: > (XEN) livepatch.c:501: livepatch: xen_bye_world: .livepatch.depends is not aligned properly! > > This fix forces all the test-cases to be built with a > .livepatch.depends structure containing the build-id extracted from > the hypervisor (except the xen_bye_world test-case). > > We use the 'mkhex' tool instead of 'xxd' as the end result is an 'unsigned' > instead of 'char' type array - which naturally forces the alignment to be of four. > Also the 'mkhex' tools allows us to pass the section name as parameter. > > The end result is much better alignment: > > [ 7] .livepatch.depend PROGBITS 00000000 000094 000024 00 A 0 0 4 > > Note that thanks to 'unsigned int .. __note_depends' the symbol becomes > global: > > $ readelf --symbols *.livepatch | grep depen > 23: 0000000000000000 36 OBJECT GLOBAL HIDDEN 6 note_depends > 49: 0000000000000000 36 OBJECT GLOBAL HIDDEN 17 note_depends > 16: 0000000000000000 36 OBJECT GLOBAL HIDDEN 3 note_depends > 21: 0000000000000000 36 OBJECT GLOBAL HIDDEN 6 note_depends > > See patch titled: "livepatch/arm/x86: Strip note_depends symbol from test-cases." > which fixes this. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > v2: First version > --- > docs/misc/livepatch.markdown | 2 ++ > xen/test/livepatch/Makefile | 56 +++++++++++++++------------------- > xen/test/livepatch/xen_bye_world.c | 1 + > xen/test/livepatch/xen_hello_world.c | 1 + > xen/test/livepatch/xen_nop.c | 1 + > xen/test/livepatch/xen_replace_world.c | 1 + > 6 files changed, 31 insertions(+), 31 deletions(-) > > diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown > index 505dc37cda..922a64436f 100644 > --- a/docs/misc/livepatch.markdown > +++ b/docs/misc/livepatch.markdown > @@ -430,6 +430,8 @@ checksum, MD5 checksum or any unique value. > > The size of these structures varies with the --build-id linker option. > > +On ARM32 this section must by four-byte aligned. > + > ## Hypercalls > > We will employ the sub operations of the system management hypercall (sysctl). > diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile > index 6831383db1..89ad89dfd5 100644 > --- a/xen/test/livepatch/Makefile > +++ b/xen/test/livepatch/Makefile > @@ -1,15 +1,7 @@ > include $(XEN_ROOT)/Config.mk > > -ifeq ($(XEN_TARGET_ARCH),x86_64) > -OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64 > -endif > -ifeq ($(XEN_TARGET_ARCH),arm64) > -OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64 > -endif > -ifeq ($(XEN_TARGET_ARCH),arm32) > -OBJCOPY_MAGIC := -I binary -O elf32-littlearm -B arm > -endif > - > +NOTE_SYMBOL = "note_depends" > +NOTE_DEPENDS = "const __section(\".livepatch.depends\") $(NOTE_SYMBOL)" > CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}') > CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}') > > @@ -38,7 +30,7 @@ uninstall: > > .PHONY: clean > clean:: > - rm -f *.o .*.o.d *.livepatch config.h > + rm -f *.o .*.o.d *.livepatch config.h livepatch_depends.h hello_world_livepatch_depends.h *.bin > > # > # To compute these values we need the binary files: xen-syms > @@ -56,10 +48,10 @@ config.h: xen_hello_world_func.o > echo "#define MINOR_VERSION_ADDR $(MINOR_VERSION_ADDR)"; \ > echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@ > > -xen_hello_world.o: config.h > +xen_hello_world.o: config.h livepatch_depends.h > > .PHONY: $(LIVEPATCH) > -$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o > +$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o > $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^ > > # > @@ -71,40 +63,42 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o > # not be built (it is for EFI builds), and that we do not have > # the note.o.bin to muck with (as it gets deleted) > # > -.PHONY: note.o > -note.o: > - $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin > - $(OBJCOPY) $(OBJCOPY_MAGIC) \ > - --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@ > - rm -f $@.bin > +.PHONY: note.bin > +note.bin: > + $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@ > + > +.PHONY: livepatch_depends.h > +livepatch_depends.h: note.bin > + $(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@)) It looks quite odd to use a file in firmware/hvmloader for livepatch. Would it be possible to move mkhex to a generic place? Cheers,
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 505dc37cda..922a64436f 100644 --- a/docs/misc/livepatch.markdown +++ b/docs/misc/livepatch.markdown @@ -430,6 +430,8 @@ checksum, MD5 checksum or any unique value. The size of these structures varies with the --build-id linker option. +On ARM32 this section must by four-byte aligned. + ## Hypercalls We will employ the sub operations of the system management hypercall (sysctl). diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile index 6831383db1..89ad89dfd5 100644 --- a/xen/test/livepatch/Makefile +++ b/xen/test/livepatch/Makefile @@ -1,15 +1,7 @@ include $(XEN_ROOT)/Config.mk -ifeq ($(XEN_TARGET_ARCH),x86_64) -OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64 -endif -ifeq ($(XEN_TARGET_ARCH),arm64) -OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64 -endif -ifeq ($(XEN_TARGET_ARCH),arm32) -OBJCOPY_MAGIC := -I binary -O elf32-littlearm -B arm -endif - +NOTE_SYMBOL = "note_depends" +NOTE_DEPENDS = "const __section(\".livepatch.depends\") $(NOTE_SYMBOL)" CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}') CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}') @@ -38,7 +30,7 @@ uninstall: .PHONY: clean clean:: - rm -f *.o .*.o.d *.livepatch config.h + rm -f *.o .*.o.d *.livepatch config.h livepatch_depends.h hello_world_livepatch_depends.h *.bin # # To compute these values we need the binary files: xen-syms @@ -56,10 +48,10 @@ config.h: xen_hello_world_func.o echo "#define MINOR_VERSION_ADDR $(MINOR_VERSION_ADDR)"; \ echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@ -xen_hello_world.o: config.h +xen_hello_world.o: config.h livepatch_depends.h .PHONY: $(LIVEPATCH) -$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o +$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^ # @@ -71,40 +63,42 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o # not be built (it is for EFI builds), and that we do not have # the note.o.bin to muck with (as it gets deleted) # -.PHONY: note.o -note.o: - $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin - $(OBJCOPY) $(OBJCOPY_MAGIC) \ - --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@ - rm -f $@.bin +.PHONY: note.bin +note.bin: + $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@ + +.PHONY: livepatch_depends.h +livepatch_depends.h: note.bin + $(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@)) # # Extract the build-id of the xen_hello_world.livepatch # (which xen_bye_world will depend on). # -.PHONY: hello_world_note.o -hello_world_note.o: $(LIVEPATCH) - $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin - $(OBJCOPY) $(OBJCOPY_MAGIC) \ - --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@ - rm -f $@.bin +.PHONY: hello_world_note.bin +hello_world_note.bin: $(LIVEPATCH) + $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@ + +.PHONY: hello_world_livepatch_depends.h +hello_world_livepatch_depends.h: hello_world_note.bin + $(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@)) -xen_bye_world.o: config.h +xen_bye_world.o: config.h hello_world_livepatch_depends.h .PHONY: $(LIVEPATCH_BYE) -$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o +$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^ -xen_replace_world.o: config.h +xen_replace_world.o: config.h livepatch_depends.h .PHONY: $(LIVEPATCH_REPLACE) -$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o +$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^ -xen_nop.o: config.h +xen_nop.o: config.h livepatch_depends.h .PHONY: $(LIVEPATCH_NOP) -$(LIVEPATCH_NOP): xen_nop.o note.o +$(LIVEPATCH_NOP): xen_nop.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^ .PHONY: livepatch diff --git a/xen/test/livepatch/xen_bye_world.c b/xen/test/livepatch/xen_bye_world.c index 2700f0eedd..935e76ca8b 100644 --- a/xen/test/livepatch/xen_bye_world.c +++ b/xen/test/livepatch/xen_bye_world.c @@ -10,6 +10,7 @@ #include <xen/livepatch.h> #include <public/sysctl.h> +#include "hello_world_livepatch_depends.h" static const char bye_world_patch_this_fnc[] = "xen_extra_version"; extern const char *xen_bye_world(void); diff --git a/xen/test/livepatch/xen_hello_world.c b/xen/test/livepatch/xen_hello_world.c index 02f3f85dc0..988a3b14f4 100644 --- a/xen/test/livepatch/xen_hello_world.c +++ b/xen/test/livepatch/xen_hello_world.c @@ -11,6 +11,7 @@ #include <xen/livepatch_payload.h> #include <public/sysctl.h> +#include "livepatch_depends.h" static const char hello_world_patch_this_fnc[] = "xen_extra_version"; extern const char *xen_hello_world(void); diff --git a/xen/test/livepatch/xen_nop.c b/xen/test/livepatch/xen_nop.c index a224b7c670..8d0c8f5097 100644 --- a/xen/test/livepatch/xen_nop.c +++ b/xen/test/livepatch/xen_nop.c @@ -7,6 +7,7 @@ #include <xen/types.h> #include <public/sysctl.h> +#include "livepatch_depends.h" /* * All of the .new_size and .old_addr are based on assumptions that the diff --git a/xen/test/livepatch/xen_replace_world.c b/xen/test/livepatch/xen_replace_world.c index 78a8f528b3..a653cc4268 100644 --- a/xen/test/livepatch/xen_replace_world.c +++ b/xen/test/livepatch/xen_replace_world.c @@ -9,6 +9,7 @@ #include <xen/livepatch.h> #include <public/sysctl.h> +#include "livepatch_depends.h" static const char xen_replace_world_name[] = "xen_extra_version"; extern const char *xen_replace_world(void);
By default when using objcopy we lose the alignment when we copy it from xen-syms - with the result that alignment (on ARM32 for example) can be 1: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al .. [ 6] .livepatch.depend PROGBITS 00000000 000093 000024 00 A 0 0 1 That, combined with wacky offset means it will be loaded in memory with the wrong alignment: (XEN) livepatch.c:425: livepatch: xen_bye_world: Loaded .livepatch.depends at 000a08043 And later we get: (XEN) livepatch.c:501: livepatch: xen_bye_world: .livepatch.depends is not aligned properly! This fix forces all the test-cases to be built with a .livepatch.depends structure containing the build-id extracted from the hypervisor (except the xen_bye_world test-case). We use the 'mkhex' tool instead of 'xxd' as the end result is an 'unsigned' instead of 'char' type array - which naturally forces the alignment to be of four. Also the 'mkhex' tools allows us to pass the section name as parameter. The end result is much better alignment: [ 7] .livepatch.depend PROGBITS 00000000 000094 000024 00 A 0 0 4 Note that thanks to 'unsigned int .. __note_depends' the symbol becomes global: $ readelf --symbols *.livepatch | grep depen 23: 0000000000000000 36 OBJECT GLOBAL HIDDEN 6 note_depends 49: 0000000000000000 36 OBJECT GLOBAL HIDDEN 17 note_depends 16: 0000000000000000 36 OBJECT GLOBAL HIDDEN 3 note_depends 21: 0000000000000000 36 OBJECT GLOBAL HIDDEN 6 note_depends See patch titled: "livepatch/arm/x86: Strip note_depends symbol from test-cases." which fixes this. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- v2: First version --- docs/misc/livepatch.markdown | 2 ++ xen/test/livepatch/Makefile | 56 +++++++++++++++------------------- xen/test/livepatch/xen_bye_world.c | 1 + xen/test/livepatch/xen_hello_world.c | 1 + xen/test/livepatch/xen_nop.c | 1 + xen/test/livepatch/xen_replace_world.c | 1 + 6 files changed, 31 insertions(+), 31 deletions(-)