If livepatching support is enabled build the hypervisor with
-f{function,data}-sections compiler options, which is required by the
livepatching tools to detect changes and create livepatches.

This shouldn't result in any functional change on the hypervisor
binary image, but does however require some changes in the linker
script in order to handle that each function and data item will now be
placed into its own section in object files. As a result add catch-all
for .text, .data and .bss in order to merge each individual item
section into the final image.

The main difference will be that .text.startup will end up being part
of .text rather than .init, and thus won't be freed. .text.exit will
also be part of .text rather than dropped. Overall this could make the
image bigger, and package some .text code in a sub-optimal way.

On Arm the .data.read_mostly needs to be moved ahead of the .data
section like it's already done on x86, so the .data.* catch-all
doesn't also include .data.read_mostly. The alignment of
.data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
up being placed at the tail of a read-only page from the previous
section. While there move the alignment of the .data section ahead of
the section declaration, like it's done for other sections.

The benefit of having CONFIG_LIVEPATCH enable those compiler option
is that the livepatch build tools no longer need to fiddle with the
build system in order to enable them. Note the current livepatch tools
are broken after the recent build changes due to the way they
attempt to set  -f{function,data}-sections.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Split the placing of the header code in a separate section to a
   pre-patch.
 - Move Kconfig option to xen/Kconfig.
 - Expand reasoning why .data.read_mostly needs to be moved on Arm.

Changes since v1:
 - Introduce CC_SPLIT_SECTIONS for selecting the compiler options.
 - Drop check for compiler options, all supported versions have them.
 - Re-arrange section placement in .text, to match the default linker
   script.
 - Introduce .text.header to contain the headers bits that must appear
   first in the final binary.
---
It seems on Arm the schedulers and hypfs .data sections should be
moved into read_mostly.
---
Tested by gitlab in order to assert I didn't introduce any regression
on Arm specially.
---
 xen/Kconfig            |  4 ++++
 xen/Makefile           |  2 ++
 xen/arch/arm/xen.lds.S | 41 +++++++++++++++++++++--------------------
 xen/arch/x86/xen.lds.S |  9 +++++----
 xen/common/Kconfig     |  1 +
 5 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index bcbd2758e5..d134397a0b 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -27,6 +27,10 @@ config CLANG_VERSION
 config CC_HAS_VISIBILITY_ATTRIBUTE
 	def_bool $(cc-option,-fvisibility=hidden)
 
+# Use -f{function,data}-sections compiler parameters
+config CC_SPLIT_SECTIONS
+	bool
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index 5c21492d6f..18a4f7e101 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -273,6 +273,8 @@ else
 CFLAGS += -fomit-frame-pointer
 endif
 
+CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
+
 CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
 $(call cc-option-add,CFLAGS,CC,-Wvla)
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 47d09d6cf1..836da880c3 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -36,6 +36,9 @@ SECTIONS
        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
 
        *(.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+       *(.text.*)
+#endif
 
        *(.fixup)
        *(.gnu.warning)
@@ -82,10 +85,24 @@ SECTIONS
 #endif
   _erodata = .;                /* End of read-only data */
 
+  . = ALIGN(PAGE_SIZE);
+  .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(SMP_CACHE_BYTES);
   .data : {                    /* Data */
-       . = ALIGN(PAGE_SIZE);
        *(.data.page_aligned)
-       *(.data)
        . = ALIGN(8);
        __start_schedulers_array = .;
        *(.data.schedulers)
@@ -98,26 +115,10 @@ SECTIONS
        __paramhypfs_end = .;
 #endif
 
-       *(.data.rel)
-       *(.data.rel.*)
+       *(.data .data.*)
        CONSTRUCTORS
   } :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 = .;
@@ -211,7 +212,7 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
-       *(.bss)
+       *(.bss .bss.*)
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 715452aad9..75925fe145 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -88,6 +88,9 @@ SECTIONS
        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
 
        *(.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+       *(.text.*)
+#endif
        *(.text.__x86_indirect_thunk_*)
        *(.text.page_aligned)
 
@@ -292,9 +295,7 @@ SECTIONS
 
   DECL_SECTION(.data) {
        *(.data.page_aligned)
-       *(.data)
-       *(.data.rel)
-       *(.data.rel.*)
+       *(.data .data.*)
   } PHDR(text)
 
   DECL_SECTION(.bss) {
@@ -309,7 +310,7 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
-       *(.bss)
+       *(.bss .bss.*)
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } PHDR(text)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6443943889..d921c74d61 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -354,6 +354,7 @@ config LIVEPATCH
 	bool "Live patching support"
 	default X86
 	depends on "$(XEN_HAS_BUILD_ID)" = "y"
+	select CC_SPLIT_SECTIONS
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
-- 
2.34.1

On 08.03.2022 14:49, Roger Pau Monne wrote:
> If livepatching support is enabled build the hypervisor with
> -f{function,data}-sections compiler options, which is required by the
> livepatching tools to detect changes and create livepatches.
> 
> This shouldn't result in any functional change on the hypervisor
> binary image, but does however require some changes in the linker
> script in order to handle that each function and data item will now be
> placed into its own section in object files. As a result add catch-all
> for .text, .data and .bss in order to merge each individual item
> section into the final image.
> 
> The main difference will be that .text.startup will end up being part
> of .text rather than .init, and thus won't be freed. .text.exit will
> also be part of .text rather than dropped. Overall this could make the
> image bigger, and package some .text code in a sub-optimal way.
> 
> On Arm the .data.read_mostly needs to be moved ahead of the .data
> section like it's already done on x86, so the .data.* catch-all
> doesn't also include .data.read_mostly. The alignment of
> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
> up being placed at the tail of a read-only page from the previous
> section. While there move the alignment of the .data section ahead of
> the section declaration, like it's done for other sections.
> 
> The benefit of having CONFIG_LIVEPATCH enable those compiler option
> is that the livepatch build tools no longer need to fiddle with the
> build system in order to enable them. Note the current livepatch tools
> are broken after the recent build changes due to the way they
> attempt to set  -f{function,data}-sections.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -88,6 +88,9 @@ SECTIONS
>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>  
>         *(.text)
> +#ifdef CONFIG_CC_SPLIT_SECTIONS
> +       *(.text.*)
> +#endif
>         *(.text.__x86_indirect_thunk_*)
>         *(.text.page_aligned)

These last two now will not have any effect anymore when
CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
overall size when there is more than one object with a
.text.page_aligned contribution. In .data ...

> @@ -292,9 +295,7 @@ SECTIONS
>  
>    DECL_SECTION(.data) {
>         *(.data.page_aligned)
> -       *(.data)
> -       *(.data.rel)
> -       *(.data.rel.*)
> +       *(.data .data.*)
>    } PHDR(text)

... this continues to be named first. I wonder whether we wouldn't
want to use SORT_BY_ALIGNMENT (if available) instead in both places.

Jan

On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
> On 08.03.2022 14:49, Roger Pau Monne wrote:
> > If livepatching support is enabled build the hypervisor with
> > -f{function,data}-sections compiler options, which is required by the
> > livepatching tools to detect changes and create livepatches.
> > 
> > This shouldn't result in any functional change on the hypervisor
> > binary image, but does however require some changes in the linker
> > script in order to handle that each function and data item will now be
> > placed into its own section in object files. As a result add catch-all
> > for .text, .data and .bss in order to merge each individual item
> > section into the final image.
> > 
> > The main difference will be that .text.startup will end up being part
> > of .text rather than .init, and thus won't be freed. .text.exit will
> > also be part of .text rather than dropped. Overall this could make the
> > image bigger, and package some .text code in a sub-optimal way.
> > 
> > On Arm the .data.read_mostly needs to be moved ahead of the .data
> > section like it's already done on x86, so the .data.* catch-all
> > doesn't also include .data.read_mostly. The alignment of
> > .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
> > up being placed at the tail of a read-only page from the previous
> > section. While there move the alignment of the .data section ahead of
> > the section declaration, like it's done for other sections.
> > 
> > The benefit of having CONFIG_LIVEPATCH enable those compiler option
> > is that the livepatch build tools no longer need to fiddle with the
> > build system in order to enable them. Note the current livepatch tools
> > are broken after the recent build changes due to the way they
> > attempt to set  -f{function,data}-sections.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -88,6 +88,9 @@ SECTIONS
> >         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >  
> >         *(.text)
> > +#ifdef CONFIG_CC_SPLIT_SECTIONS
> > +       *(.text.*)
> > +#endif
> >         *(.text.__x86_indirect_thunk_*)
> >         *(.text.page_aligned)
> 
> These last two now will not have any effect anymore when
> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
> overall size when there is more than one object with a
> .text.page_aligned contribution. In .data ...

Agreed. I wondered whether to move those ahead of the main text
section, so likely:

       *(.text.unlikely .text.*_unlikely .text.unlikely.*)

       *(.text.page_aligned)
       *(.text.__x86_indirect_thunk_*)
       *(.text)
#ifdef CONFIG_CC_SPLIT_SECTIONS
       *(.text.*)
#endif

FWIW, Linux seems fine to package .text.page_aligned together with the
rest of .text using the .text.[0-9a-zA-Z_]* catch-all.

> > @@ -292,9 +295,7 @@ SECTIONS
> >  
> >    DECL_SECTION(.data) {
> >         *(.data.page_aligned)
> > -       *(.data)
> > -       *(.data.rel)
> > -       *(.data.rel.*)
> > +       *(.data .data.*)
> >    } PHDR(text)
> 
> ... this continues to be named first. I wonder whether we wouldn't
> want to use SORT_BY_ALIGNMENT (if available) instead in both places.

We could use the command line option if available
(--sort-section=alignment) to sort all wildcard sections?

Thanks, Roger.

On 08.03.2022 15:46, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
>> On 08.03.2022 14:49, Roger Pau Monne wrote:
>>> If livepatching support is enabled build the hypervisor with
>>> -f{function,data}-sections compiler options, which is required by the
>>> livepatching tools to detect changes and create livepatches.
>>>
>>> This shouldn't result in any functional change on the hypervisor
>>> binary image, but does however require some changes in the linker
>>> script in order to handle that each function and data item will now be
>>> placed into its own section in object files. As a result add catch-all
>>> for .text, .data and .bss in order to merge each individual item
>>> section into the final image.
>>>
>>> The main difference will be that .text.startup will end up being part
>>> of .text rather than .init, and thus won't be freed. .text.exit will
>>> also be part of .text rather than dropped. Overall this could make the
>>> image bigger, and package some .text code in a sub-optimal way.
>>>
>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
>>> section like it's already done on x86, so the .data.* catch-all
>>> doesn't also include .data.read_mostly. The alignment of
>>> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
>>> up being placed at the tail of a read-only page from the previous
>>> section. While there move the alignment of the .data section ahead of
>>> the section declaration, like it's done for other sections.
>>>
>>> The benefit of having CONFIG_LIVEPATCH enable those compiler option
>>> is that the livepatch build tools no longer need to fiddle with the
>>> build system in order to enable them. Note the current livepatch tools
>>> are broken after the recent build changes due to the way they
>>> attempt to set  -f{function,data}-sections.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -88,6 +88,9 @@ SECTIONS
>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>  
>>>         *(.text)
>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
>>> +       *(.text.*)
>>> +#endif
>>>         *(.text.__x86_indirect_thunk_*)
>>>         *(.text.page_aligned)
>>
>> These last two now will not have any effect anymore when
>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
>> overall size when there is more than one object with a
>> .text.page_aligned contribution. In .data ...
> 
> Agreed. I wondered whether to move those ahead of the main text
> section, so likely:
> 
>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> 
>        *(.text.page_aligned)
>        *(.text.__x86_indirect_thunk_*)
>        *(.text)
> #ifdef CONFIG_CC_SPLIT_SECTIONS
>        *(.text.*)
> #endif

Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
though. When adding .text.* that one can likely go away.

> FWIW, Linux seems fine to package .text.page_aligned together with the
> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.

There's no question this is functionally fine. The question is how
many extra padding areas are inserted because of this.

>>> @@ -292,9 +295,7 @@ SECTIONS
>>>  
>>>    DECL_SECTION(.data) {
>>>         *(.data.page_aligned)
>>> -       *(.data)
>>> -       *(.data.rel)
>>> -       *(.data.rel.*)
>>> +       *(.data .data.*)
>>>    } PHDR(text)
>>
>> ... this continues to be named first. I wonder whether we wouldn't
>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
> 
> We could use the command line option if available
> (--sort-section=alignment) to sort all wildcard sections?

Depends on the scope of the sorting that would result when enabled
globally like this.

Jan

On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
> On 08.03.2022 15:46, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
> >> On 08.03.2022 14:49, Roger Pau Monne wrote:
> >>> If livepatching support is enabled build the hypervisor with
> >>> -f{function,data}-sections compiler options, which is required by the
> >>> livepatching tools to detect changes and create livepatches.
> >>>
> >>> This shouldn't result in any functional change on the hypervisor
> >>> binary image, but does however require some changes in the linker
> >>> script in order to handle that each function and data item will now be
> >>> placed into its own section in object files. As a result add catch-all
> >>> for .text, .data and .bss in order to merge each individual item
> >>> section into the final image.
> >>>
> >>> The main difference will be that .text.startup will end up being part
> >>> of .text rather than .init, and thus won't be freed. .text.exit will
> >>> also be part of .text rather than dropped. Overall this could make the
> >>> image bigger, and package some .text code in a sub-optimal way.
> >>>
> >>> On Arm the .data.read_mostly needs to be moved ahead of the .data
> >>> section like it's already done on x86, so the .data.* catch-all
> >>> doesn't also include .data.read_mostly. The alignment of
> >>> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
> >>> up being placed at the tail of a read-only page from the previous
> >>> section. While there move the alignment of the .data section ahead of
> >>> the section declaration, like it's done for other sections.
> >>>
> >>> The benefit of having CONFIG_LIVEPATCH enable those compiler option
> >>> is that the livepatch build tools no longer need to fiddle with the
> >>> build system in order to enable them. Note the current livepatch tools
> >>> are broken after the recent build changes due to the way they
> >>> attempt to set  -f{function,data}-sections.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >>> --- a/xen/arch/x86/xen.lds.S
> >>> +++ b/xen/arch/x86/xen.lds.S
> >>> @@ -88,6 +88,9 @@ SECTIONS
> >>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >>>  
> >>>         *(.text)
> >>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
> >>> +       *(.text.*)
> >>> +#endif
> >>>         *(.text.__x86_indirect_thunk_*)
> >>>         *(.text.page_aligned)
> >>
> >> These last two now will not have any effect anymore when
> >> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
> >> overall size when there is more than one object with a
> >> .text.page_aligned contribution. In .data ...
> > 
> > Agreed. I wondered whether to move those ahead of the main text
> > section, so likely:
> > 
> >        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> > 
> >        *(.text.page_aligned)
> >        *(.text.__x86_indirect_thunk_*)
> >        *(.text)
> > #ifdef CONFIG_CC_SPLIT_SECTIONS
> >        *(.text.*)
> > #endif
> 
> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
> though. When adding .text.* that one can likely go away.
> 
> > FWIW, Linux seems fine to package .text.page_aligned together with the
> > rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
> 
> There's no question this is functionally fine. The question is how
> many extra padding areas are inserted because of this.
> 
> >>> @@ -292,9 +295,7 @@ SECTIONS
> >>>  
> >>>    DECL_SECTION(.data) {
> >>>         *(.data.page_aligned)
> >>> -       *(.data)
> >>> -       *(.data.rel)
> >>> -       *(.data.rel.*)
> >>> +       *(.data .data.*)
> >>>    } PHDR(text)
> >>
> >> ... this continues to be named first. I wonder whether we wouldn't
> >> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
> > 
> > We could use the command line option if available
> > (--sort-section=alignment) to sort all wildcard sections?
> 
> Depends on the scope of the sorting that would result when enabled
> globally like this.

I'm not sure I'm following. Don't we generally want to sort by
alignment in order to avoid adding unnecessary padding as much as
possible?

For any wildcard sections we really don't care anymore how they are
sorted?

Thanks, Roger.

On 08.03.2022 17:41, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
>> On 08.03.2022 15:46, Roger Pau Monné wrote:
>>> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
>>>> On 08.03.2022 14:49, Roger Pau Monne wrote:
>>>>> If livepatching support is enabled build the hypervisor with
>>>>> -f{function,data}-sections compiler options, which is required by the
>>>>> livepatching tools to detect changes and create livepatches.
>>>>>
>>>>> This shouldn't result in any functional change on the hypervisor
>>>>> binary image, but does however require some changes in the linker
>>>>> script in order to handle that each function and data item will now be
>>>>> placed into its own section in object files. As a result add catch-all
>>>>> for .text, .data and .bss in order to merge each individual item
>>>>> section into the final image.
>>>>>
>>>>> The main difference will be that .text.startup will end up being part
>>>>> of .text rather than .init, and thus won't be freed. .text.exit will
>>>>> also be part of .text rather than dropped. Overall this could make the
>>>>> image bigger, and package some .text code in a sub-optimal way.
>>>>>
>>>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
>>>>> section like it's already done on x86, so the .data.* catch-all
>>>>> doesn't also include .data.read_mostly. The alignment of
>>>>> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
>>>>> up being placed at the tail of a read-only page from the previous
>>>>> section. While there move the alignment of the .data section ahead of
>>>>> the section declaration, like it's done for other sections.
>>>>>
>>>>> The benefit of having CONFIG_LIVEPATCH enable those compiler option
>>>>> is that the livepatch build tools no longer need to fiddle with the
>>>>> build system in order to enable them. Note the current livepatch tools
>>>>> are broken after the recent build changes due to the way they
>>>>> attempt to set  -f{function,data}-sections.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -88,6 +88,9 @@ SECTIONS
>>>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>  
>>>>>         *(.text)
>>>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>> +       *(.text.*)
>>>>> +#endif
>>>>>         *(.text.__x86_indirect_thunk_*)
>>>>>         *(.text.page_aligned)
>>>>
>>>> These last two now will not have any effect anymore when
>>>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
>>>> overall size when there is more than one object with a
>>>> .text.page_aligned contribution. In .data ...
>>>
>>> Agreed. I wondered whether to move those ahead of the main text
>>> section, so likely:
>>>
>>>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>
>>>        *(.text.page_aligned)
>>>        *(.text.__x86_indirect_thunk_*)
>>>        *(.text)
>>> #ifdef CONFIG_CC_SPLIT_SECTIONS
>>>        *(.text.*)
>>> #endif
>>
>> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
>> though. When adding .text.* that one can likely go away.
>>
>>> FWIW, Linux seems fine to package .text.page_aligned together with the
>>> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
>>
>> There's no question this is functionally fine. The question is how
>> many extra padding areas are inserted because of this.
>>
>>>>> @@ -292,9 +295,7 @@ SECTIONS
>>>>>  
>>>>>    DECL_SECTION(.data) {
>>>>>         *(.data.page_aligned)
>>>>> -       *(.data)
>>>>> -       *(.data.rel)
>>>>> -       *(.data.rel.*)
>>>>> +       *(.data .data.*)
>>>>>    } PHDR(text)
>>>>
>>>> ... this continues to be named first. I wonder whether we wouldn't
>>>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
>>>
>>> We could use the command line option if available
>>> (--sort-section=alignment) to sort all wildcard sections?
>>
>> Depends on the scope of the sorting that would result when enabled
>> globally like this.
> 
> I'm not sure I'm following. Don't we generally want to sort by
> alignment in order to avoid adding unnecessary padding as much as
> possible?
> 
> For any wildcard sections we really don't care anymore how they are
> sorted?

Sure. Question is whether sorting is limited to within any single
*(...) construct, or whether it could extend to adjacent ones. IOW
whether the command line option strictly is a replacement of adding
SORT_BY_ALIGNMENT to every one of these constructs.

Jan

On Tue, Mar 08, 2022 at 05:58:49PM +0100, Jan Beulich wrote:
> On 08.03.2022 17:41, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
> >> On 08.03.2022 15:46, Roger Pau Monné wrote:
> >>> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
> >>>> On 08.03.2022 14:49, Roger Pau Monne wrote:
> >>>>> If livepatching support is enabled build the hypervisor with
> >>>>> -f{function,data}-sections compiler options, which is required by the
> >>>>> livepatching tools to detect changes and create livepatches.
> >>>>>
> >>>>> This shouldn't result in any functional change on the hypervisor
> >>>>> binary image, but does however require some changes in the linker
> >>>>> script in order to handle that each function and data item will now be
> >>>>> placed into its own section in object files. As a result add catch-all
> >>>>> for .text, .data and .bss in order to merge each individual item
> >>>>> section into the final image.
> >>>>>
> >>>>> The main difference will be that .text.startup will end up being part
> >>>>> of .text rather than .init, and thus won't be freed. .text.exit will
> >>>>> also be part of .text rather than dropped. Overall this could make the
> >>>>> image bigger, and package some .text code in a sub-optimal way.
> >>>>>
> >>>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
> >>>>> section like it's already done on x86, so the .data.* catch-all
> >>>>> doesn't also include .data.read_mostly. The alignment of
> >>>>> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
> >>>>> up being placed at the tail of a read-only page from the previous
> >>>>> section. While there move the alignment of the .data section ahead of
> >>>>> the section declaration, like it's done for other sections.
> >>>>>
> >>>>> The benefit of having CONFIG_LIVEPATCH enable those compiler option
> >>>>> is that the livepatch build tools no longer need to fiddle with the
> >>>>> build system in order to enable them. Note the current livepatch tools
> >>>>> are broken after the recent build changes due to the way they
> >>>>> attempt to set  -f{function,data}-sections.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>>
> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>>> --- a/xen/arch/x86/xen.lds.S
> >>>>> +++ b/xen/arch/x86/xen.lds.S
> >>>>> @@ -88,6 +88,9 @@ SECTIONS
> >>>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >>>>>  
> >>>>>         *(.text)
> >>>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
> >>>>> +       *(.text.*)
> >>>>> +#endif
> >>>>>         *(.text.__x86_indirect_thunk_*)
> >>>>>         *(.text.page_aligned)
> >>>>
> >>>> These last two now will not have any effect anymore when
> >>>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
> >>>> overall size when there is more than one object with a
> >>>> .text.page_aligned contribution. In .data ...
> >>>
> >>> Agreed. I wondered whether to move those ahead of the main text
> >>> section, so likely:
> >>>
> >>>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >>>
> >>>        *(.text.page_aligned)
> >>>        *(.text.__x86_indirect_thunk_*)
> >>>        *(.text)
> >>> #ifdef CONFIG_CC_SPLIT_SECTIONS
> >>>        *(.text.*)
> >>> #endif
> >>
> >> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
> >> though. When adding .text.* that one can likely go away.
> >>
> >>> FWIW, Linux seems fine to package .text.page_aligned together with the
> >>> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
> >>
> >> There's no question this is functionally fine. The question is how
> >> many extra padding areas are inserted because of this.
> >>
> >>>>> @@ -292,9 +295,7 @@ SECTIONS
> >>>>>  
> >>>>>    DECL_SECTION(.data) {
> >>>>>         *(.data.page_aligned)
> >>>>> -       *(.data)
> >>>>> -       *(.data.rel)
> >>>>> -       *(.data.rel.*)
> >>>>> +       *(.data .data.*)
> >>>>>    } PHDR(text)
> >>>>
> >>>> ... this continues to be named first. I wonder whether we wouldn't
> >>>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
> >>>
> >>> We could use the command line option if available
> >>> (--sort-section=alignment) to sort all wildcard sections?
> >>
> >> Depends on the scope of the sorting that would result when enabled
> >> globally like this.
> > 
> > I'm not sure I'm following. Don't we generally want to sort by
> > alignment in order to avoid adding unnecessary padding as much as
> > possible?
> > 
> > For any wildcard sections we really don't care anymore how they are
> > sorted?
> 
> Sure. Question is whether sorting is limited to within any single
> *(...) construct, or whether it could extend to adjacent ones. IOW
> whether the command line option strictly is a replacement of adding
> SORT_BY_ALIGNMENT to every one of these constructs.

AFAICT the command line option will have the effect of setting the
sorting of any wildcard containing sections to use SORT_BY_ALIGNMENT.
Ie: .data.* would become SORT_BY_ALIGNMENT(.data.*):

*(.data SORT_BY_ALIGNMENT(.data.*))

I've taken a look at the binutils ld source and that seems to be the
case, any wildcard containing enum will get it's sorting set to by
alignment (but I'm not familiar with ld code so I might be missing
pieces).

Thanks, Roger.

On 09.03.2022 10:30, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 05:58:49PM +0100, Jan Beulich wrote:
>> On 08.03.2022 17:41, Roger Pau Monné wrote:
>>> On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
>>>> On 08.03.2022 15:46, Roger Pau Monné wrote:
>>>>> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
>>>>>> On 08.03.2022 14:49, Roger Pau Monne wrote:
>>>>>>> If livepatching support is enabled build the hypervisor with
>>>>>>> -f{function,data}-sections compiler options, which is required by the
>>>>>>> livepatching tools to detect changes and create livepatches.
>>>>>>>
>>>>>>> This shouldn't result in any functional change on the hypervisor
>>>>>>> binary image, but does however require some changes in the linker
>>>>>>> script in order to handle that each function and data item will now be
>>>>>>> placed into its own section in object files. As a result add catch-all
>>>>>>> for .text, .data and .bss in order to merge each individual item
>>>>>>> section into the final image.
>>>>>>>
>>>>>>> The main difference will be that .text.startup will end up being part
>>>>>>> of .text rather than .init, and thus won't be freed. .text.exit will
>>>>>>> also be part of .text rather than dropped. Overall this could make the
>>>>>>> image bigger, and package some .text code in a sub-optimal way.
>>>>>>>
>>>>>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
>>>>>>> section like it's already done on x86, so the .data.* catch-all
>>>>>>> doesn't also include .data.read_mostly. The alignment of
>>>>>>> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
>>>>>>> up being placed at the tail of a read-only page from the previous
>>>>>>> section. While there move the alignment of the .data section ahead of
>>>>>>> the section declaration, like it's done for other sections.
>>>>>>>
>>>>>>> The benefit of having CONFIG_LIVEPATCH enable those compiler option
>>>>>>> is that the livepatch build tools no longer need to fiddle with the
>>>>>>> build system in order to enable them. Note the current livepatch tools
>>>>>>> are broken after the recent build changes due to the way they
>>>>>>> attempt to set  -f{function,data}-sections.
>>>>>>>
>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>>
>>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>> @@ -88,6 +88,9 @@ SECTIONS
>>>>>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>>>  
>>>>>>>         *(.text)
>>>>>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>>>> +       *(.text.*)
>>>>>>> +#endif
>>>>>>>         *(.text.__x86_indirect_thunk_*)
>>>>>>>         *(.text.page_aligned)
>>>>>>
>>>>>> These last two now will not have any effect anymore when
>>>>>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
>>>>>> overall size when there is more than one object with a
>>>>>> .text.page_aligned contribution. In .data ...
>>>>>
>>>>> Agreed. I wondered whether to move those ahead of the main text
>>>>> section, so likely:
>>>>>
>>>>>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>
>>>>>        *(.text.page_aligned)
>>>>>        *(.text.__x86_indirect_thunk_*)
>>>>>        *(.text)
>>>>> #ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>>        *(.text.*)
>>>>> #endif
>>>>
>>>> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
>>>> though. When adding .text.* that one can likely go away.
>>>>
>>>>> FWIW, Linux seems fine to package .text.page_aligned together with the
>>>>> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
>>>>
>>>> There's no question this is functionally fine. The question is how
>>>> many extra padding areas are inserted because of this.
>>>>
>>>>>>> @@ -292,9 +295,7 @@ SECTIONS
>>>>>>>  
>>>>>>>    DECL_SECTION(.data) {
>>>>>>>         *(.data.page_aligned)
>>>>>>> -       *(.data)
>>>>>>> -       *(.data.rel)
>>>>>>> -       *(.data.rel.*)
>>>>>>> +       *(.data .data.*)
>>>>>>>    } PHDR(text)
>>>>>>
>>>>>> ... this continues to be named first. I wonder whether we wouldn't
>>>>>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
>>>>>
>>>>> We could use the command line option if available
>>>>> (--sort-section=alignment) to sort all wildcard sections?
>>>>
>>>> Depends on the scope of the sorting that would result when enabled
>>>> globally like this.
>>>
>>> I'm not sure I'm following. Don't we generally want to sort by
>>> alignment in order to avoid adding unnecessary padding as much as
>>> possible?
>>>
>>> For any wildcard sections we really don't care anymore how they are
>>> sorted?
>>
>> Sure. Question is whether sorting is limited to within any single
>> *(...) construct, or whether it could extend to adjacent ones. IOW
>> whether the command line option strictly is a replacement of adding
>> SORT_BY_ALIGNMENT to every one of these constructs.
> 
> AFAICT the command line option will have the effect of setting the
> sorting of any wildcard containing sections to use SORT_BY_ALIGNMENT.
> Ie: .data.* would become SORT_BY_ALIGNMENT(.data.*):
> 
> *(.data SORT_BY_ALIGNMENT(.data.*))
> 
> I've taken a look at the binutils ld source and that seems to be the
> case, any wildcard containing enum will get it's sorting set to by
> alignment (but I'm not familiar with ld code so I might be missing
> pieces).

Okay - why don't we try that then (in a separate patch, so it's going
to be easy to revert)? For the patch here all I'd like to ask for is
to keep .text.page_aligned enumerated explicitly (and the wildcard
placed after it, obviously).

Jan

On 09/03/2022 10:22, Jan Beulich wrote:
> On 09.03.2022 10:30, Roger Pau Monné wrote:
>> On Tue, Mar 08, 2022 at 05:58:49PM +0100, Jan Beulich wrote:
>>> On 08.03.2022 17:41, Roger Pau Monné wrote:
>>>> On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
>>>>> On 08.03.2022 15:46, Roger Pau Monné wrote:
>>>>>> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
>>>>>>> On 08.03.2022 14:49, Roger Pau Monne wrote:
>>>>>>>> If livepatching support is enabled build the hypervisor with
>>>>>>>> -f{function,data}-sections compiler options, which is required by the
>>>>>>>> livepatching tools to detect changes and create livepatches.
>>>>>>>>
>>>>>>>> This shouldn't result in any functional change on the hypervisor
>>>>>>>> binary image, but does however require some changes in the linker
>>>>>>>> script in order to handle that each function and data item will now be
>>>>>>>> placed into its own section in object files. As a result add catch-all
>>>>>>>> for .text, .data and .bss in order to merge each individual item
>>>>>>>> section into the final image.
>>>>>>>>
>>>>>>>> The main difference will be that .text.startup will end up being part
>>>>>>>> of .text rather than .init, and thus won't be freed. .text.exit will
>>>>>>>> also be part of .text rather than dropped. Overall this could make the
>>>>>>>> image bigger, and package some .text code in a sub-optimal way.
>>>>>>>>
>>>>>>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
>>>>>>>> section like it's already done on x86, so the .data.* catch-all
>>>>>>>> doesn't also include .data.read_mostly. The alignment of
>>>>>>>> .data.read_mostly also needs to be set to PAGE_SIZE so it doesn't end
>>>>>>>> up being placed at the tail of a read-only page from the previous
>>>>>>>> section. While there move the alignment of the .data section ahead of
>>>>>>>> the section declaration, like it's done for other sections.
>>>>>>>>
>>>>>>>> The benefit of having CONFIG_LIVEPATCH enable those compiler option
>>>>>>>> is that the livepatch build tools no longer need to fiddle with the
>>>>>>>> build system in order to enable them. Note the current livepatch tools
>>>>>>>> are broken after the recent build changes due to the way they
>>>>>>>> attempt to set  -f{function,data}-sections.
>>>>>>>>
>>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>
>>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>>> @@ -88,6 +88,9 @@ SECTIONS
>>>>>>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>>>>  
>>>>>>>>         *(.text)
>>>>>>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>>>>> +       *(.text.*)
>>>>>>>> +#endif
>>>>>>>>         *(.text.__x86_indirect_thunk_*)
>>>>>>>>         *(.text.page_aligned)
>>>>>>> These last two now will not have any effect anymore when
>>>>>>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
>>>>>>> overall size when there is more than one object with a
>>>>>>> .text.page_aligned contribution. In .data ...
>>>>>> Agreed. I wondered whether to move those ahead of the main text
>>>>>> section, so likely:
>>>>>>
>>>>>>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>>
>>>>>>        *(.text.page_aligned)
>>>>>>        *(.text.__x86_indirect_thunk_*)
>>>>>>        *(.text)
>>>>>> #ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>>>        *(.text.*)
>>>>>> #endif
>>>>> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
>>>>> though. When adding .text.* that one can likely go away.
>>>>>
>>>>>> FWIW, Linux seems fine to package .text.page_aligned together with the
>>>>>> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
>>>>> There's no question this is functionally fine. The question is how
>>>>> many extra padding areas are inserted because of this.
>>>>>
>>>>>>>> @@ -292,9 +295,7 @@ SECTIONS
>>>>>>>>  
>>>>>>>>    DECL_SECTION(.data) {
>>>>>>>>         *(.data.page_aligned)
>>>>>>>> -       *(.data)
>>>>>>>> -       *(.data.rel)
>>>>>>>> -       *(.data.rel.*)
>>>>>>>> +       *(.data .data.*)
>>>>>>>>    } PHDR(text)
>>>>>>> ... this continues to be named first. I wonder whether we wouldn't
>>>>>>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
>>>>>> We could use the command line option if available
>>>>>> (--sort-section=alignment) to sort all wildcard sections?
>>>>> Depends on the scope of the sorting that would result when enabled
>>>>> globally like this.
>>>> I'm not sure I'm following. Don't we generally want to sort by
>>>> alignment in order to avoid adding unnecessary padding as much as
>>>> possible?
>>>>
>>>> For any wildcard sections we really don't care anymore how they are
>>>> sorted?
>>> Sure. Question is whether sorting is limited to within any single
>>> *(...) construct, or whether it could extend to adjacent ones. IOW
>>> whether the command line option strictly is a replacement of adding
>>> SORT_BY_ALIGNMENT to every one of these constructs.
>> AFAICT the command line option will have the effect of setting the
>> sorting of any wildcard containing sections to use SORT_BY_ALIGNMENT.
>> Ie: .data.* would become SORT_BY_ALIGNMENT(.data.*):
>>
>> *(.data SORT_BY_ALIGNMENT(.data.*))
>>
>> I've taken a look at the binutils ld source and that seems to be the
>> case, any wildcard containing enum will get it's sorting set to by
>> alignment (but I'm not familiar with ld code so I might be missing
>> pieces).
> Okay - why don't we try that then (in a separate patch, so it's going
> to be easy to revert)? For the patch here all I'd like to ask for is
> to keep .text.page_aligned enumerated explicitly (and the wildcard
> placed after it, obviously).

I'm not convinced this will be an improvement.  It will make a marginal
space saving, but cost runtime performance by breaking
locality-of-reference inside a TU.

What would make an improvement (if this isn't how it already works) is
having each TU sort by alignment on its own, then link in order.

~Andrew