| Message ID | 20170912003726.368-6-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote: > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -155,11 +155,9 @@ SECTIONS > __initcall_end = .; > > #ifdef CONFIG_HAS_ALTERNATIVE > - . = ALIGN(4); I think this one needs to say, while ... > __alt_instructions = .; > *(.altinstructions) > __alt_instructions_end = .; > - . = ALIGN(4); ... this one can go and ... > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -202,7 +202,6 @@ SECTIONS > * "Alternative instructions for different CPU types or capabilities" > * Think locking instructions on spinlocks. > */ > - . = ALIGN(8); > __alt_instructions = .; ... this one again needs to stay, but the 8 can be replaced by a 4. That's because otherwise there's the risk of the __alt_instructions label to not be at the start of the first .altinstructions (solely depending on what happens to be immediately before this script section). I'm sorry for the earlier mis-guiding of mine. > --- a/xen/include/asm-x86/alternative.h > +++ b/xen/include/asm-x86/alternative.h > @@ -56,6 +56,7 @@ extern void alternative_instructions(void); > > #define ALTERNATIVE_N(newinstr, feature, number) \ > ".pushsection .altinstructions,\"a\"\n" \ > + ".p2align 2\n" \ > ALTINSTR_ENTRY(feature, number) \ I think the would better go into ALTINSTR_ENTRY(), and then also into the altinstruction_entry assembler macro. Jan
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index c9b9546435..447d33888f 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -155,11 +155,9 @@ SECTIONS __initcall_end = .; #ifdef CONFIG_HAS_ALTERNATIVE - . = ALIGN(4); __alt_instructions = .; *(.altinstructions) __alt_instructions_end = .; - . = ALIGN(4); *(.altinstr_replacement) #endif diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index d5e8821d41..9eb42048d5 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -202,7 +202,6 @@ SECTIONS * "Alternative instructions for different CPU types or capabilities" * Think locking instructions on spinlocks. */ - . = ALIGN(8); __alt_instructions = .; *(.altinstructions) __alt_instructions_end = .; diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h index 6cc9d0dc5f..cd1373fdd5 100644 --- a/xen/include/asm-arm/alternative.h +++ b/xen/include/asm-arm/alternative.h @@ -54,9 +54,11 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en oldinstr "\n" \ "662:\n" \ ".pushsection .altinstructions,\"a\"\n" \ + ".p2align 2\n" \ ALTINSTR_ENTRY(feature) \ ".popsection\n" \ ".pushsection .altinstr_replacement, \"a\"\n" \ + ".p2align 2\n" \ "663:\n\t" \ newinstr "\n" \ "664:\n\t" \ @@ -84,6 +86,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en .if \enable 661: \insn1 662: .pushsection .altinstructions, "a" + .p2align 2 altinstruction_entry 661b, 663f, \cap, 662b-661b, 664f-663f .popsection .pushsection .altinstr_replacement, "ax" @@ -103,6 +106,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en .macro alternative_if_not cap, enable = 1 .if \enable .pushsection .altinstructions, "a" + .p2align 2 altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f .popsection 661: diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h index db4f08e0e7..79430fdf05 100644 --- a/xen/include/asm-x86/alternative.h +++ b/xen/include/asm-x86/alternative.h @@ -56,6 +56,7 @@ extern void alternative_instructions(void); #define ALTERNATIVE_N(newinstr, feature, number) \ ".pushsection .altinstructions,\"a\"\n" \ + ".p2align 2\n" \ ALTINSTR_ENTRY(feature, number) \ ".section .discard,\"a\",@progbits\n" \ DISCARD_ENTRY(number) \
This is very similar to 137c59b9ff3f7a214f03b52d9c00a0a02374af1f "bug/x86/arm: Align bug_frames sections." On ARM and on x86 the C and assembler macros don't include any alignment information - hence they end up being the default byte granularity. On ARM32 it is paramount that the alignment is word-size (4) otherwise if one tries to use (uint32_t*) access (such as livepatch ELF relocations) we get a Data Abort. Specifically this issue was observed on ARM32 with a cross compiler for the livepatches. Mainly the livepatches .data section size was not padded to the section alignment: ARM32 native: Contents of section .rodata: 0000 68695f66 756e6300 63686563 6b5f666e hi_func.check_fn 0010 63000000 78656e5f 65787472 615f7665 c...xen_extra_ve 0020 7273696f 6e000000 rsion... ARM32 cross compiler: Contents of section .rodata: 0000 68695f66 756e6300 63686563 6b5f666e hi_func.check_fn 0010 63000000 78656e5f 65787472 615f7665 c...xen_extra_ve 0020 7273696f 6e00 rsion. And when we loaded it the next section would be put right behind it: native: (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 00a02000 (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 00a04024 (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .altinstructions at 00a0404c cross compiler: (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 00a02000 (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 00a04024 (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .altinstructions at 00a0404a (See 4a vs 4c) native readelf: [ 4] .rodata PROGBITS 00000000 000164 000028 00 A 0 0 4 [ 5] .altinstructions PROGBITS 00000000 00018c 00000c 00 A 0 0 1 cross compiler readelf --sections: [ 4] .rodata PROGBITS 00000000 000164 000026 00 A 0 0 4 [ 5] .altinstructions PROGBITS 00000000 00018a 00000c 00 A 0 0 1 And as can be seen the .altinstructions have alignment of 1 which from 'man elf' is: "Values of zero and one mean no alignment is required." which means we can ignore it. Enforcing .altinstructions (and also .altinstr_replacement for completness on ARM) to have the proper alignment across all architectures and in both C and x86 makes them all the same. On x86 the bloat-o-meter detects that with this change the file shrinks: add/remove: 1/0 grow/shrink: 0/2 up/down: 156/-367 (-211) function old new delta get_page_from_gfn - 156 +156 do_mmu_update 4578 4569 -9 do_mmuext_op 5604 5246 -358 Total: Before=3170439, After=3170228, chg -0.01% But as found adding even "#Hi!\n" will casue this optimization, so the bloat-o-meter value here is useless. While on ARM 32/64: add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) function old new delta Total: Before=822563, After=822563, chg +0.00% Also since the macros have the alignment coded in them there is no need to do that for the xen.lds.S anymore. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- v2: - First version v3: - Figured out the x86 bloat-o-meter results. - Removed the .ALIGN from xen.lds.S - Removed the .p2align on .altinstr_replacement per Jan's request. - Put most of the commit description for the original issue --- xen/arch/arm/xen.lds.S | 2 -- xen/arch/x86/xen.lds.S | 1 - xen/include/asm-arm/alternative.h | 4 ++++ xen/include/asm-x86/alternative.h | 1 + 4 files changed, 5 insertions(+), 3 deletions(-)