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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.12.2023 12:31, Roger Pau Monné wrote:
> On Fri, Dec 01, 2023 at 11:59:09AM +0100, Jan Beulich wrote:
>> On 01.12.2023 11:21, Roger Pau Monné wrote:
>>> On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote:
>>>> On 01.12.2023 09:50, Roger Pau Monné wrote:
>>>>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
>>>>>> On 30.11.2023 18:37, Roger Pau Monné wrote:
>>>>>>> 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.
>>>>>>
>>>>>> I'm not sure of there being a requirement for a section to be padded to
>>>>>> its alignment. If the following section has smaller alignment, it could
>>>>>> be made start earlier. Of course our linker scripts might guarantee
>>>>>> this ...
>>>>>
>>>>> I do think so, given our linker script arrangements for the .text
>>>>> section:
>>>>>
>>>>> DECL_SECTION(.text) {
>>>>>     [...]
>>>>> } PHDR(text) = 0x9090
>>>>>
>>>>> . = ALIGN(SECTION_ALIGN);
>>>>>
>>>>> The end of the text section is aligned to SECTION_ALIGN, so as long as
>>>>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
>>>>> guarantee a minimal function size.
>>>>>
>>>>> Do you think it would be clearer if I add the following paragraph:
>>>>>
>>>>> "Given the Xen linker script arrangement of the .text section, we can
>>>>> ensure that when all functions are aligned to the given boundary the
>>>>> function size will always be a multiple of such alignment, even for
>>>>> the last function in .text, as the linker script aligns the end of the
>>>>> section to SECTION_ALIGN."
>>>>
>>>> I think this would be useful to have there. Beyond that, assembly code
>>>> also needs considering btw.
>>>
>>> Assembly will get dealt with once we start to also have separate
>>> sections for each assembly function.  We cannot patch assembly code at
>>> the moment anyway, due to lack of debug symbols.
>>
>> Well, yes, that's one part of it. The other is that some .text coming
>> from an assembly source may follow one coming from some C source, and
>> if the assembly one then isn't properly aligned, padding space again
>> wouldn't necessarily be large enough. This may be alright now (where
>> .text is the only thing that can come from .S and would be linked
>> ahead of all .text.*, being the only thing that can come from .c),
> 
> What about adding:
> 
> #ifdef CONFIG_CC_SPLIT_SECTIONS
>        *(.text.*)
> #endif
> #ifdef CONFIG_CC_FUNCTION_ALIGNMENT
>        /* Ensure enough padding regardless of next section alignment. */
>        . = ALIGN(CONFIG_CC_FUNCTION_ALIGNMENT)
> #endif
> 
> In order to assert that the end of .text.* is also aligned?

Probably.

>> but
>> it might subtly when assembly code is also switched to per-function
>> sections (you may recall that a patch to this effect is already
>> pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly
>> functions").
> 
> Yes, I think such patch should also honor the required alignment
> specified in CONFIG_CC_FUNCTION_ALIGNMENT.

I've added a note for myself to that patch, to adjust once yours has
landed (which given the state of my series is likely going to be much
earlier).

Jan