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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Apr 19, 2023 at 10:43:22AM +0200, Jan Beulich wrote:
> 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.

Right, I would only introduce the ones above and as lonog as I have at
least one user for them. I don't think there's much value in importing
the file wholesale if we have no use case for a lot of the imported
macros.

The main issue with ENTRY() and ENDPROC() / ENDDATA() is that we still
need a tag for local function-like entry point labels, would you then
use PROC() for those? ENTRY_LOCAL()?

I have to admit I prefer the FUNC_START{_LOCAL} for that purpose as I
think it's clearer.  I would agree on dropping the SYM_ prefix from
the Linux ones if there's consensus.

I would ideally like to be able to make progress on this before the
XenSummit.  I think we all agree we want those annotations, being
blocked on the names of the helpers seems absurd.

Thanks, Roger.