diff mbox series

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

Message ID 1560975087-25632-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series xen/link: Fixes and improvements to Xen's linking | expand

Commit Message

Andrew Cooper June 19, 2019, 8:11 p.m. UTC
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(-)

Comments

Julien Grall June 19, 2019, 9:21 p.m. UTC | #1
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,
Roger Pau Monné June 20, 2019, 8:40 a.m. UTC | #2
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.
Andrew Cooper June 20, 2019, 12:34 p.m. UTC | #3
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
Jan Beulich June 21, 2019, 9:14 a.m. UTC | #4
>>> 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
diff mbox series

Patch

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) {