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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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é <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.

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.