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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

What happens on ARM, which also picks up an explicit choice in livepatch
builds?

>
> 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é <roger.pau@xxxxxxxxxx>
> ---
> 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.


> 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?

~Andrew