[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option
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. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |