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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan

> Feel free to drop the ARM one.
> 
> Thanks, Roger.