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

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 07/03/2022 15:55, 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.
>
> 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. This requires having a separate section for the
> header in order to place it at the begging of the output image,
> followed with the unlikely and related sections, and finally the main
> .text section.
>
> On Arm the .data.read_mostly needs to be moved ahead of the .data
> section like it's already done on x86, and the alignment 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.
This sounds like a bug not related to this patch. Can this be split 
separately?
> 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@xxxxxxxxxx>
> ---
> 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.
> ---
> Given that now the header is explicitly placed in .text.header, it's
> likely possible to simplify some of the ordering of the object files
> for the prelink.o generation, as arch/x86/boot/built_in.o no longer
> needs to be the first object file in the list.
>
> It also 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/Makefile              |  2 ++
>   xen/arch/arm/arm32/head.S |  1 +
>   xen/arch/arm/arm64/head.S |  1 +
>   xen/arch/arm/xen.lds.S    | 49 +++++++++++++++++++++------------------
>   xen/arch/x86/boot/head.S  |  2 +-
>   xen/arch/x86/xen.lds.S    | 22 +++++++++++-------
>   xen/common/Kconfig        |  4 ++++
>   7 files changed, 49 insertions(+), 32 deletions(-)
>
> 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/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/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..836da880c3 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -30,9 +30,16 @@ SECTIONS
>     _start = .;
>     .text : {
>           _stext = .;            /* Text section */
> +       *(.text.header)
With this change, the changes in arch/*/arch.mk to order head.o looks 
unnecessary. Can we remove it at the same time? (The .text.header and 
the update of arch.mk may want to be done together in a separate patch).
> +
> +       *(.text.cold .text.cold.*)
> +       *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> +
>          *(.text)
> -       *(.text.cold)
> -       *(.text.unlikely)
> +#ifdef CONFIG_CC_SPLIT_SECTIONS
> +       *(.text.*)
> +#endif
> +
>          *(.fixup)
>          *(.gnu.warning)
>          _etext = .;             /* End of text section */
> @@ -78,10 +85,24 @@ SECTIONS
>   #endif
>     _erodata = .;                /* End of read-only data */
Cheers,

--
Julien Grall