Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote:
> On 26.02.2024 12:32, Roger Pau Monné wrote:
> > On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote:
> >> On 07.02.2024 15:55, Roger Pau Monne wrote:
> >>> The minimal function size requirements for an x86 livepatch are either 5 bytes
> >>> (for jmp) or 9 bytes (for endbr + jmp), 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 scripts, 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.
> >>
> >> Perhaps "... minimal required size"?
> > 
> > Yes.
> > 
> >>> --- a/xen/common/Kconfig
> >>> +++ b/xen/common/Kconfig
> >>> @@ -395,8 +395,11 @@ config CRYPTO
> >>>  config LIVEPATCH
> >>>  	bool "Live patching support"
> >>>  	default X86
> >>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> >>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
> >>>  	select CC_SPLIT_SECTIONS
> >>> +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
> >>> +	select FUNCTION_ALIGNMENT_8B  if X86
> >>> +	select FUNCTION_ALIGNMENT_4B  if ARM
> >>
> >> This isn't strictly needed, is it? Would be nice to avoid re-selection
> >> of what the default for an arch is anyway, as otherwise this will start
> >> looking clumsy when a couple more architectures are added.
> > 
> > My worry was that the default per-arch could change, ie: for example
> > x86 moving from 16 to 8 and then it would hamper livepatch support if
> > IBT is also enabled.  I however think it's very unlikely to reduce the
> > default alignment, and in any case we would hit a build time assert if
> > that ever happens.
> > 
> > So yes, I'm fine with dropping those.
> 
> Oh, no - not "those", only "that", i.e. only the last (Arm) one.

Oh, I see what you mean, even x86 selects the default one when IBT is
enabled, and when not the requirement for livepatch is < than the
default anyway.  That's why I said that we could even drop all of them
and just rely on the build time assert to catch any changes here.

Feel free to drop the ARM one.

Thanks, Roger.