Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

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

On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
> On 28.11.2023 11:03, Roger Pau Monne wrote:
> > The minimal function size requirements for livepatch are either 5 bytes (for
> > jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at 
> > least
> > that size by requesting the compiled to align the functions to 8 or 16 
> > bytes,
> > depending on whether Xen is build with IBT support.
> 
> How is alignment going to enforce minimum function size? If a function is
> last in a section, there may not be any padding added (ahead of linking at
> least). The trailing padding also isn't part of the function.

If each function lives in it's own section (by using
-ffunction-sections), and each section is aligned, then I think we can
guarantee that there will always be enough padding space?

Even the last function/section on the .text block would still be
aligned, and as long as the function alignment <= SECTION_ALIGN
there will be enough padding left.  I should add some build time
assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.

While the trailing padding is not part of the function itself, I don't
see issues in overwriting it with the replacement function code.

> > 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.
> > 
> > Since the option (-falign-functions) is supported by both minimal required
> > compiler versions of clang and gcc there's no need to add a test to check 
> > for
> > its presence.
> > 
> > The alignment is currently only implemented for livepatch on x86, I'm unsure
> > whether ARM has a mandatory function alignment high enough to cover for the
> > space required by the replacement instruction(s).
> 
> Indeed I was wondering whether this shouldn't be generalized, if indeed
> required.

Well, more than required it's nice to have in order to avoid failures.
The checks in the livepatch code will trigger if there's not enough
space to apply the replacement code.

Thanks, Roger.