| Message ID | 1474043908-12101-13-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: > Most of the WARN_ON or BUG_ON sections are properly aligned on > x86. However on ARM and on x86 assembler the macros don't include > any aligment information - hence they end up being the default > byte granularity. > > On ARM32 it is paramount that the aligment is word-size (4) > otherwise if one tries to use (uint32_t*) access (such > as livepatch ELF relocations) we get a Data Abort. > > Enforcing bug_frames to have the proper aligment across all > architectures and in both C and x86 makes them all the same. > > Furthermore on x86 the bloat-o-meter detects that with this > change: > > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) > function old new delta > > On ARM32: > add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96) > function old new delta > gnttab_unpopulate_status_frames - 384 +384 > do_grant_table_op 10808 10520 -288 > > And ARM64: > add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72) > function old new delta > gnttab_map_grant_ref - 4164 +4164 > do_grant_table_op 9892 9836 -56 > grant_map_exists 300 - -300 > __gnttab_map_grant_ref 3880 - -3880 > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> x86 parts: Acked-by: Jan Beulich <jbeulich@suse.com>
Hi Konrad, On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote: > Most of the WARN_ON or BUG_ON sections are properly aligned on > x86. However on ARM and on x86 assembler the macros don't include > any aligment information - hence they end up being the default s/aligment/alignment/ > byte granularity. > > On ARM32 it is paramount that the aligment is word-size (4) Ditto > otherwise if one tries to use (uint32_t*) access (such > as livepatch ELF relocations) we get a Data Abort. > > Enforcing bug_frames to have the proper aligment across all Ditto > architectures and in both C and x86 makes them all the same. > > Furthermore on x86 the bloat-o-meter detects that with this > change: > > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) > function old new delta > > On ARM32: > add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96) > function old new delta > gnttab_unpopulate_status_frames - 384 +384 > do_grant_table_op 10808 10520 -288 > > And ARM64: > add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72) > function old new delta > gnttab_map_grant_ref - 4164 +4164 > do_grant_table_op 9892 9836 -56 > grant_map_exists 300 - -300 > __gnttab_map_grant_ref 3880 - -3880 > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > Cc: Julien Grall <julien.grall@arm.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > v3: First submission. Replaces the "livepatch/elf: Adjust section aligment to word" > patch. > v4: Remove the .ALIGN(4) in xen.lds.S for x86 (the only place needing > that change). > Update the commit description with correct x86 results > Remove the . = ALIGN(4); in linker filer on x86 [the only file needing the change] > --- > xen/arch/x86/xen.lds.S | 1 - > xen/include/asm-arm/bug.h | 1 + > xen/include/asm-x86/bug.h | 1 + > 3 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index d903c31..7676de9 100644 > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -79,7 +79,6 @@ SECTIONS > .rodata : { > _srodata = .; > /* Bug frames table */ > - . = ALIGN(4); > __start_bug_frames = .; > *(.bug_frames.0) > __stop_bug_frames_0 = .; > diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h > index 68353e1..773d63e 100644 > --- a/xen/include/asm-arm/bug.h > +++ b/xen/include/asm-arm/bug.h > @@ -52,6 +52,7 @@ struct bug_frame { > ".popsection\n" \ > ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ > "4:\n" \ > + ".align 4\n" \ > ".long (1b - 4b)\n" \ > ".long (2b - 4b)\n" \ > ".long (3b - 4b)\n" \ > diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h > index c5d2d4c..9bb4a19 100644 > --- a/xen/include/asm-x86/bug.h > +++ b/xen/include/asm-x86/bug.h > @@ -98,6 +98,7 @@ extern const struct bug_frame __start_bug_frames[], > .popsection > > .pushsection .bug_frames.\type, "a", @progbits > + .p2align 2 > .L\@bf: > .long (.L\@ud - .L\@bf) + \ > ((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH) > Regards,
On 19/09/2016 16:34, Julien Grall wrote: > Hi Konrad, > > On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote: >> Most of the WARN_ON or BUG_ON sections are properly aligned on >> x86. However on ARM and on x86 assembler the macros don't include >> any aligment information - hence they end up being the default > > s/aligment/alignment/ > >> byte granularity. >> >> On ARM32 it is paramount that the aligment is word-size (4) > > Ditto > >> otherwise if one tries to use (uint32_t*) access (such >> as livepatch ELF relocations) we get a Data Abort. >> >> Enforcing bug_frames to have the proper aligment across all > > Ditto > >> architectures and in both C and x86 makes them all the same. >> >> Furthermore on x86 the bloat-o-meter detects that with this >> change: >> >> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) >> function old new delta >> >> On ARM32: >> add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96) >> function old new delta >> gnttab_unpopulate_status_frames - 384 +384 >> do_grant_table_op 10808 10520 -288 >> >> And ARM64: >> add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72) >> function old new delta >> gnttab_map_grant_ref - 4164 +4164 >> do_grant_table_op 9892 9836 -56 >> grant_map_exists 300 - -300 >> __gnttab_map_grant_ref 3880 - -3880 >> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> I forgot to mention that with those NITs fixed: Reviewed-by: Julien Grall <julien.grall@arm.com> >> --- >> Cc: Julien Grall <julien.grall@arm.com> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> >> v3: First submission. Replaces the "livepatch/elf: Adjust section >> aligment to word" >> patch. >> v4: Remove the .ALIGN(4) in xen.lds.S for x86 (the only place needing >> that change). >> Update the commit description with correct x86 results >> Remove the . = ALIGN(4); in linker filer on x86 [the only file >> needing the change] >> --- >> xen/arch/x86/xen.lds.S | 1 - >> xen/include/asm-arm/bug.h | 1 + >> xen/include/asm-x86/bug.h | 1 + >> 3 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S >> index d903c31..7676de9 100644 >> --- a/xen/arch/x86/xen.lds.S >> +++ b/xen/arch/x86/xen.lds.S >> @@ -79,7 +79,6 @@ SECTIONS >> .rodata : { >> _srodata = .; >> /* Bug frames table */ >> - . = ALIGN(4); >> __start_bug_frames = .; >> *(.bug_frames.0) >> __stop_bug_frames_0 = .; >> diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h >> index 68353e1..773d63e 100644 >> --- a/xen/include/asm-arm/bug.h >> +++ b/xen/include/asm-arm/bug.h >> @@ -52,6 +52,7 @@ struct bug_frame { >> >> ".popsection\n" \ >> ".pushsection .bug_frames." __stringify(type) ", \"a\", >> %progbits\n"\ >> >> "4:\n" \ >> + ".align >> 4\n" \ >> ".long (1b - >> 4b)\n" \ >> ".long (2b - >> 4b)\n" \ >> ".long (3b - >> 4b)\n" \ >> diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h >> index c5d2d4c..9bb4a19 100644 >> --- a/xen/include/asm-x86/bug.h >> +++ b/xen/include/asm-x86/bug.h >> @@ -98,6 +98,7 @@ extern const struct bug_frame __start_bug_frames[], >> .popsection >> >> .pushsection .bug_frames.\type, "a", @progbits >> + .p2align 2 >> .L\@bf: >> .long (.L\@ud - .L\@bf) + \ >> ((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH) >> > > Regards, >
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index d903c31..7676de9 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -79,7 +79,6 @@ SECTIONS .rodata : { _srodata = .; /* Bug frames table */ - . = ALIGN(4); __start_bug_frames = .; *(.bug_frames.0) __stop_bug_frames_0 = .; diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h index 68353e1..773d63e 100644 --- a/xen/include/asm-arm/bug.h +++ b/xen/include/asm-arm/bug.h @@ -52,6 +52,7 @@ struct bug_frame { ".popsection\n" \ ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ "4:\n" \ + ".align 4\n" \ ".long (1b - 4b)\n" \ ".long (2b - 4b)\n" \ ".long (3b - 4b)\n" \ diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h index c5d2d4c..9bb4a19 100644 --- a/xen/include/asm-x86/bug.h +++ b/xen/include/asm-x86/bug.h @@ -98,6 +98,7 @@ extern const struct bug_frame __start_bug_frames[], .popsection .pushsection .bug_frames.\type, "a", @progbits + .p2align 2 .L\@bf: .long (.L\@ud - .L\@bf) + \ ((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
Most of the WARN_ON or BUG_ON sections are properly aligned on x86. However on ARM and on x86 assembler the macros don't include any aligment information - hence they end up being the default byte granularity. On ARM32 it is paramount that the aligment is word-size (4) otherwise if one tries to use (uint32_t*) access (such as livepatch ELF relocations) we get a Data Abort. Enforcing bug_frames to have the proper aligment across all architectures and in both C and x86 makes them all the same. Furthermore on x86 the bloat-o-meter detects that with this change: add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) function old new delta On ARM32: add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96) function old new delta gnttab_unpopulate_status_frames - 384 +384 do_grant_table_op 10808 10520 -288 And ARM64: add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72) function old new delta gnttab_map_grant_ref - 4164 +4164 do_grant_table_op 9892 9836 -56 grant_map_exists 300 - -300 __gnttab_map_grant_ref 3880 - -3880 Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Julien Grall <julien.grall@arm.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> v3: First submission. Replaces the "livepatch/elf: Adjust section aligment to word" patch. v4: Remove the .ALIGN(4) in xen.lds.S for x86 (the only place needing that change). Update the commit description with correct x86 results Remove the . = ALIGN(4); in linker filer on x86 [the only file needing the change] --- xen/arch/x86/xen.lds.S | 1 - xen/include/asm-arm/bug.h | 1 + xen/include/asm-x86/bug.h | 1 + 3 files changed, 2 insertions(+), 1 deletion(-)