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

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

On Mon, Mar 07, 2022 at 06:07:00PM +0100, Jan Beulich wrote:
> On 07.03.2022 17:36, Roger Pau Monné wrote:
> > 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.
> 
> I guess the main question is whether this option is intended to gain
> a prompt. If so, xen/common/Kconfig is likely the better place. If
> not, I think it better fits in xen/Kconfig.

I think it's unlikely for it to gain a prompt, other options selecting
it is what I would expect.

Will move to xen/Kconfig unless someone objects.

Thanks, Roger.