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

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

On Mon, Mar 07, 2022 at 05:28:20PM +0100, Jan Beulich wrote:
> On 07.03.2022 16: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.
> > 
> > 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>
> 
> The x86 part of this looks fine to me, just one other remark:
> 
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -350,10 +350,14 @@ source "common/sched/Kconfig"
> >  config CRYPTO
> >     bool
> >  
> > +config CC_SPLIT_SECTIONS
> > +   bool
> 
> I think this wants to live higher up in the file, perhaps between
> ALTERNATIVE_CALL and HAS_ALTERNATIVE. (CRYPTO, as seen in context
> here, imo also would better live higher up.) Or alternatively it may
> want to move to xen/Kconfig, next to CC_HAS_VISIBILITY_ATTRIBUTE.

I was tempted to place it in xen/Kconfig. The logic for the current
suggested placement is to be closer to it's current only user
(LIVEPATCH), but I'm not opposed to moving it somewhere else if
there's consensus.

Thanks, Roger.