Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich <jbeulich@xxxxxxxx>]

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