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

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

On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote:
> 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
> 
> ?

Sure.

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

I can give that a try.

> 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?

Will drop if you agree. I didn't want to risk introducing unwanted
changes in the !CONFIG_LIVEPATCH case.

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

I've checked clang and it seems to be prevent in at least Clang 5,
which is likely enough?

I've added the check just to be on the safe side.

Thanks, Roger.