Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.

2016-09-20 Thread Julien Grall 

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.

2016-09-19 Thread Konrad Rzeszutek Wilk 
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"

Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.

2016-09-19 Thread Konrad Rzeszutek Wilk 
> > > 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.

2016-09-19 Thread Julien Grall 

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.

2016-09-19 Thread Julien Grall 



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.

2016-09-19 Thread Jan Beulich 
>>> 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