Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.
Hi Konrad, On 19/09/2016 22:19, Konrad Rzeszutek Wilk wrote: Signed-off-by: Konrad Rzeszutek Wilk I forgot to mention that with those NITs fixed: Reviewed-by: Julien Grall Thanks. However I noticed (and I recall having it replaced in earlier versions so I must have in a hurry missed it), but this: --- 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" \ [fixed up, your mailer mangled it] This should have been: diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h index 773d63e..affe64f 100644 --- a/xen/include/asm-arm/bug.h +++ b/xen/include/asm-arm/bug.h @@ -52,7 +52,7 @@ struct bug_frame { ".popsection\n"\ ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ "4:\n" \ - ".align 4\n" \ + ".p2align 4\n" \ ".long (1b - 4b)\n"\ ".long (2b - 4b)\n"\ ".long (3b - 4b)\n"\ So that it would be in sync with x86 version. The end result is exactly the same - it is just that p2align is preferred because it has the same semantics across platforms while .align differs. I made the change (along with your recommendations) and put your Reviewed-by. I hope that is OK. I am fine with that. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.
On Mon, Sep 19, 2016 at 04:19:55PM -0400, Konrad Rzeszutek Wilk wrote: > > > > Signed-off-by: Konrad Rzeszutek Wilk > > > > I forgot to mention that with those NITs fixed: > > > > Reviewed-by: Julien Grall > > Thanks. > > However I noticed (and I recall having it replaced in earlier > versions so I must have in a hurry missed it), but this: > > > > > --- 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" > > > > \ > > [fixed up, your mailer mangled it] > This should have been: > > diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h > index 773d63e..affe64f 100644 > --- a/xen/include/asm-arm/bug.h > +++ b/xen/include/asm-arm/bug.h > @@ -52,7 +52,7 @@ struct bug_frame { > ".popsection\n"\ > ".pushsection .bug_frames." __stringify(type) ", \"a\", > %progbits\n"\ > "4:\n" \ > - ".align 4\n" \ > + ".p2align 4\n" \ Argh. '.p2align 2' ! For reference, here is the full patch: From ecf25f594118042364367d0bfacb3ee25046 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Wed, 7 Sep 2016 11:57:05 -0400 Subject: [PATCH] bug/x86/arm: Align bug_frames sections. 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 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. Enforcing bug_frames to have the proper alignment 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 98929836 -56 grant_map_exists 300 --300 __gnttab_map_grant_ref 3880 - -3880 Reviewed-by: Julien Grall Acked-by: Jan Beulich [x86 parts] Signed-off-by: Konrad Rzeszutek Wilk --- Cc: Julien Grall Cc: Stefano Stabellini Cc: Jan Beulich Cc: Andrew Cooper 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] v5: Add Jan's Ack on x86 parts. v6: Added Julien's Reviewed-by s/aligment/alignment/ s/align 4/p2align 2/ as the align semnatics varies by platforms, while p2align is the same across all of them. --- 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..4704e2d 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" \ + ".p2align 2\n" \ ".long (1b - 4b)\n"\ ".long (2b - 4b)\n"
Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.
> > > Signed-off-by: Konrad Rzeszutek Wilk > > I forgot to mention that with those NITs fixed: > > Reviewed-by: Julien Grall Thanks. However I noticed (and I recall having it replaced in earlier versions so I must have in a hurry missed it), but this: > > > --- 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" > > >\ [fixed up, your mailer mangled it] This should have been: diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h index 773d63e..affe64f 100644 --- a/xen/include/asm-arm/bug.h +++ b/xen/include/asm-arm/bug.h @@ -52,7 +52,7 @@ struct bug_frame { ".popsection\n"\ ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ "4:\n" \ - ".align 4\n" \ + ".p2align 4\n" \ ".long (1b - 4b)\n"\ ".long (2b - 4b)\n"\ ".long (3b - 4b)\n"\ So that it would be in sync with x86 version. The end result is exactly the same - it is just that p2align is preferred because it has the same semantics across platforms while .align differs. I made the change (along with your recommendations) and put your Reviewed-by. I hope that is OK. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.
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 98929836 -56 grant_map_exists 300 --300 __gnttab_map_grant_ref 3880 - -3880 Signed-off-by: Konrad Rzeszutek Wilk --- Cc: Julien Grall Cc: Stefano Stabellini Cc: Jan Beulich Cc: Andrew Cooper 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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.
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 98929836 -56 grant_map_exists 300 --300 __gnttab_map_grant_ref 3880 - -3880 Signed-off-by: Konrad Rzeszutek Wilk I forgot to mention that with those NITs fixed: Reviewed-by: Julien Grall --- Cc: Julien Grall Cc: Stefano Stabellini Cc: Jan Beulich Cc: Andrew Cooper 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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.
>>> On 16.09.16 at 18:38, 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 98929836 -56 > grant_map_exists 300 --300 > __gnttab_map_grant_ref 3880 - -3880 > > Signed-off-by: Konrad Rzeszutek Wilk x86 parts: Acked-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel