[Xen-devel] [PATCH 0/4] xen/link: Fixes and improvements to Xen's linking

[Xen-devel] [PATCH 0/4] xen/link: Fixes and improvements to Xen's linking

[Andrew Cooper]

Patch 1 came from Roger's observation that a clang/llvm-8 binary doesn't
actually boot.  All other patches are ancillary cleanup.

I'm afraid that I thing we still have further problems:

andrewcoop@andrewcoop:/local/xen.git/xen$ readelf -WS xen-syms
There are 23 section headers, starting at offset 0x13aa0b0:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .reloc            PROGBITS        ffff82d080000000 001000 000008 00   A  0   0  4
  [ 2] .text             PROGBITS        ffff82d080200000 201000 1611ba 00  AX  0   0 4096
  [ 3] .rodata           PROGBITS        ffff82d080362000 363000 0a24e8 00 WAMS  0   0 32
  [ 4] .got.plt          PROGBITS        ffff82d0804044e8 4054e8 000018 00  WA  0   0  8
  [ 5] .note.Xen         NOTE            ffff82d080404500 405500 000014 00   A  0   0  4
  [ 6] .note.gnu.build-id NOTE            ffff82d080404514 405514 000024 00   A  0   0  4
  [ 7] .init.text        PROGBITS        ffff82d080405000 406000 03baa3 00  AX  0   0 16
  [ 8] .init.data        PROGBITS        ffff82d080441000 442000 038878 00 WAMS  0   0 4096
  [ 9] .data             PROGBITS        ffff82d08047a000 47b000 018988 00  WA  0   0 4096
  [10] .bss              NOBITS          ffff82d080498000 493988 13fa80 00  WA  0   0 32768
  [11] .comment          PROGBITS        0000000000000000 493988 000060 01  MS  0   0  1
  [12] .debug_str        PROGBITS        0000000000000000 4939e8 048986 01  MS  0   0  1
  [13] .debug_loc        PROGBITS        0000000000000000 4dc36e 1e5905 00      0   0  1
  [14] .debug_abbrev     PROGBITS        0000000000000000 6c1c73 0478de 00      0   0  1
  [15] .debug_info       PROGBITS        0000000000000000 709551 948b38 00      0   0  1
  [16] .debug_ranges     PROGBITS        0000000000000000 1052089 01b650 00      0   0  1
  [17] .debug_macinfo    PROGBITS        0000000000000000 106d6d9 00015a 00      0   0  1
  [18] .debug_line       PROGBITS        0000000000000000 106d833 10f396 00      0   0  1
  [19] .debug_frame      PROGBITS        0000000000000000 117cbd0 069ca0 00      0   0  8
  [20] .symtab           SYMTAB          0000000000000000 11e6870 1985d0 18     22 69621  8
  [21] .shstrtab         STRTAB          0000000000000000 137ee40 0000e6 00      0   0  1
  [22] .strtab           STRTAB          0000000000000000 137ef26 02b185 00      0   0  1

With this fix in place, I see the unexpected .reloc which Roger noticed, but
we've got a .got.plt in there which surely can't be correct, either.

Andrew Cooper (4):
  xen/link: Cope with .rodata.cst* sections
  xen/link: Link .data.schedulers and CONSTRUCTERS in more appropriate locations
  xen/link: Fold .data.read_mostly into .data
  xen/link: Misc cleanup

 xen/arch/arm/arm32/lib/assembler.h | 19 ++---------------
 xen/arch/arm/xen.lds.S             | 43 +++++++++++++-------------------------
 xen/arch/x86/xen.lds.S             | 28 ++++++++++---------------
 3 files changed, 27 insertions(+), 63 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/4] xen/link: Cope with .rodata.cst* sections

[Andrew Cooper]

.rodata.cst* sections are used for mergable constant data, and the clang/llvm
8 toolchain has been observed to create .rodata.cst8 in a default Xen build.

Unfortunately, this section (and its .init counterpart) aren't captured by
Xen's linker globs, and end up as orphaned sections.

Generalise the data globbing to pick up cst and future special sections.

Reported-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/xen.lds.S | 9 +++------
 xen/arch/x86/xen.lds.S | 9 +++------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index e664c44..31d74a8 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -96,8 +96,7 @@ SECTIONS
        __start_schedulers_array = .;
        *(.data.schedulers)
        __end_schedulers_array = .;
-       *(.data.rel)
-       *(.data.rel.*)
+       *(.data.*)
        CONSTRUCTORS
   } :text
 
@@ -154,8 +153,7 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);
   .init.data : {
        *(.init.rodata)
-       *(.init.rodata.rel)
-       *(.init.rodata.str*)
+       *(.init.rodata.*)
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
@@ -176,8 +174,7 @@ SECTIONS
        *(.altinstr_replacement)
 
        *(.init.data)
-       *(.init.data.rel)
-       *(.init.data.rel.*)
+       *(.init.data.*)
 
        . = ALIGN(8);
        __ctors_start = .;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index cb42dc8..ec37d38 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -206,8 +206,7 @@ SECTIONS
 #endif
 
        *(.init.rodata)
-       *(.init.rodata.rel)
-       *(.init.rodata.str*)
+       *(.init.rodata.*)
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
@@ -221,8 +220,7 @@ SECTIONS
        __initcall_end = .;
 
        *(.init.data)
-       *(.init.data.rel)
-       *(.init.data.rel.*)
+       *(.init.data.*)
        . = ALIGN(4);
        __trampoline_rel_start = .;
        *(.trampoline_rel)
@@ -272,8 +270,7 @@ SECTIONS
   DECL_SECTION(.data) {
        *(.data.page_aligned)
        *(.data)
-       *(.data.rel)
-       *(.data.rel.*)
+       *(.data.*)
        CONSTRUCTORS
   } :text
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/link: Cope with .rodata.cst* sections

[Julien Grall]

Hi Andrew,

On 6/19/19 9:11 PM, Andrew Cooper wrote:
> .rodata.cst* sections are used for mergable constant data, and the clang/llvm
> 8 toolchain has been observed to create .rodata.cst8 in a default Xen build.
> 
> Unfortunately, this section (and its .init counterpart) aren't captured by
> Xen's linker globs, and end up as orphaned sections.
> 
> Generalise the data globbing to pick up cst and future special sections.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/xen.lds.S | 9 +++------
>   xen/arch/x86/xen.lds.S | 9 +++------
>   2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index e664c44..31d74a8 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -96,8 +96,7 @@ SECTIONS
>          __start_schedulers_array = .;
>          *(.data.schedulers)
>          __end_schedulers_array = .;
> -       *(.data.rel)
> -       *(.data.rel.*)
> +       *(.data.*)

My knowledge of linker is quite limited, so I might be wrong. But will 
not this match .data.vcpi & co?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/link: Cope with .rodata.cst* sections

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 10:18:42PM +0100, Julien Grall wrote:
> Hi Andrew,
> 
> On 6/19/19 9:11 PM, Andrew Cooper wrote:
> > .rodata.cst* sections are used for mergable constant data, and the clang/llvm
> > 8 toolchain has been observed to create .rodata.cst8 in a default Xen build.
> > 
> > Unfortunately, this section (and its .init counterpart) aren't captured by
> > Xen's linker globs, and end up as orphaned sections.
> > 
> > Generalise the data globbing to pick up cst and future special sections.
> > 
> > Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Wei Liu <wl@xen.org>
> > CC: Roger Pau Monné <roger.pau@citrix.com>
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Julien Grall <julien.grall@arm.com>
> > ---
> >   xen/arch/arm/xen.lds.S | 9 +++------
> >   xen/arch/x86/xen.lds.S | 9 +++------
> >   2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index e664c44..31d74a8 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -96,8 +96,7 @@ SECTIONS
> >          __start_schedulers_array = .;
> >          *(.data.schedulers)
> >          __end_schedulers_array = .;
> > -       *(.data.rel)
> > -       *(.data.rel.*)
> > +       *(.data.*)
> 
> My knowledge of linker is quite limited, so I might be wrong. But will not
> this match .data.vcpi & co?

AFAICT the x86 part of this change is fine, because the wildcard
matches are added after the more narrow matches of .data.* sections.

However for ARM the change is not correct, since the .data.* wildcard
is added before the more narrow match of .data.vpci.*. This could be
solved by moving the .data section at the end of the script (ie: after
the .init sections), like it's done on x86.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/link: Cope with .rodata.cst* sections

[Jan Beulich]

>>> On 19.06.19 at 22:11, <andrew.cooper3@citrix.com> wrote:
> .rodata.cst* sections are used for mergable constant data, and the clang/llvm
> 8 toolchain has been observed to create .rodata.cst8 in a default Xen build.

Neither this nor ...

> Unfortunately, this section (and its .init counterpart) aren't captured by
> Xen's linker globs, and end up as orphaned sections.

... this are problems on their own. The issue is if these sections end up
first in the binary (which, as we all appear to agree, should never have
happened).

> Generalise the data globbing to pick up cst and future special sections.

I'm a little wary of this, and had in the past specifically avoided adding
"too wide" a glob pattern: There's a small risk of covering a section
that's meant to be covered elsewhere. The observation of the issue
with the Arm side change is an example of such. Therefore I'd prefer
if we could get away with just adding .init.rodata.cst* and .rodata.cst*.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/4] xen/link: Link .data.schedulers and CONSTRUCTERS in more appropriate locations

[Andrew Cooper]

Neither of these should live in .data

 * .data.schedulers is only ever read, so is moved into .rodata
 * CONSTRUCTORS is only ever read, and only at boot, so is moved to beside
   .init.rodata

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/xen.lds.S | 11 ++++++-----
 xen/arch/x86/xen.lds.S | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 31d74a8..2b44e5d 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -66,6 +66,11 @@ SECTIONS
        *(.data.param)
        __param_end = .;
 
+       . = ALIGN(POINTER_ALIGN);
+       __start_schedulers_array = .;
+       *(.data.schedulers)
+       __end_schedulers_array = .;
+
        __proc_info_start = .;
        *(.proc.info)
        __proc_info_end = .;
@@ -92,12 +97,7 @@ SECTIONS
        . = ALIGN(PAGE_SIZE);
        *(.data.page_aligned)
        *(.data)
-       . = ALIGN(8);
-       __start_schedulers_array = .;
-       *(.data.schedulers)
-       __end_schedulers_array = .;
        *(.data.*)
-       CONSTRUCTORS
   } :text
 
   . = ALIGN(SMP_CACHE_BYTES);
@@ -154,6 +154,7 @@ SECTIONS
   .init.data : {
        *(.init.rodata)
        *(.init.rodata.*)
+       CONSTRUCTORS
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index ec37d38..9fa6c99 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -140,6 +140,11 @@ SECTIONS
        *(.data.param)
        __param_end = .;
 
+       . = ALIGN(POINTER_ALIGN);
+       __start_schedulers_array = .;
+       *(.data.schedulers)
+       __end_schedulers_array = .;
+
 #if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
        . = ALIGN(POINTER_ALIGN);
        __start_vpci_array = .;
@@ -207,6 +212,7 @@ SECTIONS
 
        *(.init.rodata)
        *(.init.rodata.*)
+       CONSTRUCTORS
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
@@ -261,17 +267,12 @@ SECTIONS
   . = ALIGN(SMP_CACHE_BYTES);
   DECL_SECTION(.data.read_mostly) {
        *(.data.read_mostly)
-       . = ALIGN(8);
-       __start_schedulers_array = .;
-       *(.data.schedulers)
-       __end_schedulers_array = .;
   } :text
 
   DECL_SECTION(.data) {
        *(.data.page_aligned)
        *(.data)
        *(.data.*)
-       CONSTRUCTORS
   } :text
 
   DECL_SECTION(.bss) {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen/link: Link .data.schedulers and CONSTRUCTERS in more appropriate locations

[Julien Grall]

Hi Andrew,

On 6/19/19 9:11 PM, Andrew Cooper wrote:
> Neither of these should live in .data
> 
>   * .data.schedulers is only ever read, so is moved into .rodata
>   * CONSTRUCTORS is only ever read, and only at boot, so is moved to beside
>     .init.rodata
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/xen.lds.S | 11 ++++++-----
>   xen/arch/x86/xen.lds.S | 11 ++++++-----
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 31d74a8..2b44e5d 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -66,6 +66,11 @@ SECTIONS
>          *(.data.param)
>          __param_end = .;
>   
> +       . = ALIGN(POINTER_ALIGN);

The alignment is going to be different on arm32 now. Please explain in 
the commit message why this is fine.

> +       __start_schedulers_array = .;
> +       *(.data.schedulers)
> +       __end_schedulers_array = .;
> +

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen/link: Link .data.schedulers and CONSTRUCTERS in more appropriate locations

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 09:11:25PM +0100, Andrew Cooper wrote:
> Neither of these should live in .data
> 
>  * .data.schedulers is only ever read, so is moved into .rodata
>  * CONSTRUCTORS is only ever read, and only at boot, so is moved to beside
>    .init.rodata
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

For x86:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

One comment below:

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/xen.lds.S | 11 ++++++-----
>  xen/arch/x86/xen.lds.S | 11 ++++++-----
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index ec37d38..9fa6c99 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -140,6 +140,11 @@ SECTIONS
>         *(.data.param)
>         __param_end = .;
>  
> +       . = ALIGN(POINTER_ALIGN);
> +       __start_schedulers_array = .;
> +       *(.data.schedulers)
> +       __end_schedulers_array = .;
> +
>  #if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
>         . = ALIGN(POINTER_ALIGN);
>         __start_vpci_array = .;
> @@ -207,6 +212,7 @@ SECTIONS
>  
>         *(.init.rodata)
>         *(.init.rodata.*)
> +       CONSTRUCTORS

According to the ld manual CONSTRUCTORS is only relevant for a.out,
ECOFF and XCOFF. I'm unsure whether PE does use CONSTRUCTORS or not,
since it's a descendant of COFF.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen/link: Link .data.schedulers and CONSTRUCTERS in more appropriate locations

[Andrew Cooper]

On 20/06/2019 09:40, Roger Pau Monné wrote:
> On Wed, Jun 19, 2019 at 09:11:25PM +0100, Andrew Cooper wrote:
>> Neither of these should live in .data
>>
>>  * .data.schedulers is only ever read, so is moved into .rodata
>>  * CONSTRUCTORS is only ever read, and only at boot, so is moved to beside
>>    .init.rodata
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> For x86:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> One comment below:
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/xen.lds.S | 11 ++++++-----
>>  xen/arch/x86/xen.lds.S | 11 ++++++-----
>>  2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index ec37d38..9fa6c99 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -140,6 +140,11 @@ SECTIONS
>>         *(.data.param)
>>         __param_end = .;
>>  
>> +       . = ALIGN(POINTER_ALIGN);
>> +       __start_schedulers_array = .;
>> +       *(.data.schedulers)
>> +       __end_schedulers_array = .;
>> +
>>  #if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
>>         . = ALIGN(POINTER_ALIGN);
>>         __start_vpci_array = .;
>> @@ -207,6 +212,7 @@ SECTIONS
>>  
>>         *(.init.rodata)
>>         *(.init.rodata.*)
>> +       CONSTRUCTORS
> According to the ld manual CONSTRUCTORS is only relevant for a.out,
> ECOFF and XCOFF. I'm unsure whether PE does use CONSTRUCTORS or not,
> since it's a descendant of COFF.

CONSTRUCTORS is strictly needed for the GCC coverage build to function
(hence its introduction in the first place), and any code which uses
__attribute__((constructor)) (which we use it in libxc, but not in Xen).

I'd say that the ld manual is out-of-date.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen/link: Link .data.schedulers and CONSTRUCTERS in more appropriate locations

[Jan Beulich]

>>> On 19.06.19 at 22:11, <andrew.cooper3@citrix.com> wrote:
> Neither of these should live in .data
> 
>  * .data.schedulers is only ever read, so is moved into .rodata

Which then would better be .rodata.schedulers, wouldn't it?
Same would apparently go for .data.param.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/4] xen/link: Fold .data.read_mostly into .data

[Andrew Cooper]

.data.read_mostly only needs separating from adjacent data by one cache line
to be effective, and placing it adjacent to .data.page_aligned fulfills this
requirement.

For ARM, .ex_table appears to be a vestigial remnant.  Nothing in the
resulting build ever inspects or acts on the contents of the table.  The arm32
build does however have some assembly routines which fill .ex_table.

Drop all of ARM's .ex_table infrastructure, to reduce the size of the compiled
binary, and avoid giving the illusion of exception handling working.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

Only compile tested on ARM.
---
 xen/arch/arm/arm32/lib/assembler.h | 19 ++-----------------
 xen/arch/arm/xen.lds.S             | 16 +---------------
 xen/arch/x86/xen.lds.S             |  7 ++-----
 3 files changed, 5 insertions(+), 37 deletions(-)

diff --git a/xen/arch/arm/arm32/lib/assembler.h b/xen/arch/arm/arm32/lib/assembler.h
index 6de2638..42eaabb 100644
--- a/xen/arch/arm/arm32/lib/assembler.h
+++ b/xen/arch/arm/arm32/lib/assembler.h
@@ -160,13 +160,6 @@
 	restore_irqs_notrace \oldcpsr
 	.endm
 
-#define USER(x...)				\
-9999:	x;					\
-	.pushsection __ex_table,"a";		\
-	.align	3;				\
-	.long	9999b,9001f;			\
-	.popsection
-
 #ifdef CONFIG_SMP
 #define ALT_SMP(instr...)					\
 9998:	instr
@@ -247,7 +240,7 @@
 #ifdef CONFIG_THUMB2_KERNEL
 
 	.macro	usraccoff, instr, reg, ptr, inc, off, cond, abort, t=T()
-9999:
+
 	.if	\inc == 1
 	\instr\cond\()b\()\t\().w \reg, [\ptr, #\off]
 	.elseif	\inc == 4
@@ -256,10 +249,6 @@
 	.error	"Unsupported inc macro argument"
 	.endif
 
-	.pushsection __ex_table,"a"
-	.align	3
-	.long	9999b, \abort
-	.popsection
 	.endm
 
 	.macro	usracc, instr, reg, ptr, inc, cond, rept, abort
@@ -288,7 +277,7 @@
 
 	.macro	usracc, instr, reg, ptr, inc, cond, rept, abort, t=T()
 	.rept	\rept
-9999:
+
 	.if	\inc == 1
 	\instr\cond\()b\()\t \reg, [\ptr], #\inc
 	.elseif	\inc == 4
@@ -297,10 +286,6 @@
 	.error	"Unsupported inc macro argument"
 	.endif
 
-	.pushsection __ex_table,"a"
-	.align	3
-	.long	9999b, \abort
-	.popsection
 	.endr
 	.endm
 
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 2b44e5d..3dc5117 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -94,27 +94,13 @@ SECTIONS
   _erodata = .;                /* End of read-only data */
 
   .data : {                    /* Data */
+       *(.data.read_mostly)
        . = ALIGN(PAGE_SIZE);
        *(.data.page_aligned)
        *(.data)
        *(.data.*)
   } :text
 
-  . = ALIGN(SMP_CACHE_BYTES);
-  .data.read_mostly : {
-       /* Exception table */
-       __start___ex_table = .;
-       *(.ex_table)
-       __stop___ex_table = .;
-
-       /* Pre-exception table */
-       __start___pre_ex_table = .;
-       *(.ex_table.pre)
-       __stop___pre_ex_table = .;
-
-       *(.data.read_mostly)
-  } :text
-
   . = ALIGN(8);
   .arch.info : {
       _splatform = .;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 9fa6c99..ef11949 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -264,12 +264,9 @@ SECTIONS
   __2M_init_end = .;
 
   __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
-  . = ALIGN(SMP_CACHE_BYTES);
-  DECL_SECTION(.data.read_mostly) {
-       *(.data.read_mostly)
-  } :text
-
   DECL_SECTION(.data) {
+       *(.data.read_mostly)
+       . = ALIGN(PAGE_SIZE);
        *(.data.page_aligned)
        *(.data)
        *(.data.*)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/link: Fold .data.read_mostly into .data

[Julien Grall]

On 6/19/19 9:11 PM, Andrew Cooper wrote:
> .data.read_mostly only needs separating from adjacent data by one cache line
> to be effective, and placing it adjacent to .data.page_aligned fulfills this
> requirement.
> 
> For ARM, .ex_table appears to be a vestigial remnant.  Nothing in the
> resulting build ever inspects or acts on the contents of the table.  The arm32
> build does however have some assembly routines which fill .ex_table.
> 
> Drop all of ARM's .ex_table infrastructure, to reduce the size of the compiled
> binary, and avoid giving the illusion of exception handling working.

I am not in favor of this change. assembler.h is meant to be a verbatim 
copy of the Linux counterpart.

[...]

> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 2b44e5d..3dc5117 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -94,27 +94,13 @@ SECTIONS
>     _erodata = .;                /* End of read-only data */
>   
>     .data : {                    /* Data */
> +       *(.data.read_mostly)

Before, .data.read_mostly was SMP_CACHE_BYTES aligned. Now, it seems 
there are no alignment.

This may end up to have _erodata and .data.read_mostly to be part of the 
same page. As Arm enforce read-only, you may crash on access to 
.data.read_mostly.

So I think you have
. = ALIGN(PAGE_SIZE)
*(.data.read_mostly)
.align(SMP_CACHE_BYTES).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/link: Fold .data.read_mostly into .data

[Andrew Cooper]

On 19/06/2019 22:28, Julien Grall wrote:
> On 6/19/19 9:11 PM, Andrew Cooper wrote:
>> .data.read_mostly only needs separating from adjacent data by one
>> cache line
>> to be effective, and placing it adjacent to .data.page_aligned
>> fulfills this
>> requirement.
>>
>> For ARM, .ex_table appears to be a vestigial remnant.  Nothing in the
>> resulting build ever inspects or acts on the contents of the table. 
>> The arm32
>> build does however have some assembly routines which fill .ex_table.
>>
>> Drop all of ARM's .ex_table infrastructure, to reduce the size of the
>> compiled
>> binary, and avoid giving the illusion of exception handling working.
>
> I am not in favor of this change. assembler.h is meant to be a
> verbatim copy of the Linux counterpart.

What alternative do you propose then, because having infrastructure that
gives the illusion of exception recovery working is a far worse option
than deviating from Linux.

>
> [...]
>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 2b44e5d..3dc5117 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -94,27 +94,13 @@ SECTIONS
>>     _erodata = .;                /* End of read-only data */
>>       .data : {                    /* Data */
>> +       *(.data.read_mostly)
>
> Before, .data.read_mostly was SMP_CACHE_BYTES aligned. Now, it seems
> there are no alignment.
>
> This may end up to have _erodata and .data.read_mostly to be part of
> the same page. As Arm enforce read-only, you may crash on access to
> .data.read_mostly.
>
> So I think you have
> . = ALIGN(PAGE_SIZE)
> *(.data.read_mostly)
> .align(SMP_CACHE_BYTES).

These both need to be PAGE_SIZE, or .data.page_aligned can end up having
problems.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/link: Fold .data.read_mostly into .data

[Julien Grall]

Hi Andrew,

On 6/19/19 10:48 PM, Andrew Cooper wrote:
> On 19/06/2019 22:28, Julien Grall wrote:
>> On 6/19/19 9:11 PM, Andrew Cooper wrote:
>>> .data.read_mostly only needs separating from adjacent data by one
>>> cache line
>>> to be effective, and placing it adjacent to .data.page_aligned
>>> fulfills this
>>> requirement.
>>>
>>> For ARM, .ex_table appears to be a vestigial remnant.  Nothing in the
>>> resulting build ever inspects or acts on the contents of the table.
>>> The arm32
>>> build does however have some assembly routines which fill .ex_table.
>>>
>>> Drop all of ARM's .ex_table infrastructure, to reduce the size of the
>>> compiled
>>> binary, and avoid giving the illusion of exception handling working.
>>
>> I am not in favor of this change. assembler.h is meant to be a
>> verbatim copy of the Linux counterpart.
> 
> What alternative do you propose then, because having infrastructure that
> gives the illusion of exception recovery working is a far worse option
> than deviating from Linux.

I learnt the hard way before that trying to adapt a Linux file to Xen 
makes very difficult to keep track what's going on.

So my preference here is to just disable the section if they exists.

>>
>> [...]
>>
>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>>> index 2b44e5d..3dc5117 100644
>>> --- a/xen/arch/arm/xen.lds.S
>>> +++ b/xen/arch/arm/xen.lds.S
>>> @@ -94,27 +94,13 @@ SECTIONS
>>>      _erodata = .;                /* End of read-only data */
>>>        .data : {                    /* Data */
>>> +       *(.data.read_mostly)
>>
>> Before, .data.read_mostly was SMP_CACHE_BYTES aligned. Now, it seems
>> there are no alignment.
>>
>> This may end up to have _erodata and .data.read_mostly to be part of
>> the same page. As Arm enforce read-only, you may crash on access to
>> .data.read_mostly.
>>
>> So I think you have
>> . = ALIGN(PAGE_SIZE)
>> *(.data.read_mostly)
>> .align(SMP_CACHE_BYTES).
> 
> These both need to be PAGE_SIZE, or .data.page_aligned can end up having
> problems.

Good point, I missed the .data.page_aligned.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/link: Fold .data.read_mostly into .data

[Jan Beulich]

>>> On 19.06.19 at 22:11, <andrew.cooper3@citrix.com> wrote:
> .data.read_mostly only needs separating from adjacent data by one cache line
> to be effective, and placing it adjacent to .data.page_aligned fulfills this
> requirement.
> 
> For ARM, .ex_table appears to be a vestigial remnant.  Nothing in the
> resulting build ever inspects or acts on the contents of the table.  The arm32
> build does however have some assembly routines which fill .ex_table.
> 
> Drop all of ARM's .ex_table infrastructure, to reduce the size of the compiled
> binary, and avoid giving the illusion of exception handling working.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Without you undoing part of 01fe4da624 ("x86: force suitable
alignment in sources rather than in linker script"), i.e. with the
ALIGN(PAGE_SIZE) dropped, x86 part
Acked-by: Jan Beulich <jbeulich@suse.com>

Yet FTR - I don't think there was anything wrong with it having its own
section in the final binary. Of course there's also nothing wrong with it
getting folded into .data, in particular with it sitting ahead of
.data.page_aligned anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/4] xen/link: Misc cleanup

[Andrew Cooper]

 * Drop .gnu.warning.  Xen, not being a library, has no need for
   __attribute__((__warning__("str"))) and isn't liable to ever gain such
   annotations for link time warnings.
 * Adjust the indentation of the start of ARM's .rodata section.
 * Discard .discard on ARM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/xen.lds.S | 7 ++++---
 xen/arch/x86/xen.lds.S | 1 -
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 3dc5117..c5ef5d5 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -35,14 +35,13 @@ SECTIONS
        *(.text.cold)
        *(.text.unlikely)
        *(.fixup)
-       *(.gnu.warning)
        _etext = .;             /* End of text section */
   } :text = 0x9090
 
   . = ALIGN(PAGE_SIZE);
   .rodata : {
-        _srodata = .;          /* Read-only data */
-        /* Bug frames table */
+       _srodata = .;           /* Read-only data */
+       /* Bug frames table */
        __start_bug_frames = .;
        *(.bug_frames.0)
        __stop_bug_frames_0 = .;
@@ -209,6 +208,8 @@ SECTIONS
        *(.exit.text)
        *(.exit.data)
        *(.exitcall.exit)
+       *(.discard)
+       *(.discard.*)
        *(.eh_frame)
   }
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index ef11949..8bc2be3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -82,7 +82,6 @@ SECTIONS
        *(.text.unlikely)
        *(.fixup)
        *(.text.kexec)
-       *(.gnu.warning)
        _etext = .;             /* End of text section */
   } :text = 0x9090
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/link: Misc cleanup

[Julien Grall]

Hi,

On 6/19/19 9:11 PM, Andrew Cooper wrote:
>   * Drop .gnu.warning.  Xen, not being a library, has no need for
>     __attribute__((__warning__("str"))) and isn't liable to ever gain such
>     annotations for link time warnings.

What if this is introduced? How do we catch it?

>   * Adjust the indentation of the start of ARM's .rodata section.
>   * Discard .discard on ARM.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/link: Misc cleanup

[Andrew Cooper]

On 19/06/2019 22:30, Julien Grall wrote:
> Hi,
>
> On 6/19/19 9:11 PM, Andrew Cooper wrote:
>>   * Drop .gnu.warning.  Xen, not being a library, has no need for
>>     __attribute__((__warning__("str"))) and isn't liable to ever gain
>> such
>>     annotations for link time warnings.
>
> What if this is introduced?

Then attempting to link Xen as a library against another object file
won't emit the custom linker warning.

Its main use is for phase-out of problematic API's, but for Xen (and
other standalone binaries) we do that by replacing problematic functions
entirely.

> How do we catch it?

Code review?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/link: Misc cleanup

[Julien Grall]

Hi Andrew,

On 6/19/19 10:38 PM, Andrew Cooper wrote:
> On 19/06/2019 22:30, Julien Grall wrote:
>> Hi,
>>
>> On 6/19/19 9:11 PM, Andrew Cooper wrote:
>>>    * Drop .gnu.warning.  Xen, not being a library, has no need for
>>>      __attribute__((__warning__("str"))) and isn't liable to ever gain
>>> such
>>>      annotations for link time warnings.
>>
>> What if this is introduced?
> 
> Then attempting to link Xen as a library against another object file
> won't emit the custom linker warning.

Ok, so it is not like Xen will crash.

> Its main use is for phase-out of problematic API's, but for Xen (and
> other standalone binaries) we do that by replacing problematic functions
> entirely.
>> How do we catch it?
> 
> Code review?

I usually quite like when the tools help us to catch such issue :).

Anyway, as this is not overly critical:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/link: Misc cleanup

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 09:11:27PM +0100, Andrew Cooper wrote:
>  * Drop .gnu.warning.  Xen, not being a library, has no need for
>    __attribute__((__warning__("str"))) and isn't liable to ever gain such
>    annotations for link time warnings.
>  * Adjust the indentation of the start of ARM's .rodata section.
>  * Discard .discard on ARM.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

For x86:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/link: Misc cleanup

[Jan Beulich]

 >>> On 19.06.19 at 22:11, <andrew.cooper3@citrix.com> wrote:
> * Drop .gnu.warning.  Xen, not being a library, has no need for
>    __attribute__((__warning__("str"))) and isn't liable to ever gain such
>    annotations for link time warnings.
>  * Adjust the indentation of the start of ARM's .rodata section.
>  * Discard .discard on ARM.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

x86:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel