On 14.12.2023 11:17, Roger Pau Monne wrote: > The minimal function size requirements for livepatch are either 5 bytes (for > 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.
That's a possibility which we don't need to be concerned about. Yet isn't it also possible that we override a larger, deemed better (e.g. performance-wise) value? I'm somewhat concerned of that case. IOW is -falign-functions= really the right option to use here? (There may not be one which we would actually prefer to use.) Specifically -falign-functions (without a value, i.e. using a machine dependent default) is implied by -O2 and -O3, as per 13.2 gcc doc. > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -37,6 +37,24 @@ 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. > +config CC_HAS_FUNCTION_ALIGNMENT > + def_bool $(cc-option,-falign-functions=8) How does this check make sure 4- or 16-byte alignment are also accepted as valid? (Requesting 8-byte alignment would be at least bogus on e.g. IA-64.) > +config FUNCTION_ALIGNMENT_4B > + bool > +config FUNCTION_ALIGNMENT_8B > + bool > +config FUNCTION_ALIGNMENT_16B > + bool > +config FUNCTION_ALIGNMENT > + int > + default 16 if FUNCTION_ALIGNMENT_16B > + default 8 if FUNCTION_ALIGNMENT_8B > + default 4 if FUNCTION_ALIGNMENT_4B While for the immediate purpose here no "depends on CC_HAS_FUNCTION_ALIGNMENT" is okay, I still wonder if it wouldn't better be added in case further "select"s appear. > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer > endif > > CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections > +ifdef CONFIG_FUNCTION_ALIGNMENT ... e.g. this meaningless? Or is the lack of a default value leading to the option remaining undefined (rather than assigning some "default" default, e.g. 0)? > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -44,6 +44,10 @@ SECTIONS > #ifdef CONFIG_CC_SPLIT_SECTIONS > *(.text.*) > #endif > +#ifdef CONFIG_FUNCTION_ALIGNMENT > + /* Ensure enough distance with the next placed section. */ > + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); > +#endif > > *(.fixup) Seeing .fixup nicely in context - can't that (and other specialized sections containing code) also be patched? Jan
