| Message ID | 20220307155558.72876-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] livepatch: set -f{function,data}-sections compiler option | expand |
On 07.03.2022 16:55, 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. > > Note that placement of the sections inside of .text is also slightly > adjusted to be more similar to the position found in the default GNU > ld linker script. This requires having a separate section for the > header in order to place it at the begging of the output image, > followed with the unlikely and related sections, and finally the main > .text section. > > On Arm the .data.read_mostly needs to be moved ahead of the .data > section like it's already done on x86, and the alignment 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> The x86 part of this looks fine to me, just one other remark: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -350,10 +350,14 @@ source "common/sched/Kconfig" > config CRYPTO > bool > > +config CC_SPLIT_SECTIONS > + bool I think this wants to live higher up in the file, perhaps between ALTERNATIVE_CALL and HAS_ALTERNATIVE. (CRYPTO, as seen in context here, imo also would better live higher up.) Or alternatively it may want to move to xen/Kconfig, next to CC_HAS_VISIBILITY_ATTRIBUTE. Jan
On Mon, Mar 07, 2022 at 05:28:20PM +0100, Jan Beulich wrote: > On 07.03.2022 16:55, 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. > > > > Note that placement of the sections inside of .text is also slightly > > adjusted to be more similar to the position found in the default GNU > > ld linker script. This requires having a separate section for the > > header in order to place it at the begging of the output image, > > followed with the unlikely and related sections, and finally the main > > .text section. > > > > On Arm the .data.read_mostly needs to be moved ahead of the .data > > section like it's already done on x86, and the alignment 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> > > The x86 part of this looks fine to me, just one other remark: > > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -350,10 +350,14 @@ source "common/sched/Kconfig" > > config CRYPTO > > bool > > > > +config CC_SPLIT_SECTIONS > > + bool > > I think this wants to live higher up in the file, perhaps between > ALTERNATIVE_CALL and HAS_ALTERNATIVE. (CRYPTO, as seen in context > here, imo also would better live higher up.) Or alternatively it may > want to move to xen/Kconfig, next to CC_HAS_VISIBILITY_ATTRIBUTE. I was tempted to place it in xen/Kconfig. The logic for the current suggested placement is to be closer to it's current only user (LIVEPATCH), but I'm not opposed to moving it somewhere else if there's consensus. Thanks, Roger.
On 07.03.2022 17:36, Roger Pau Monné wrote: > On Mon, Mar 07, 2022 at 05:28:20PM +0100, Jan Beulich wrote: >> On 07.03.2022 16:55, 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. >>> >>> Note that placement of the sections inside of .text is also slightly >>> adjusted to be more similar to the position found in the default GNU >>> ld linker script. This requires having a separate section for the >>> header in order to place it at the begging of the output image, >>> followed with the unlikely and related sections, and finally the main >>> .text section. >>> >>> On Arm the .data.read_mostly needs to be moved ahead of the .data >>> section like it's already done on x86, and the alignment 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> >> >> The x86 part of this looks fine to me, just one other remark: >> >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -350,10 +350,14 @@ source "common/sched/Kconfig" >>> config CRYPTO >>> bool >>> >>> +config CC_SPLIT_SECTIONS >>> + bool >> >> I think this wants to live higher up in the file, perhaps between >> ALTERNATIVE_CALL and HAS_ALTERNATIVE. (CRYPTO, as seen in context >> here, imo also would better live higher up.) Or alternatively it may >> want to move to xen/Kconfig, next to CC_HAS_VISIBILITY_ATTRIBUTE. > > I was tempted to place it in xen/Kconfig. The logic for the current > suggested placement is to be closer to it's current only user > (LIVEPATCH), but I'm not opposed to moving it somewhere else if > there's consensus. I guess the main question is whether this option is intended to gain a prompt. If so, xen/common/Kconfig is likely the better place. If not, I think it better fits in xen/Kconfig. Jan
Hi Roger, On 07/03/2022 15:55, 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. > > Note that placement of the sections inside of .text is also slightly > adjusted to be more similar to the position found in the default GNU > ld linker script. This requires having a separate section for the > header in order to place it at the begging of the output image, > followed with the unlikely and related sections, and finally the main > .text section. > > On Arm the .data.read_mostly needs to be moved ahead of the .data > section like it's already done on x86, and the alignment 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. This sounds like a bug not related to this patch. Can this be split separately? > > 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> > --- > 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. > --- > Given that now the header is explicitly placed in .text.header, it's > likely possible to simplify some of the ordering of the object files > for the prelink.o generation, as arch/x86/boot/built_in.o no longer > needs to be the first object file in the list. > > It also 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/Makefile | 2 ++ > xen/arch/arm/arm32/head.S | 1 + > xen/arch/arm/arm64/head.S | 1 + > xen/arch/arm/xen.lds.S | 49 +++++++++++++++++++++------------------ > xen/arch/x86/boot/head.S | 2 +- > xen/arch/x86/xen.lds.S | 22 +++++++++++------- > xen/common/Kconfig | 4 ++++ > 7 files changed, 49 insertions(+), 32 deletions(-) > > 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/arm32/head.S b/xen/arch/arm/arm32/head.S > index 7a906167ef..c837d3054c 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -120,6 +120,7 @@ > > #endif /* !CONFIG_EARLY_PRINTK */ > > + .section .text.header, "ax", %progbits > .arm > > /* > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 66d862fc81..e62c48ec1c 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -133,6 +133,7 @@ > add \xb, \xb, x20 > .endm > > + .section .text.header, "ax", %progbits > /*.aarch64*/ > > /* > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 08016948ab..836da880c3 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -30,9 +30,16 @@ SECTIONS > _start = .; > .text : { > _stext = .; /* Text section */ > + *(.text.header) With this change, the changes in arch/*/arch.mk to order head.o looks unnecessary. Can we remove it at the same time? (The .text.header and the update of arch.mk may want to be done together in a separate patch). > + > + *(.text.cold .text.cold.*) > + *(.text.unlikely .text.*_unlikely .text.unlikely.*) > + > *(.text) > - *(.text.cold) > - *(.text.unlikely) > +#ifdef CONFIG_CC_SPLIT_SECTIONS > + *(.text.*) > +#endif > + > *(.fixup) > *(.gnu.warning) > _etext = .; /* End of text section */ > @@ -78,10 +85,24 @@ SECTIONS > #endif > _erodata = .; /* End of read-only data */ Cheers,
On Mon, Mar 07, 2022 at 06:07:00PM +0100, Jan Beulich wrote: > On 07.03.2022 17:36, Roger Pau Monné wrote: > > On Mon, Mar 07, 2022 at 05:28:20PM +0100, Jan Beulich wrote: > >> On 07.03.2022 16:55, 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. > >>> > >>> Note that placement of the sections inside of .text is also slightly > >>> adjusted to be more similar to the position found in the default GNU > >>> ld linker script. This requires having a separate section for the > >>> header in order to place it at the begging of the output image, > >>> followed with the unlikely and related sections, and finally the main > >>> .text section. > >>> > >>> On Arm the .data.read_mostly needs to be moved ahead of the .data > >>> section like it's already done on x86, and the alignment 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> > >> > >> The x86 part of this looks fine to me, just one other remark: > >> > >>> --- a/xen/common/Kconfig > >>> +++ b/xen/common/Kconfig > >>> @@ -350,10 +350,14 @@ source "common/sched/Kconfig" > >>> config CRYPTO > >>> bool > >>> > >>> +config CC_SPLIT_SECTIONS > >>> + bool > >> > >> I think this wants to live higher up in the file, perhaps between > >> ALTERNATIVE_CALL and HAS_ALTERNATIVE. (CRYPTO, as seen in context > >> here, imo also would better live higher up.) Or alternatively it may > >> want to move to xen/Kconfig, next to CC_HAS_VISIBILITY_ATTRIBUTE. > > > > I was tempted to place it in xen/Kconfig. The logic for the current > > suggested placement is to be closer to it's current only user > > (LIVEPATCH), but I'm not opposed to moving it somewhere else if > > there's consensus. > > I guess the main question is whether this option is intended to gain > a prompt. If so, xen/common/Kconfig is likely the better place. If > not, I think it better fits in xen/Kconfig. I think it's unlikely for it to gain a prompt, other options selecting it is what I would expect. Will move to xen/Kconfig unless someone objects. Thanks, Roger.
On Mon, Mar 07, 2022 at 05:19:53PM +0000, Julien Grall wrote: > Hi Roger, > > On 07/03/2022 15:55, 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. > > > > Note that placement of the sections inside of .text is also slightly > > adjusted to be more similar to the position found in the default GNU > > ld linker script. This requires having a separate section for the > > header in order to place it at the begging of the output image, > > followed with the unlikely and related sections, and finally the main > > .text section. > > > > On Arm the .data.read_mostly needs to be moved ahead of the .data > > section like it's already done on x86, and the alignment 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. > > This sounds like a bug not related to this patch. Can this be split > separately? No, .data.read_mostly needs to be moved before .data so that the catch all .data.* introduced to .data to account for -fdata-sections doesn't end up also including .data.read_mostly. > > > > 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> > > --- > > 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. > > --- > > Given that now the header is explicitly placed in .text.header, it's > > likely possible to simplify some of the ordering of the object files > > for the prelink.o generation, as arch/x86/boot/built_in.o no longer > > needs to be the first object file in the list. > > > > It also 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/Makefile | 2 ++ > > xen/arch/arm/arm32/head.S | 1 + > > xen/arch/arm/arm64/head.S | 1 + > > xen/arch/arm/xen.lds.S | 49 +++++++++++++++++++++------------------ > > xen/arch/x86/boot/head.S | 2 +- > > xen/arch/x86/xen.lds.S | 22 +++++++++++------- > > xen/common/Kconfig | 4 ++++ > > 7 files changed, 49 insertions(+), 32 deletions(-) > > > > 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/arm32/head.S b/xen/arch/arm/arm32/head.S > > index 7a906167ef..c837d3054c 100644 > > --- a/xen/arch/arm/arm32/head.S > > +++ b/xen/arch/arm/arm32/head.S > > @@ -120,6 +120,7 @@ > > #endif /* !CONFIG_EARLY_PRINTK */ > > + .section .text.header, "ax", %progbits > > .arm > > /* > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > > index 66d862fc81..e62c48ec1c 100644 > > --- a/xen/arch/arm/arm64/head.S > > +++ b/xen/arch/arm/arm64/head.S > > @@ -133,6 +133,7 @@ > > add \xb, \xb, x20 > > .endm > > + .section .text.header, "ax", %progbits > > /*.aarch64*/ > > /* > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > > index 08016948ab..836da880c3 100644 > > --- a/xen/arch/arm/xen.lds.S > > +++ b/xen/arch/arm/xen.lds.S > > @@ -30,9 +30,16 @@ SECTIONS > > _start = .; > > .text : { > > _stext = .; /* Text section */ > > + *(.text.header) > > With this change, the changes in arch/*/arch.mk to order head.o looks > unnecessary. Can we remove it at the same time? (The .text.header and the > update of arch.mk may want to be done together in a separate patch). I had a note below the commit message about this, as I didn't think it was strictly necessary to get this accepted. I will do a pre-patch then with those added. Thanks, Roger.
Hi Roger, On 08/03/2022 08:13, Roger Pau Monné wrote: > On Mon, Mar 07, 2022 at 05:19:53PM +0000, Julien Grall wrote: >> Hi Roger, >> >> On 07/03/2022 15:55, 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. >>> >>> Note that placement of the sections inside of .text is also slightly >>> adjusted to be more similar to the position found in the default GNU >>> ld linker script. This requires having a separate section for the >>> header in order to place it at the begging of the output image, >>> followed with the unlikely and related sections, and finally the main >>> .text section. >>> >>> On Arm the .data.read_mostly needs to be moved ahead of the .data >>> section like it's already done on x86, and the alignment 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. >> >> This sounds like a bug not related to this patch. Can this be split >> separately? > > No, .data.read_mostly needs to be moved before .data so that the catch > all .data.* introduced to .data to account for -fdata-sections doesn't > end up also including .data.read_mostly. Sorry I misread it. I thought you suggested the alignment was also wrong before this patch. Thanks for the clarification! > >>> >>> 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> >>> --- >>> 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. >>> --- >>> Given that now the header is explicitly placed in .text.header, it's >>> likely possible to simplify some of the ordering of the object files >>> for the prelink.o generation, as arch/x86/boot/built_in.o no longer >>> needs to be the first object file in the list. >>> >>> It also 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/Makefile | 2 ++ >>> xen/arch/arm/arm32/head.S | 1 + >>> xen/arch/arm/arm64/head.S | 1 + >>> xen/arch/arm/xen.lds.S | 49 +++++++++++++++++++++------------------ >>> xen/arch/x86/boot/head.S | 2 +- >>> xen/arch/x86/xen.lds.S | 22 +++++++++++------- >>> xen/common/Kconfig | 4 ++++ >>> 7 files changed, 49 insertions(+), 32 deletions(-) >>> >>> 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/arm32/head.S b/xen/arch/arm/arm32/head.S >>> index 7a906167ef..c837d3054c 100644 >>> --- a/xen/arch/arm/arm32/head.S >>> +++ b/xen/arch/arm/arm32/head.S >>> @@ -120,6 +120,7 @@ >>> #endif /* !CONFIG_EARLY_PRINTK */ >>> + .section .text.header, "ax", %progbits >>> .arm >>> /* >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>> index 66d862fc81..e62c48ec1c 100644 >>> --- a/xen/arch/arm/arm64/head.S >>> +++ b/xen/arch/arm/arm64/head.S >>> @@ -133,6 +133,7 @@ >>> add \xb, \xb, x20 >>> .endm >>> + .section .text.header, "ax", %progbits >>> /*.aarch64*/ >>> /* >>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>> index 08016948ab..836da880c3 100644 >>> --- a/xen/arch/arm/xen.lds.S >>> +++ b/xen/arch/arm/xen.lds.S >>> @@ -30,9 +30,16 @@ SECTIONS >>> _start = .; >>> .text : { >>> _stext = .; /* Text section */ >>> + *(.text.header) >> >> With this change, the changes in arch/*/arch.mk to order head.o looks >> unnecessary. Can we remove it at the same time? (The .text.header and the >> update of arch.mk may want to be done together in a separate patch). > > I had a note below the commit message about this, as I didn't think it > was strictly necessary to get this accepted. I will do a pre-patch > then with those added. Ah, I didn't spot the note. I would prefer if such clean-up is done now. Cheers,
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/arm32/head.S b/xen/arch/arm/arm32/head.S index 7a906167ef..c837d3054c 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -120,6 +120,7 @@ #endif /* !CONFIG_EARLY_PRINTK */ + .section .text.header, "ax", %progbits .arm /* diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 66d862fc81..e62c48ec1c 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -133,6 +133,7 @@ add \xb, \xb, x20 .endm + .section .text.header, "ax", %progbits /*.aarch64*/ /* diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 08016948ab..836da880c3 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -30,9 +30,16 @@ SECTIONS _start = .; .text : { _stext = .; /* Text section */ + *(.text.header) + + *(.text.cold .text.cold.*) + *(.text.unlikely .text.*_unlikely .text.unlikely.*) + *(.text) - *(.text.cold) - *(.text.unlikely) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.text.*) +#endif + *(.fixup) *(.gnu.warning) _etext = .; /* End of text section */ @@ -78,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) @@ -94,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 = .; @@ -207,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/boot/head.S b/xen/arch/x86/boot/head.S index dd1bea0d10..92d73345f0 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -9,7 +9,7 @@ #include <asm/cpufeature.h> #include <public/elfnote.h> - .text + .section .text.header, "ax", @progbits .code32 #define sym_offs(sym) ((sym) - __XEN_VIRT_START) diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 506bc8e404..75925fe145 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -73,9 +73,7 @@ SECTIONS _start = .; DECL_SECTION(.text) { _stext = .; /* Text and read-only data */ - *(.text) - *(.text.__x86_indirect_thunk_*) - *(.text.page_aligned) + *(.text.header) . = ALIGN(PAGE_SIZE); _stextentry = .; @@ -86,8 +84,16 @@ SECTIONS *(.text.kexec) /* Page aligned in the object file. */ kexec_reloc_end = .; - *(.text.cold) - *(.text.unlikely) + *(.text.cold .text.cold.*) + *(.text.unlikely .text.*_unlikely .text.unlikely.*) + + *(.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.text.*) +#endif + *(.text.__x86_indirect_thunk_*) + *(.text.page_aligned) + *(.fixup) *(.gnu.warning) _etext = .; /* End of text section */ @@ -289,9 +295,7 @@ SECTIONS DECL_SECTION(.data) { *(.data.page_aligned) - *(.data) - *(.data.rel) - *(.data.rel.*) + *(.data .data.*) } PHDR(text) DECL_SECTION(.bss) { @@ -306,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..202f0811d1 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -350,10 +350,14 @@ source "common/sched/Kconfig" config CRYPTO bool +config CC_SPLIT_SECTIONS + bool + 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. Note that placement of the sections inside of .text is also slightly adjusted to be more similar to the position found in the default GNU ld linker script. This requires having a separate section for the header in order to place it at the begging of the output image, followed with the unlikely and related sections, and finally the main .text section. On Arm the .data.read_mostly needs to be moved ahead of the .data section like it's already done on x86, and the alignment 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> --- 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. --- Given that now the header is explicitly placed in .text.header, it's likely possible to simplify some of the ordering of the object files for the prelink.o generation, as arch/x86/boot/built_in.o no longer needs to be the first object file in the list. It also 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/Makefile | 2 ++ xen/arch/arm/arm32/head.S | 1 + xen/arch/arm/arm64/head.S | 1 + xen/arch/arm/xen.lds.S | 49 +++++++++++++++++++++------------------ xen/arch/x86/boot/head.S | 2 +- xen/arch/x86/xen.lds.S | 22 +++++++++++------- xen/common/Kconfig | 4 ++++ 7 files changed, 49 insertions(+), 32 deletions(-)