| Message ID | 20220309122846.89696-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | livepatch: enable -f{function,data}-sections compiler option | expand |
On 09.03.2022 13:28, 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@citrix.com> > Acked-by: Julien Grall <jgrall@amazon.com> # xen/arm Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 09/03/2022 12:28, Roger Pau Monne wrote: > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index 960c51eb4c..4103763f63 100644 > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -87,9 +87,12 @@ SECTIONS > *(.text.cold) > *(.text.unlikely .text.*_unlikely .text.unlikely.*) > > + *(.text.page_aligned) > *(.text) > +#ifdef CONFIG_CC_SPLIT_SECTIONS > + *(.text.*) > +#endif > *(.text.__x86_indirect_thunk_*) > - *(.text.page_aligned) > > *(.fixup) > *(.gnu.warning) > @@ -292,9 +295,7 @@ SECTIONS > > DECL_SECTION(.data) { > *(.data.page_aligned) > - *(.data) > - *(.data.rel) > - *(.data.rel.*) > + *(.data .data.*) > } PHDR(text) > > DECL_SECTION(.bss) { > @@ -309,7 +310,7 @@ SECTIONS > *(.bss.percpu.read_mostly) > . = ALIGN(SMP_CACHE_BYTES); > __per_cpu_data_end = .; > - *(.bss) > + *(.bss .bss.*) Sorry if I've missed it elsewhere, but why are .data.* and .bss.* unguarded, but .text.* under ifdef ? Surely they should have the same disposition? ~Andrew
On Wed, Mar 09, 2022 at 02:58:06PM +0000, Andrew Cooper wrote: > On 09/03/2022 12:28, Roger Pau Monne wrote: > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > index 960c51eb4c..4103763f63 100644 > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -87,9 +87,12 @@ SECTIONS > > *(.text.cold) > > *(.text.unlikely .text.*_unlikely .text.unlikely.*) > > > > + *(.text.page_aligned) > > *(.text) > > +#ifdef CONFIG_CC_SPLIT_SECTIONS > > + *(.text.*) > > +#endif > > *(.text.__x86_indirect_thunk_*) > > - *(.text.page_aligned) > > > > *(.fixup) > > *(.gnu.warning) > > @@ -292,9 +295,7 @@ SECTIONS > > > > DECL_SECTION(.data) { > > *(.data.page_aligned) > > - *(.data) > > - *(.data.rel) > > - *(.data.rel.*) > > + *(.data .data.*) > > } PHDR(text) > > > > DECL_SECTION(.bss) { > > @@ -309,7 +310,7 @@ SECTIONS > > *(.bss.percpu.read_mostly) > > . = ALIGN(SMP_CACHE_BYTES); > > __per_cpu_data_end = .; > > - *(.bss) > > + *(.bss .bss.*) > > Sorry if I've missed it elsewhere, but why are .data.* and .bss.* > unguarded, but .text.* under ifdef ? > > Surely they should have the same disposition? The catch-all .text.* added for -ffunction-sections will mean that .text.startup and .text.exit will end up in .text instead of in .init and discarded respectively. That's not the case for the data or bss catch-alls. Thanks, Roger.
diff --git a/xen/Kconfig b/xen/Kconfig index bcbd2758e5..d134397a0b 100644 --- a/xen/Kconfig +++ b/xen/Kconfig @@ -27,6 +27,10 @@ config CLANG_VERSION config CC_HAS_VISIBILITY_ATTRIBUTE def_bool $(cc-option,-fvisibility=hidden) +# Use -f{function,data}-sections compiler parameters +config CC_SPLIT_SECTIONS + bool + source "arch/$(SRCARCH)/Kconfig" config DEFCONFIG_LIST diff --git a/xen/Makefile b/xen/Makefile index 5c21492d6f..18a4f7e101 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -273,6 +273,8 @@ else CFLAGS += -fomit-frame-pointer endif +CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections + CFLAGS += -nostdinc -fno-builtin -fno-common CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith $(call cc-option-add,CFLAGS,CC,-Wvla) diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 743455a5f9..7921d8fa28 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -36,6 +36,9 @@ SECTIONS *(.text.unlikely .text.*_unlikely .text.unlikely.*) *(.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.text.*) +#endif *(.fixup) *(.gnu.warning) @@ -82,10 +85,24 @@ SECTIONS #endif _erodata = .; /* End of read-only data */ + . = ALIGN(PAGE_SIZE); + .data.read_mostly : { + /* Exception table */ + __start___ex_table = .; + *(.ex_table) + __stop___ex_table = .; + + /* Pre-exception table */ + __start___pre_ex_table = .; + *(.ex_table.pre) + __stop___pre_ex_table = .; + + *(.data.read_mostly) + } :text + + . = ALIGN(SMP_CACHE_BYTES); .data : { /* Data */ - . = ALIGN(PAGE_SIZE); *(.data.page_aligned) - *(.data) . = ALIGN(8); __start_schedulers_array = .; *(.data.schedulers) @@ -98,26 +115,10 @@ SECTIONS __paramhypfs_end = .; #endif - *(.data.rel) - *(.data.rel.*) + *(.data .data.*) CONSTRUCTORS } :text - . = ALIGN(SMP_CACHE_BYTES); - .data.read_mostly : { - /* Exception table */ - __start___ex_table = .; - *(.ex_table) - __stop___ex_table = .; - - /* Pre-exception table */ - __start___pre_ex_table = .; - *(.ex_table.pre) - __stop___pre_ex_table = .; - - *(.data.read_mostly) - } :text - . = ALIGN(8); .arch.info : { _splatform = .; @@ -211,7 +212,7 @@ SECTIONS *(.bss.percpu.read_mostly) . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; - *(.bss) + *(.bss .bss.*) . = ALIGN(POINTER_ALIGN); __bss_end = .; } :text diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 960c51eb4c..4103763f63 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -87,9 +87,12 @@ SECTIONS *(.text.cold) *(.text.unlikely .text.*_unlikely .text.unlikely.*) + *(.text.page_aligned) *(.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.text.*) +#endif *(.text.__x86_indirect_thunk_*) - *(.text.page_aligned) *(.fixup) *(.gnu.warning) @@ -292,9 +295,7 @@ SECTIONS DECL_SECTION(.data) { *(.data.page_aligned) - *(.data) - *(.data.rel) - *(.data.rel.*) + *(.data .data.*) } PHDR(text) DECL_SECTION(.bss) { @@ -309,7 +310,7 @@ SECTIONS *(.bss.percpu.read_mostly) . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; - *(.bss) + *(.bss .bss.*) . = ALIGN(POINTER_ALIGN); __bss_end = .; } PHDR(text) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 6443943889..d921c74d61 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -354,6 +354,7 @@ config LIVEPATCH bool "Live patching support" default X86 depends on "$(XEN_HAS_BUILD_ID)" = "y" + select CC_SPLIT_SECTIONS ---help--- Allows a running Xen hypervisor to be dynamically patched using binary patches without rebooting. This is primarily used to binarily
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@citrix.com> Acked-by: Julien Grall <jgrall@amazon.com> # xen/arm --- Changes since v3: - Place .text.page_aligned ahead of the .text.* catch-all. Changes since v2: - Split the placing of the header code in a separate section to a pre-patch. - Move Kconfig option to xen/Kconfig. - Expand reasoning why .data.read_mostly needs to be moved on Arm. Changes since v1: - Introduce CC_SPLIT_SECTIONS for selecting the compiler options. - Drop check for compiler options, all supported versions have them. - Re-arrange section placement in .text, to match the default linker script. - Introduce .text.header to contain the headers bits that must appear first in the final binary. --- It seems on Arm the schedulers and hypfs .data sections should be moved into read_mostly. --- Tested by gitlab in order to assert I didn't introduce any regression on Arm specially. --- xen/Kconfig | 4 ++++ xen/Makefile | 2 ++ xen/arch/arm/xen.lds.S | 41 +++++++++++++++++++++-------------------- xen/arch/x86/xen.lds.S | 11 ++++++----- xen/common/Kconfig | 1 + 5 files changed, 34 insertions(+), 25 deletions(-)