| Message ID | 20220302134425.38465-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | livepatch: set -f{function,data}-sections compiler option | expand |
On 02.03.2022 14:44, 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. Such section only > seems to appear when using -Os, which not the default for debug or > production binaries. That's too optimistic a statement imo. I've observed it appear with -Os, but looking at gcc's gcc/varasm.c:default_function_section() there's ample room for this appearing for other reasons. Also you don't mention .text.exit, which will no longer be discarded. > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -269,6 +269,10 @@ else > CFLAGS += -fomit-frame-pointer > endif > > +ifeq ($(CONFIG_LIVEPATCH),y) > +CFLAGS += -ffunction-sections -fdata-sections > +endif Perhaps CFLAGS-$(CONFIG_LIVEPATCH) += -ffunction-sections -fdata-sections ? > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -88,6 +88,9 @@ SECTIONS > > *(.text.cold) > *(.text.unlikely) > +#ifdef CONFIG_LIVEPATCH > + *(.text.*) > +#endif This coming after the "cold" and "unlikely" special sections and ahead of .fixup isn't very nice. Also from looking at the linker scripts ld supplies I'm getting the impression that there could/ would then also be e.g. .text.cold.* and .text.unlikely.* which you'd want to separate. We may want to put the entry point in a special .text.head, put that first, and then follow ld in putting cold/unlikely stuff ahead of main .text. For the reason given in the description I can see why a conditional is warranted here. But ... > @@ -292,6 +295,9 @@ SECTIONS > *(.data) > *(.data.rel) > *(.data.rel.*) > +#ifdef CONFIG_LIVEPATCH > + *(.data.*) > +#endif > CONSTRUCTORS > } PHDR(text) > > @@ -308,6 +314,9 @@ SECTIONS > . = ALIGN(SMP_CACHE_BYTES); > __per_cpu_data_end = .; > *(.bss) > +#ifdef CONFIG_LIVEPATCH > + *(.bss.*) > +#endif ... are these two really in need of being conditional? > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -353,7 +353,9 @@ config CRYPTO > config LIVEPATCH > bool "Live patching support" > default X86 > - depends on "$(XEN_HAS_BUILD_ID)" = "y" > + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ > + $(cc-option,-ffunction-sections) && \ > + $(cc-option,-fdata-sections) Is this for certain Clang versions? Gcc has been supporting this in 4.1.x already (didn't check when it was introduced). Jan
On 02/03/2022 13:44, Roger Pau Monne wrote: > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index 6443943889..2423d9f490 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -353,7 +353,9 @@ config CRYPTO > config LIVEPATCH > bool "Live patching support" > default X86 > - depends on "$(XEN_HAS_BUILD_ID)" = "y" > + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ > + $(cc-option,-ffunction-sections) && \ > + $(cc-option,-fdata-sections) cc-option can take multiple, so just $(cc-option,-ffunction-sections -fdata-sections) However, we in practice want these in combination with $(LD) --gc-sections anyway although that wants to be separately configurable. Therefore, we probably want something like: config FUNC_SECTIONS bool config LIVEPATCH selects FUNC_SECTIONS or so, so in the future we can add "config LD_GC_SECTIONS" which also selects FUNC_SECTIONS. Thoughts? ~Andrew
On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote: > On 02.03.2022 14:44, 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. Such section only > > seems to appear when using -Os, which not the default for debug or > > production binaries. > > That's too optimistic a statement imo. I've observed it appear with -Os, > but looking at gcc's gcc/varasm.c:default_function_section() there's > ample room for this appearing for other reasons. Also you don't mention > .text.exit, which will no longer be discarded. > > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -269,6 +269,10 @@ else > > CFLAGS += -fomit-frame-pointer > > endif > > > > +ifeq ($(CONFIG_LIVEPATCH),y) > > +CFLAGS += -ffunction-sections -fdata-sections > > +endif > > Perhaps > > CFLAGS-$(CONFIG_LIVEPATCH) += -ffunction-sections -fdata-sections > > ? Sure. > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -88,6 +88,9 @@ SECTIONS > > > > *(.text.cold) > > *(.text.unlikely) > > +#ifdef CONFIG_LIVEPATCH > > + *(.text.*) > > +#endif > > This coming after the "cold" and "unlikely" special sections and > ahead of .fixup isn't very nice. Also from looking at the linker > scripts ld supplies I'm getting the impression that there could/ > would then also be e.g. .text.cold.* and .text.unlikely.* which > you'd want to separate. > > We may want to put the entry point in a special .text.head, put > that first, and then follow ld in putting cold/unlikely stuff ahead > of main .text. I can give that a try. > For the reason given in the description I can see why a conditional > is warranted here. But ... > > > @@ -292,6 +295,9 @@ SECTIONS > > *(.data) > > *(.data.rel) > > *(.data.rel.*) > > +#ifdef CONFIG_LIVEPATCH > > + *(.data.*) > > +#endif > > CONSTRUCTORS > > } PHDR(text) > > > > @@ -308,6 +314,9 @@ SECTIONS > > . = ALIGN(SMP_CACHE_BYTES); > > __per_cpu_data_end = .; > > *(.bss) > > +#ifdef CONFIG_LIVEPATCH > > + *(.bss.*) > > +#endif > > ... are these two really in need of being conditional? Will drop if you agree. I didn't want to risk introducing unwanted changes in the !CONFIG_LIVEPATCH case. > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -353,7 +353,9 @@ config CRYPTO > > config LIVEPATCH > > bool "Live patching support" > > default X86 > > - depends on "$(XEN_HAS_BUILD_ID)" = "y" > > + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ > > + $(cc-option,-ffunction-sections) && \ > > + $(cc-option,-fdata-sections) > > Is this for certain Clang versions? Gcc has been supporting this in > 4.1.x already (didn't check when it was introduced). I've checked clang and it seems to be prevent in at least Clang 5, which is likely enough? I've added the check just to be on the safe side. Thanks, Roger.
On 02.03.2022 16:46, Roger Pau Monné wrote: > On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote: >> On 02.03.2022 14:44, Roger Pau Monne wrote: >>> @@ -292,6 +295,9 @@ SECTIONS >>> *(.data) >>> *(.data.rel) >>> *(.data.rel.*) >>> +#ifdef CONFIG_LIVEPATCH >>> + *(.data.*) >>> +#endif >>> CONSTRUCTORS >>> } PHDR(text) >>> >>> @@ -308,6 +314,9 @@ SECTIONS >>> . = ALIGN(SMP_CACHE_BYTES); >>> __per_cpu_data_end = .; >>> *(.bss) >>> +#ifdef CONFIG_LIVEPATCH >>> + *(.bss.*) >>> +#endif >> >> ... are these two really in need of being conditional? > > Will drop if you agree. I didn't want to risk introducing unwanted > changes in the !CONFIG_LIVEPATCH case. The only "unwanted" change I can imagine here would be that we place a section which the linker would otherwise need to guess how to place, for being "orphan". >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -353,7 +353,9 @@ config CRYPTO >>> config LIVEPATCH >>> bool "Live patching support" >>> default X86 >>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" >>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ >>> + $(cc-option,-ffunction-sections) && \ >>> + $(cc-option,-fdata-sections) >> >> Is this for certain Clang versions? Gcc has been supporting this in >> 4.1.x already (didn't check when it was introduced). > > I've checked clang and it seems to be prevent in at least Clang 5, > which is likely enough? Clang5 accepts the options fine here. But that wouldn't be enough, ./README says "Clang/LLVM 3.5 or later". > I've added the check just to be on the safe side. Well, yes, if you're unsure and the old version can't be checked, then perhaps indeed better to probe. Jan
On 02.03.2022 15:41, Jan Beulich wrote: > On 02.03.2022 14:44, Roger Pau Monne wrote: >> --- a/xen/arch/x86/xen.lds.S >> +++ b/xen/arch/x86/xen.lds.S >> @@ -88,6 +88,9 @@ SECTIONS >> >> *(.text.cold) >> *(.text.unlikely) >> +#ifdef CONFIG_LIVEPATCH >> + *(.text.*) >> +#endif > > This coming after the "cold" and "unlikely" special sections and > ahead of .fixup isn't very nice. Also from looking at the linker > scripts ld supplies I'm getting the impression that there could/ > would then also be e.g. .text.cold.* and .text.unlikely.* which > you'd want to separate. Thinking about it, with -ffunction-sections startup code cannot reasonably be placed in .text.startup, or else there would be confusion with code originating from a function named startup(). But of course .text.startup.* is similarly difficult to arrange for not ending up in the output's main .text, so adding .text.startup.* and .text.exit.* later in the script wouldn't gain us anything. Jan
On Wed, Mar 02, 2022 at 03:35:07PM +0000, Andrew Cooper wrote: > On 02/03/2022 13:44, Roger Pau Monne wrote: > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > > index 6443943889..2423d9f490 100644 > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -353,7 +353,9 @@ config CRYPTO > > config LIVEPATCH > > bool "Live patching support" > > default X86 > > - depends on "$(XEN_HAS_BUILD_ID)" = "y" > > + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ > > + $(cc-option,-ffunction-sections) && \ > > + $(cc-option,-fdata-sections) > > cc-option can take multiple, so just $(cc-option,-ffunction-sections > -fdata-sections) > > However, we in practice want these in combination with $(LD) > --gc-sections anyway although that wants to be separately configurable. > > Therefore, we probably want something like: > > config FUNC_SECTIONS > bool > > config LIVEPATCH > selects FUNC_SECTIONS > > or so, so in the future we can add "config LD_GC_SECTIONS" which also > selects FUNC_SECTIONS. > > Thoughts? Do we want separate options for ffunction-sections and fdata-sections options, or is FUNC_SECTIONS supposed to cover them both? I assume you are fine with Jan's suggestion to not check for the option presence, since it should be in all supported versions. Thanks, Roger.
On Wed, Mar 02, 2022 at 05:09:10PM +0100, Jan Beulich wrote: > On 02.03.2022 16:46, Roger Pau Monné wrote: > > On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote: > >> On 02.03.2022 14:44, Roger Pau Monne wrote: > >>> @@ -292,6 +295,9 @@ SECTIONS > >>> *(.data) > >>> *(.data.rel) > >>> *(.data.rel.*) > >>> +#ifdef CONFIG_LIVEPATCH > >>> + *(.data.*) > >>> +#endif > >>> CONSTRUCTORS > >>> } PHDR(text) > >>> > >>> @@ -308,6 +314,9 @@ SECTIONS > >>> . = ALIGN(SMP_CACHE_BYTES); > >>> __per_cpu_data_end = .; > >>> *(.bss) > >>> +#ifdef CONFIG_LIVEPATCH > >>> + *(.bss.*) > >>> +#endif > >> > >> ... are these two really in need of being conditional? > > > > Will drop if you agree. I didn't want to risk introducing unwanted > > changes in the !CONFIG_LIVEPATCH case. > > The only "unwanted" change I can imagine here would be that we place a > section which the linker would otherwise need to guess how to place, > for being "orphan". > > >>> --- a/xen/common/Kconfig > >>> +++ b/xen/common/Kconfig > >>> @@ -353,7 +353,9 @@ config CRYPTO > >>> config LIVEPATCH > >>> bool "Live patching support" > >>> default X86 > >>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" > >>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ > >>> + $(cc-option,-ffunction-sections) && \ > >>> + $(cc-option,-fdata-sections) > >> > >> Is this for certain Clang versions? Gcc has been supporting this in > >> 4.1.x already (didn't check when it was introduced). > > > > I've checked clang and it seems to be prevent in at least Clang 5, > > which is likely enough? > > Clang5 accepts the options fine here. But that wouldn't be enough, > ./README says "Clang/LLVM 3.5 or later". > > > I've added the check just to be on the safe side. > > Well, yes, if you're unsure and the old version can't be checked, > then perhaps indeed better to probe. OK, so I've managed to probe clang 3.5.0 and it does support -f{function,data}-sections so we can drop the check. Thanks, Roger.
diff --git a/xen/Makefile b/xen/Makefile index ed4891daf1..bf14a9bdd2 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -269,6 +269,10 @@ else CFLAGS += -fomit-frame-pointer endif +ifeq ($(CONFIG_LIVEPATCH),y) +CFLAGS += -ffunction-sections -fdata-sections +endif + 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 08016948ab..1c7c7d5469 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -33,6 +33,9 @@ SECTIONS *(.text) *(.text.cold) *(.text.unlikely) +#ifdef CONFIG_LIVEPATCH + *(.text.*) +#endif *(.fixup) *(.gnu.warning) _etext = .; /* End of text section */ @@ -96,6 +99,9 @@ SECTIONS *(.data.rel) *(.data.rel.*) +#ifdef CONFIG_LIVEPATCH + *(.data.*) +#endif CONSTRUCTORS } :text @@ -208,6 +214,9 @@ SECTIONS . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; *(.bss) +#ifdef CONFIG_LIVEPATCH + *(.bss.*) +#endif . = ALIGN(POINTER_ALIGN); __bss_end = .; } :text diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 83def6541e..81bb608151 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -88,6 +88,9 @@ SECTIONS *(.text.cold) *(.text.unlikely) +#ifdef CONFIG_LIVEPATCH + *(.text.*) +#endif *(.fixup) *(.gnu.warning) _etext = .; /* End of text section */ @@ -292,6 +295,9 @@ SECTIONS *(.data) *(.data.rel) *(.data.rel.*) +#ifdef CONFIG_LIVEPATCH + *(.data.*) +#endif CONSTRUCTORS } PHDR(text) @@ -308,6 +314,9 @@ SECTIONS . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; *(.bss) +#ifdef CONFIG_LIVEPATCH + *(.bss.*) +#endif . = ALIGN(POINTER_ALIGN); __bss_end = .; } PHDR(text) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 6443943889..2423d9f490 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -353,7 +353,9 @@ config CRYPTO config LIVEPATCH bool "Live patching support" default X86 - depends on "$(XEN_HAS_BUILD_ID)" = "y" + depends on "$(XEN_HAS_BUILD_ID)" = "y" && \ + $(cc-option,-ffunction-sections) && \ + $(cc-option,-fdata-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. Such section only seems to appear when using -Os, which not the default for debug or production binaries. The benefit of having CONFIG_LIVEPATCH enable those compiler options 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> --- xen/Makefile | 4 ++++ xen/arch/arm/xen.lds.S | 9 +++++++++ xen/arch/x86/xen.lds.S | 9 +++++++++ xen/common/Kconfig | 4 +++- 4 files changed, 25 insertions(+), 1 deletion(-)