Re: [PATCH] x86/livepatch: enable livepatching assembly source files

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2023 10:25, Roger Pau Monné wrote:
> On Wed, Apr 19, 2023 at 08:17:45AM +0200, Jan Beulich wrote:
>> On 18.04.2023 15:06, Roger Pau Monné wrote:
>>> On Tue, Apr 18, 2023 at 01:00:53PM +0200, Jan Beulich wrote:
>>>> On 18.04.2023 11:24, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/include/asm/config.h
>>>>> +++ b/xen/arch/x86/include/asm/config.h
>>>>> @@ -44,6 +44,20 @@
>>>>>  /* Linkage for x86 */
>>>>>  #ifdef __ASSEMBLY__
>>>>>  #define ALIGN .align 16,0x90
>>>>> +#ifdef CONFIG_LIVEPATCH
>>>>> +#define START_LP(name)                          \
>>>>> +  jmp name;                                     \
>>>>> +  .pushsection .text.name, "ax", @progbits;     \
>>>>> +  name:
>>>>> +#define END_LP(name)                            \
>>>>> +  .size name, . - name;                         \
>>>>> +  .type name, @function;                        \
>>>>> +  .popsection
>>>>> +#else
>>>>> +#define START_LP(name)                          \
>>>>> +  name:
>>>>> +#define END_LP(name)
>>>>> +#endif
>>>>>  #define ENTRY(name)                             \
>>>>>    .globl name;                                  \
>>>>>    ALIGN;                                        \
>>>>
>>>> Couldn't END_LP() set type and size unconditionally? (But see also
>>>> below.)
>>>
>>> I see, so that we could also use it for debug purposes.  I guess at
>>> that point it might be better to use {START,END}_FUNC() to note that
>>> the macros also have an effect beyond that of livepatching.
>>>
>>> Maybe also introduce a START_ENTRY() that replaces ENTRY()?  Albeit I
>>> find START_ENTRY a weird name.
>>
>> So do I. {START,END}_FUNC() or whatever else are in principle fine, but
>> I take it that you're aware that we meanwhile have two or even three
>> concurring proposals on a general scheme of such annotations, and we
>> don't seem to be able to agree on one. (I guess I'll make a design
>> session proposal on this topic for Prague.)
> 
> Oh, I wasn't aware we had other proposals, I've been away on an off
> quite a lot recently, and haven't been able to keep up with all
> xen-devel email.  Do you have any references at hand?

Andrew said he had posted something long ago, but I didn't recall and
hence have no reference. My posting from about a year ago is
https://lists.xen.org/archives/html/xen-devel/2022-04/msg00876.html
Subsequently Jane went kind of the Linux route:
https://lists.xen.org/archives/html/xen-devel/2022-08/msg00236.html

>> One thing needs to be clear though: Macros doing things solely needed
>> for LP need to not have extra effects with it disabled, and such
>> macros also better wouldn't e.g. insert stray JMP when not really
>> needed. Hence I expect we still want (some) LP-specific macros besides
>> whatever we settle on as the generic ones.
> 
> The stray jmp can be inserted only in the livepatch case, if we end up
> having to add it.
> 
> Maybe we should just go with Linux names, so initially I would like to
> use:
> 
> SYM_FUNC_START{_NOALIGN}(name)
> SYM_FUNC_START_LOCAL{_NOALIGN}(name)
> SYM_FUNC_END(name)

As said in replies on the earlier threads, I think these are overly
verbose and come in overly many variations.

Jan