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

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

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?

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

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

Sure, thanks for the feedback.

Roger.