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(-)
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(-)
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
--
2.34.1
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: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 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 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
© 2016 - 2024 Red Hat, Inc.