On Tue, Dec 19, 2023 at 07:46:11PM +0000, Andrew Cooper wrote: > On 15/12/2023 11:18 am, Roger Pau Monne wrote: > > The minimal function size requirements for livepatch are either 5 bytes (for > > "for an x86 livepatch", seeing as we're touching multiple architectures > worth of files. > > I know it's at the end of the sentence, but it wants to be earlier to be > clearer. > > > jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm. Ensure > > that distance between functions entry points is always at least of the > > minimal > > required size for livepatch instruction replacement to be successful. > > > > Add an additional align directive to the linker script, in order to ensure > > that > > the next section placed after the .text.* (per-function sections) is also > > aligned to the required boundary, so that the distance of the last function > > entry point with the next symbol is also of minimal size. > > > > Note that it's possible for the compiler to end up using a higher function > > alignment regardless of the passed value, so this change just make sure that > > the minimum required for livepatch to work is present. Different compilers > > handle the option differently, as clang will ignore -falign-functions value > > if it's smaller than the one that would be set by the optimization level, > > while > > gcc seems to always honor the function alignment passed in > > -falign-functions. > > In order to cope with this behavior and avoid that setting -falign-functions > > results in an alignment inferior to what the optimization level would have > > selected force x86 release builds to use a function alignment of 16 bytes. > > Yuck :( > > The same will be true for all other architectures too?
I would expect that for gcc I guess. > What happens on ARM, which also picks up an explicit choice in livepatch > builds? Arm AFAICT seems to use a 4 byte function alignment with -O2 (both gcc and clang), so that matches what we need to enforce for livepatch. If we ever need a higher alignment for livepatch reasons it would be a multiple of the minimum one set by the compiler, so that should be fine. > > > > The compiler option -falign-functions is not available on at least clang > > 3.8, > > so introduce a Kconfig check for it and make the livepatch option depend on > > the > > compiler supporting the option. > > > > The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not > > mentioning CC in preparation for the option also being used by assembly > > code. > > > > Signed-off-by: Roger Pau Monné <[email protected]> > > --- > > Changes since v3: > > - Test for compiler option with -falign-functions. > > - Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT. > > - Set 16byte function alignment for x86 release builds. > > > > Changes since v2: > > - Add Arm side. > > - Align end of section in the linker script to ensure enough padding for > > the > > last function. > > - Expand commit message and subject. > > - Rework Kconfig options. > > - Check that the compiler supports the option. > > > > Changes since v1: > > - New in this version. > > --- > > xen/Kconfig | 19 +++++++++++++++++++ > > xen/Makefile | 3 +++ > > xen/arch/arm/livepatch.c | 2 ++ > > xen/arch/arm/xen.lds.S | 4 ++++ > > xen/arch/x86/Kconfig | 1 + > > xen/arch/x86/livepatch.c | 4 ++++ > > xen/arch/x86/xen.lds.S | 4 ++++ > > xen/common/Kconfig | 5 ++++- > > 8 files changed, 41 insertions(+), 1 deletion(-) > > xen$ git ls-files | grep xen.lds.S > arch/arm/xen.lds.S > arch/ppc/xen.lds.S > arch/riscv/xen.lds.S > arch/x86/xen.lds.S > > RISC-V and PPC have the same pattern that you're patching for x86 and ARM. I've avoided touching those because there's no livepatch support there (yet), and I didn't want to give the impression that the option is supported or tested for those architectures. I have no idea what function alignments would be sensible for riscv or ppc. > > diff --git a/xen/Kconfig b/xen/Kconfig > > index 134e6e68ad84..c2cc3fe165eb 100644 > > --- a/xen/Kconfig > > +++ b/xen/Kconfig > > @@ -37,6 +37,25 @@ config CC_HAS_VISIBILITY_ATTRIBUTE > > config CC_SPLIT_SECTIONS > > bool > > > > +# Set function alignment. > > +# > > +# Allow setting on a boolean basis, and then convert such selection to an > > +# integer for the build system and code to consume more easily. > > # Clang >= 6.0 > > > +config CC_HAS_FUNCTION_ALIGNMENT > > + def_bool $(cc-option,-falign-functions) > > +config FUNCTION_ALIGNMENT_4B > > + bool > > +config FUNCTION_ALIGNMENT_8B > > + bool > > +config FUNCTION_ALIGNMENT_16B > > + bool > > +config FUNCTION_ALIGNMENT > > + int > > + depends on CC_HAS_FUNCTION_ALIGNMENT > > + default 16 if FUNCTION_ALIGNMENT_16B > > + default 8 if FUNCTION_ALIGNMENT_8B > > + default 4 if FUNCTION_ALIGNMENT_4B > > What value do we get here for RISCV/PPC? Do we need another override > for them? Hm, I wasn't planning on adding support for PPC/RISCV here, if those arches want to use a specific function alignment they might need to adjust the options here, I think that's a reasonable compromise, as I don't see a need for this to be blocked on also agreeing values for ppc or riscv. Thanks, Roger.
