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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 09/03/2022 10:22, Jan Beulich wrote:
> On 09.03.2022 10:30, Roger Pau Monné wrote:
>> 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).
> Okay - why don't we try that then (in a separate patch, so it's going
> to be easy to revert)? For the patch here all I'd like to ask for is
> to keep .text.page_aligned enumerated explicitly (and the wildcard
> placed after it, obviously).

I'm not convinced this will be an improvement.  It will make a marginal
space saving, but cost runtime performance by breaking
locality-of-reference inside a TU.

What would make an improvement (if this isn't how it already works) is
having each TU sort by alignment on its own, then link in order.

~Andrew