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

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

On Wed, Apr 19, 2023 at 02:00:38PM +0200, Jan Beulich wrote:
> On 19.04.2023 13:44, Roger Pau Monné wrote:
> > 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.
> 
> Okay, I'm glad we can agree on no SYM_. But what value does START have?
> And why would the type be (re)specified via ..._END()? FUNC(), DATA(),
> and END() ought to be all we need.

Does it imply that we would then drop ENTRY()? (seems so, would just
like to confirm).

> The type would be set by the entry
> point macros, and the size by END(). To cover local vs global I could
> live with _LOCAL suffixes, but personally would prefer e.g. LFUNC()
> and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and
> have (non-)global expressed by END() and e.g. LEND() or END_LOCAL().
> One less macro, but maybe slightly odd to have the .global directives
> then at the end rather than at the beginning.

Hm, yes, I do find it odd to have the .global at the end.  FUNC and
FUNC_LOCAL would be my preference, I do find {L,G}FUNC a bit
confusing.

> 
> Note that this is different from my proposed patch, where I aimed at
> the change being unintrusive. This includes that this was matching
> what Arm has (and hence required no change there at all). I think it
> would certainly be nice if these constructs were as similar as
> possible between arch-es; some may even be possible to share.

Well, yes, that would seem desirable as long as we can agree on a set
of helper names.

Thanks, Roger.