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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2022 14:44, 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. Such section only
> seems to appear when using -Os, which not the default for debug or
> production binaries.

That's too optimistic a statement imo. I've observed it appear with -Os,
but looking at gcc's gcc/varasm.c:default_function_section() there's
ample room for this appearing for other reasons. Also you don't mention
.text.exit, which will no longer be discarded.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -269,6 +269,10 @@ else
>  CFLAGS += -fomit-frame-pointer
>  endif
>  
> +ifeq ($(CONFIG_LIVEPATCH),y)
> +CFLAGS += -ffunction-sections -fdata-sections
> +endif

Perhaps

CFLAGS-$(CONFIG_LIVEPATCH) += -ffunction-sections -fdata-sections

?

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

This coming after the "cold" and "unlikely" special sections and
ahead of .fixup isn't very nice. Also from looking at the linker
scripts ld supplies I'm getting the impression that there could/
would then also be e.g. .text.cold.* and .text.unlikely.* which
you'd want to separate.

We may want to put the entry point in a special .text.head, put
that first, and then follow ld in putting cold/unlikely stuff ahead
of main .text.

For the reason given in the description I can see why a conditional
is warranted here. But ...

> @@ -292,6 +295,9 @@ SECTIONS
>         *(.data)
>         *(.data.rel)
>         *(.data.rel.*)
> +#ifdef CONFIG_LIVEPATCH
> +       *(.data.*)
> +#endif
>         CONSTRUCTORS
>    } PHDR(text)
>  
> @@ -308,6 +314,9 @@ SECTIONS
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_data_end = .;
>         *(.bss)
> +#ifdef CONFIG_LIVEPATCH
> +       *(.bss.*)
> +#endif

... are these two really in need of being conditional?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -353,7 +353,9 @@ config CRYPTO
>  config LIVEPATCH
>       bool "Live patching support"
>       default X86
> -     depends on "$(XEN_HAS_BUILD_ID)" = "y"
> +     depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
> +                $(cc-option,-ffunction-sections) && \
> +                $(cc-option,-fdata-sections)

Is this for certain Clang versions? Gcc has been supporting this in
4.1.x already (didn't check when it was introduced).

Jan