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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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.

>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
>>>  
>>>          ALIGN
>>>  /* No special register assumptions. */
>>> -restore_all_xen:
>>> +START_LP(restore_all_xen)
>>>          /*
>>>           * Check whether we need to switch to the per-CPU page tables, in
>>>           * case we return to late PV exit code (from an NMI or #MC).
>>> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
>>>  
>>>          RESTORE_ALL adj=8
>>>          iretq
>>> +END_LP(restore_all_xen)
>>
>> While I'm fine with this conversion, ...
> 
> So I take that overall you would agree to adding this extra
> information using a pair of macros similar to the proposed ones.

Yes (with the above in mind, though).

Jan