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

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

On Tue, Mar 08, 2022 at 05:58:49PM +0100, Jan Beulich wrote:
> On 08.03.2022 17:41, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
> >> On 08.03.2022 15:46, Roger Pau Monné wrote:
> >>> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
> >>>> On 08.03.2022 14:49, 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.
> >>>>>
> >>>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
> >>>>> section like it's already done on x86, so the .data.* catch-all
> >>>>> doesn't also include .data.read_mostly. The alignment of
> >>>>> .data.read_mostly also 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>
> >>>>
> >>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>>
> >>>>> --- a/xen/arch/x86/xen.lds.S
> >>>>> +++ b/xen/arch/x86/xen.lds.S
> >>>>> @@ -88,6 +88,9 @@ SECTIONS
> >>>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >>>>>  
> >>>>>         *(.text)
> >>>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
> >>>>> +       *(.text.*)
> >>>>> +#endif
> >>>>>         *(.text.__x86_indirect_thunk_*)
> >>>>>         *(.text.page_aligned)
> >>>>
> >>>> These last two now will not have any effect anymore when
> >>>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
> >>>> overall size when there is more than one object with a
> >>>> .text.page_aligned contribution. In .data ...
> >>>
> >>> Agreed. I wondered whether to move those ahead of the main text
> >>> section, so likely:
> >>>
> >>>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> >>>
> >>>        *(.text.page_aligned)
> >>>        *(.text.__x86_indirect_thunk_*)
> >>>        *(.text)
> >>> #ifdef CONFIG_CC_SPLIT_SECTIONS
> >>>        *(.text.*)
> >>> #endif
> >>
> >> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
> >> though. When adding .text.* that one can likely go away.
> >>
> >>> FWIW, Linux seems fine to package .text.page_aligned together with the
> >>> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
> >>
> >> There's no question this is functionally fine. The question is how
> >> many extra padding areas are inserted because of this.
> >>
> >>>>> @@ -292,9 +295,7 @@ SECTIONS
> >>>>>  
> >>>>>    DECL_SECTION(.data) {
> >>>>>         *(.data.page_aligned)
> >>>>> -       *(.data)
> >>>>> -       *(.data.rel)
> >>>>> -       *(.data.rel.*)
> >>>>> +       *(.data .data.*)
> >>>>>    } PHDR(text)
> >>>>
> >>>> ... this continues to be named first. I wonder whether we wouldn't
> >>>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
> >>>
> >>> We could use the command line option if available
> >>> (--sort-section=alignment) to sort all wildcard sections?
> >>
> >> Depends on the scope of the sorting that would result when enabled
> >> globally like this.
> > 
> > I'm not sure I'm following. Don't we generally want to sort by
> > alignment in order to avoid adding unnecessary padding as much as
> > possible?
> > 
> > For any wildcard sections we really don't care anymore how they are
> > sorted?
> 
> Sure. Question is whether sorting is limited to within any single
> *(...) construct, or whether it could extend to adjacent ones. IOW
> whether the command line option strictly is a replacement of adding
> SORT_BY_ALIGNMENT to every one of these constructs.

AFAICT the command line option will have the effect of setting the
sorting of any wildcard containing sections to use SORT_BY_ALIGNMENT.
Ie: .data.* would become SORT_BY_ALIGNMENT(.data.*):

*(.data SORT_BY_ALIGNMENT(.data.*))

I've taken a look at the binutils ld source and that seems to be the
case, any wildcard containing enum will get it's sorting set to by
alignment (but I'm not familiar with ld code so I might be missing
pieces).

Thanks, Roger.