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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan