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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Mar 07, 2022 at 05:19:53PM +0000, Julien Grall wrote:
> 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?

No, .data.read_mostly needs to be moved before .data so that the catch
all .data.* introduced to .data to account for -fdata-sections doesn't
end up also including .data.read_mostly.

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

I had a note below the commit message about this, as I didn't think it
was strictly necessary to get this accepted. I will do a pre-patch
then with those added.

Thanks, Roger.