[PATCH v3 0/2] livepatch: enable -f{function,data}-sections compiler option

[PATCH v3 0/2] livepatch: enable -f{function,data}-sections compiler option

[Roger Pau Monne]

Hello,

The content in v3 has been split in two patches, but is still mostly the
same. The main difference is that first patch does a bit of cleanup of
the build logic now that the header object file doesn't need to be the
first one passed to the linker script.

Thanks, Roger.

Roger Pau Monne (2):
  xen/build: put image header into a separate section
  livepatch: set -f{function,data}-sections compiler option

 xen/Kconfig                 |  4 +++
 xen/Makefile                |  2 ++
 xen/arch/arm/arch.mk        |  2 --
 xen/arch/arm/arm32/Makefile |  3 +--
 xen/arch/arm/arm32/head.S   |  1 +
 xen/arch/arm/arm64/Makefile |  3 +--
 xen/arch/arm/arm64/head.S   |  1 +
 xen/arch/arm/xen.lds.S      | 49 ++++++++++++++++++++-----------------
 xen/arch/x86/Makefile       |  5 ++--
 xen/arch/x86/arch.mk        |  2 --
 xen/arch/x86/boot/head.S    |  2 +-
 xen/arch/x86/xen.lds.S      | 22 ++++++++++-------
 xen/common/Kconfig          |  1 +
 13 files changed, 54 insertions(+), 43 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] xen/build: put image header into a separate section

[Roger Pau Monne]

So it can be explicitly placed ahead of the rest of the .text content
in the linker script (and thus the resulting image). This is a
prerequisite for further work that will add a catch-all to the text
section (.text.*).

Note that placement of the sections inside of .text is also slightly
adjusted to be more similar to the position found in the default GNU
ld linker script.

The special handling of the object file containing the header data as
the first object file passed to the linker command line can also be
removed.

While there also remove the special handling of efi/ on x86. There's
no need for the resulting object file to be passed in any special
order to the linker.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version (split from patch 2).
---
 xen/arch/arm/arch.mk        |  2 --
 xen/arch/arm/arm32/Makefile |  3 +--
 xen/arch/arm/arm32/head.S   |  1 +
 xen/arch/arm/arm64/Makefile |  3 +--
 xen/arch/arm/arm64/head.S   |  1 +
 xen/arch/arm/xen.lds.S      |  8 ++++++--
 xen/arch/x86/Makefile       |  5 ++---
 xen/arch/x86/arch.mk        |  2 --
 xen/arch/x86/boot/head.S    |  2 +-
 xen/arch/x86/xen.lds.S      | 13 ++++++++-----
 10 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/arch.mk b/xen/arch/arm/arch.mk
index 094b670723..58db76c4e1 100644
--- a/xen/arch/arm/arch.mk
+++ b/xen/arch/arm/arch.mk
@@ -23,5 +23,3 @@ ifeq ($(CONFIG_ARM64_ERRATUM_843419),y)
         LDFLAGS += --fix-cortex-a53-843419
     endif
 endif
-
-ALL_OBJS-y := arch/arm/$(TARGET_SUBARCH)/head.o $(ALL_OBJS-y)
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index 3040eabce3..520fb42054 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -4,11 +4,10 @@ obj-$(CONFIG_EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
+obj-y += head.o
 obj-y += insn.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += proc-v7.o proc-caxx.o
 obj-y += smpboot.o
 obj-y += traps.o
 obj-y += vfp.o
-
-extra-y += head.o
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 7a906167ef..c837d3054c 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -120,6 +120,7 @@
 
 #endif /* !CONFIG_EARLY_PRINTK */
 
+        .section .text.header, "ax", %progbits
         .arm
 
         /*
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index baa87655fa..6d507da0d4 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
+obj-y += head.o
 obj-y += insn.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += smc.o
@@ -14,5 +15,3 @@ obj-y += smpboot.o
 obj-y += traps.o
 obj-y += vfp.o
 obj-y += vsysreg.o
-
-extra-y += head.o
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 66d862fc81..e62c48ec1c 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -133,6 +133,7 @@
         add \xb, \xb, x20
 .endm
 
+        .section .text.header, "ax", %progbits
         /*.aarch64*/
 
         /*
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 08016948ab..47d09d6cf1 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -30,9 +30,13 @@ SECTIONS
   _start = .;
   .text : {
         _stext = .;            /* Text section */
+       *(.text.header)
+
+       *(.text.cold .text.cold.*)
+       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
+
        *(.text)
-       *(.text.cold)
-       *(.text.unlikely)
+
        *(.fixup)
        *(.gnu.warning)
        _etext = .;             /* End of text section */
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 9c40e0b4d7..04065a7310 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -1,5 +1,7 @@
 obj-y += acpi/
+obj-y += boot/
 obj-y += cpu/
+obj-y += efi/
 obj-y += genapic/
 obj-$(CONFIG_GUEST) += guest/
 obj-$(CONFIG_HVM) += hvm/
@@ -77,9 +79,6 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
 obj-y += sysctl.o
 endif
 
-# Allows "clean" to descend into boot/
-subdir- += boot
-
 extra-y += asm-macros.i
 extra-y += xen.lds
 
diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk
index 8e57476d65..c90e56aeab 100644
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -117,5 +117,3 @@ endif
 
 # Set up the assembler include path properly for older toolchains.
 CFLAGS += -Wa,-I$(srctree)/include
-
-ALL_OBJS-y := arch/x86/boot/built_in.o arch/x86/efi/built_in.o $(ALL_OBJS-y)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index dd1bea0d10..92d73345f0 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -9,7 +9,7 @@
 #include <asm/cpufeature.h>
 #include <public/elfnote.h>
 
-        .text
+        .section .text.header, "ax", @progbits
         .code32
 
 #define sym_offs(sym)     ((sym) - __XEN_VIRT_START)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 506bc8e404..715452aad9 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -73,9 +73,7 @@ SECTIONS
   _start = .;
   DECL_SECTION(.text) {
         _stext = .;            /* Text and read-only data */
-       *(.text)
-       *(.text.__x86_indirect_thunk_*)
-       *(.text.page_aligned)
+       *(.text.header)
 
        . = ALIGN(PAGE_SIZE);
        _stextentry = .;
@@ -86,8 +84,13 @@ SECTIONS
        *(.text.kexec)          /* Page aligned in the object file. */
        kexec_reloc_end = .;
 
-       *(.text.cold)
-       *(.text.unlikely)
+       *(.text.cold .text.cold.*)
+       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
+
+       *(.text)
+       *(.text.__x86_indirect_thunk_*)
+       *(.text.page_aligned)
+
        *(.fixup)
        *(.gnu.warning)
        _etext = .;             /* End of text section */
-- 
2.34.1

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Jan Beulich]

On 08.03.2022 14:49, Roger Pau Monne wrote:
> So it can be explicitly placed ahead of the rest of the .text content
> in the linker script (and thus the resulting image). This is a
> prerequisite for further work that will add a catch-all to the text
> section (.text.*).
> 
> Note that placement of the sections inside of .text is also slightly
> adjusted to be more similar to the position found in the default GNU
> ld linker script.
> 
> The special handling of the object file containing the header data as
> the first object file passed to the linker command line can also be
> removed.
> 
> While there also remove the special handling of efi/ on x86. There's
> no need for the resulting object file to be passed in any special
> order to the linker.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Looks good to me, but I have one question before feeling ready to
offer R-b:

> @@ -86,8 +84,13 @@ SECTIONS
>         *(.text.kexec)          /* Page aligned in the object file. */
>         kexec_reloc_end = .;
>  
> -       *(.text.cold)
> -       *(.text.unlikely)
> +       *(.text.cold .text.cold.*)
> +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)

What generates .text.*_unlikely? And if anything really does, why
would .text.cold not have a similar equivalent?

Jan

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Roger Pau Monné]

On Tue, Mar 08, 2022 at 02:57:23PM +0100, Jan Beulich wrote:
> On 08.03.2022 14:49, Roger Pau Monne wrote:
> > So it can be explicitly placed ahead of the rest of the .text content
> > in the linker script (and thus the resulting image). This is a
> > prerequisite for further work that will add a catch-all to the text
> > section (.text.*).
> > 
> > Note that placement of the sections inside of .text is also slightly
> > adjusted to be more similar to the position found in the default GNU
> > ld linker script.
> > 
> > The special handling of the object file containing the header data as
> > the first object file passed to the linker command line can also be
> > removed.
> > 
> > While there also remove the special handling of efi/ on x86. There's
> > no need for the resulting object file to be passed in any special
> > order to the linker.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Looks good to me, but I have one question before feeling ready to
> offer R-b:
> 
> > @@ -86,8 +84,13 @@ SECTIONS
> >         *(.text.kexec)          /* Page aligned in the object file. */
> >         kexec_reloc_end = .;
> >  
> > -       *(.text.cold)
> > -       *(.text.unlikely)
> > +       *(.text.cold .text.cold.*)
> > +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> 
> What generates .text.*_unlikely? And if anything really does, why
> would .text.cold not have a similar equivalent?

That matches what I saw in the default linker script from my version
of GNU ld:

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

I really don't know what could generate .text.*_unlikely, but since
it's part of the default linker script I assumed it was better to just
add it.

Thanks, Roger.

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Jan Beulich]

On 08.03.2022 15:18, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 02:57:23PM +0100, Jan Beulich wrote:
>> On 08.03.2022 14:49, Roger Pau Monne wrote:
>>> So it can be explicitly placed ahead of the rest of the .text content
>>> in the linker script (and thus the resulting image). This is a
>>> prerequisite for further work that will add a catch-all to the text
>>> section (.text.*).
>>>
>>> Note that placement of the sections inside of .text is also slightly
>>> adjusted to be more similar to the position found in the default GNU
>>> ld linker script.
>>>
>>> The special handling of the object file containing the header data as
>>> the first object file passed to the linker command line can also be
>>> removed.
>>>
>>> While there also remove the special handling of efi/ on x86. There's
>>> no need for the resulting object file to be passed in any special
>>> order to the linker.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Looks good to me, but I have one question before feeling ready to
>> offer R-b:
>>
>>> @@ -86,8 +84,13 @@ SECTIONS
>>>         *(.text.kexec)          /* Page aligned in the object file. */
>>>         kexec_reloc_end = .;
>>>  
>>> -       *(.text.cold)
>>> -       *(.text.unlikely)
>>> +       *(.text.cold .text.cold.*)
>>> +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>
>> What generates .text.*_unlikely? And if anything really does, why
>> would .text.cold not have a similar equivalent?
> 
> That matches what I saw in the default linker script from my version
> of GNU ld:
> 
>     *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> 
> I really don't know what could generate .text.*_unlikely, but since
> it's part of the default linker script I assumed it was better to just
> add it.

I've checked - gcc up to 4.5.x would generate .text.*_unlikely; from
4.6.x. onwards it would be .text.unlikely.*.

As to the dissimilarity with .text.cold: I wonder why we have that in
the first place. It matches our __cold attribute, just that we don't
use that anywhere afaics.

In any event:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit preferably with .text.cold.* dropped again.

Jan

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Roger Pau Monné]

On Tue, Mar 08, 2022 at 04:08:53PM +0100, Jan Beulich wrote:
> On 08.03.2022 15:18, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 02:57:23PM +0100, Jan Beulich wrote:
> >> On 08.03.2022 14:49, Roger Pau Monne wrote:
> >>> So it can be explicitly placed ahead of the rest of the .text content
> >>> in the linker script (and thus the resulting image). This is a
> >>> prerequisite for further work that will add a catch-all to the text
> >>> section (.text.*).
> >>>
> >>> Note that placement of the sections inside of .text is also slightly
> >>> adjusted to be more similar to the position found in the default GNU
> >>> ld linker script.
> >>>
> >>> The special handling of the object file containing the header data as
> >>> the first object file passed to the linker command line can also be
> >>> removed.
> >>>
> >>> While there also remove the special handling of efi/ on x86. There's
> >>> no need for the resulting object file to be passed in any special
> >>> order to the linker.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Looks good to me, but I have one question before feeling ready to
> >> offer R-b:
> >>
> >>> @@ -86,8 +84,13 @@ SECTIONS
> >>>         *(.text.kexec)          /* Page aligned in the object file. */
> >>>         kexec_reloc_end = .;
> >>>  
> >>> -       *(.text.cold)
> >>> -       *(.text.unlikely)
> >>> +       *(.text.cold .text.cold.*)
> >>> +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >>
> >> What generates .text.*_unlikely? And if anything really does, why
> >> would .text.cold not have a similar equivalent?
> > 
> > That matches what I saw in the default linker script from my version
> > of GNU ld:
> > 
> >     *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> > 
> > I really don't know what could generate .text.*_unlikely, but since
> > it's part of the default linker script I assumed it was better to just
> > add it.
> 
> I've checked - gcc up to 4.5.x would generate .text.*_unlikely; from
> 4.6.x. onwards it would be .text.unlikely.*.
> 
> As to the dissimilarity with .text.cold: I wonder why we have that in
> the first place. It matches our __cold attribute, just that we don't
> use that anywhere afaics.
> 
> In any event:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit preferably with .text.cold.* dropped again.

Would you mind dropping the .text.cold.* at commit? Otherwise I can
resend.

Thanks, Roger.

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Jan Beulich]

On 08.03.2022 17:36, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 04:08:53PM +0100, Jan Beulich wrote:
>> On 08.03.2022 15:18, Roger Pau Monné wrote:
>>> On Tue, Mar 08, 2022 at 02:57:23PM +0100, Jan Beulich wrote:
>>>> On 08.03.2022 14:49, Roger Pau Monne wrote:
>>>>> So it can be explicitly placed ahead of the rest of the .text content
>>>>> in the linker script (and thus the resulting image). This is a
>>>>> prerequisite for further work that will add a catch-all to the text
>>>>> section (.text.*).
>>>>>
>>>>> Note that placement of the sections inside of .text is also slightly
>>>>> adjusted to be more similar to the position found in the default GNU
>>>>> ld linker script.
>>>>>
>>>>> The special handling of the object file containing the header data as
>>>>> the first object file passed to the linker command line can also be
>>>>> removed.
>>>>>
>>>>> While there also remove the special handling of efi/ on x86. There's
>>>>> no need for the resulting object file to be passed in any special
>>>>> order to the linker.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Looks good to me, but I have one question before feeling ready to
>>>> offer R-b:
>>>>
>>>>> @@ -86,8 +84,13 @@ SECTIONS
>>>>>         *(.text.kexec)          /* Page aligned in the object file. */
>>>>>         kexec_reloc_end = .;
>>>>>  
>>>>> -       *(.text.cold)
>>>>> -       *(.text.unlikely)
>>>>> +       *(.text.cold .text.cold.*)
>>>>> +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>
>>>> What generates .text.*_unlikely? And if anything really does, why
>>>> would .text.cold not have a similar equivalent?
>>>
>>> That matches what I saw in the default linker script from my version
>>> of GNU ld:
>>>
>>>     *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>
>>> I really don't know what could generate .text.*_unlikely, but since
>>> it's part of the default linker script I assumed it was better to just
>>> add it.
>>
>> I've checked - gcc up to 4.5.x would generate .text.*_unlikely; from
>> 4.6.x. onwards it would be .text.unlikely.*.
>>
>> As to the dissimilarity with .text.cold: I wonder why we have that in
>> the first place. It matches our __cold attribute, just that we don't
>> use that anywhere afaics.
>>
>> In any event:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit preferably with .text.cold.* dropped again.
> 
> Would you mind dropping the .text.cold.* at commit? Otherwise I can
> resend.

Sure; no need to resend just for this.

Jan

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Andrew Cooper]

On 08/03/2022 13:49, Roger Pau Monne wrote:
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 08016948ab..47d09d6cf1 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -30,9 +30,13 @@ SECTIONS
>    _start = .;
>    .text : {
>          _stext = .;            /* Text section */
> +       *(.text.header)
> +
> +       *(.text.cold .text.cold.*)
> +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> +
>         *(.text)
> -       *(.text.cold)
> -       *(.text.unlikely)
> +

Most of this hunk wants to move into patch 2.  Patch 1 should only
introduce .text.header.

Same for x86.  Can fix on commit.

~Andrew

Re: [PATCH v3 1/2] xen/build: put image header into a separate section

[Roger Pau Monné]

On Tue, Mar 08, 2022 at 02:11:28PM +0000, Andrew Cooper wrote:
> On 08/03/2022 13:49, Roger Pau Monne wrote:
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 08016948ab..47d09d6cf1 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -30,9 +30,13 @@ SECTIONS
> >    _start = .;
> >    .text : {
> >          _stext = .;            /* Text section */
> > +       *(.text.header)
> > +
> > +       *(.text.cold .text.cold.*)
> > +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> > +
> >         *(.text)
> > -       *(.text.cold)
> > -       *(.text.unlikely)
> > +
> 
> Most of this hunk wants to move into patch 2.  Patch 1 should only
> introduce .text.header.
> 
> Same for x86.  Can fix on commit.

The justification for doing it here is to better match the ordering
used by the default linker script from GNU ld. Obviously this also
benefits the following patch. Feel free to move, but I think it's also
fine to do it here.

Thanks, Roger.

[PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monne]

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

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

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

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monné]

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.

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

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

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monné]

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.

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

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

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monné]

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.

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

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

Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

[Andrew Cooper]

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

Re: [PATCH v3 0/2] livepatch: enable -f{function,data}-sections compiler option

[Julien Grall]

Hi,

On 08/03/2022 13:49, Roger Pau Monne wrote:
> Hello,
> 
> The content in v3 has been split in two patches, but is still mostly the
> same. The main difference is that first patch does a bit of cleanup of
> the build logic now that the header object file doesn't need to be the
> first one passed to the linker script.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (2):
>    xen/build: put image header into a separate section
>    livepatch: set -f{function,data}-sections compiler option

For the Arm bits:

Acked-by: Julien Grall <jgrall@amazon.com> # xen/arm

Cheers,

> 
>   xen/Kconfig                 |  4 +++
>   xen/Makefile                |  2 ++
>   xen/arch/arm/arch.mk        |  2 --
>   xen/arch/arm/arm32/Makefile |  3 +--
>   xen/arch/arm/arm32/head.S   |  1 +
>   xen/arch/arm/arm64/Makefile |  3 +--
>   xen/arch/arm/arm64/head.S   |  1 +
>   xen/arch/arm/xen.lds.S      | 49 ++++++++++++++++++++-----------------
>   xen/arch/x86/Makefile       |  5 ++--
>   xen/arch/x86/arch.mk        |  2 --
>   xen/arch/x86/boot/head.S    |  2 +-
>   xen/arch/x86/xen.lds.S      | 22 ++++++++++-------
>   xen/common/Kconfig          |  1 +
>   13 files changed, 54 insertions(+), 43 deletions(-)
> 

-- 
Julien Grall

Re: [PATCH v3 0/2] livepatch: enable -f{function,data}-sections compiler option

[Roger Pau Monné]

On Tue, Mar 08, 2022 at 02:52:55PM +0000, Julien Grall wrote:
> Hi,
> 
> On 08/03/2022 13:49, Roger Pau Monne wrote:
> > Hello,
> > 
> > The content in v3 has been split in two patches, but is still mostly the
> > same. The main difference is that first patch does a bit of cleanup of
> > the build logic now that the header object file doesn't need to be the
> > first one passed to the linker script.
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (2):
> >    xen/build: put image header into a separate section
> >    livepatch: set -f{function,data}-sections compiler option
> 
> For the Arm bits:
> 
> Acked-by: Julien Grall <jgrall@amazon.com> # xen/arm

Thanks!

I don't think Acks require a scope tag, as you can only Ack changes to
the code you are a maintainer for?

OTOH a 'Reviewed-by:' tag would indeed need to be limited if it only
means to cover xen/arm bits.

In any case I've added the tag as provided to v4.

Roger.

Re: [PATCH v3 0/2] livepatch: enable -f{function,data}-sections compiler option

[Julien Grall]

Hi Roger,

On 09/03/2022 12:28, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 02:52:55PM +0000, Julien Grall wrote:
>> Hi,
>>
>> On 08/03/2022 13:49, Roger Pau Monne wrote:
>>> Hello,
>>>
>>> The content in v3 has been split in two patches, but is still mostly the
>>> same. The main difference is that first patch does a bit of cleanup of
>>> the build logic now that the header object file doesn't need to be the
>>> first one passed to the linker script.
>>>
>>> Thanks, Roger.
>>>
>>> Roger Pau Monne (2):
>>>     xen/build: put image header into a separate section
>>>     livepatch: set -f{function,data}-sections compiler option
>>
>> For the Arm bits:
>>
>> Acked-by: Julien Grall <jgrall@amazon.com> # xen/arm
> 
> Thanks!
> 
> I don't think Acks require a scope tag, as you can only Ack changes to
> the code you are a maintainer for?

It is not required (same for reviewed-by). But it can be used to specify 
exactly which part you ack.

In this case, I am a maintainer for both common and Arm. I chose to ack 
Arm and leave someone else to confirm the change in common is fine.

Furthermore, with my committer hat on, I find handy to know which person 
acked what. This makes easier to check that the code is suitably acked.

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/process/sending-patches.pandoc;h=7ff7826c992b68804b41cc4a8605329f7a10e44c;hb=refs/heads/staging#l241

-- 
Julien Grall