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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan