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

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

On Tue, Feb 27, 2024 at 09:53:24AM +0100, Jan Beulich wrote:
> On 27.02.2024 09:15, Roger Pau Monné wrote:
> > 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.
> 
> Just to clarify: The default I mean is the architecture imposed one.
> Leaving aside Thumb mode, Arm instructions are all 32-bit words, and
> hence less than 4-byte alignment makes no sense (and may even be
> disallowed by the architecture). Whereas for x86 what you're talking
> about is just a compiler default, which isn't really guaranteed to
> never be lower (with -Os for example I'd expect it to be perhaps as
> low as 1).

Right, it's a compiler default, but in patch 1 we already set the
default alignment for x86 to be 16 bytes.

When in your first comment you mentioned "... default for an arch is
anyway" I assumed you mean the default in the arch Kconfig file, not
what the ISA mandates.

Thanks, Roger.