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

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

On Wed, Apr 19, 2023 at 04:39:01PM +0200, Jan Beulich wrote:
> On 19.04.2023 15:36, Roger Pau Monné wrote:
> > 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).
> 
> Yes. ENTRY() may not go away immediately, but I'd expect it to be
> phased out.
> 
> >> 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.
> 
> Well, yes, I was expecting this to be the case. Hence why I said I could
> live with _LOCAL suffixes, even if they aren't my preference. What we
> may want to keep in mind is that sooner or later we may want to have
> non-aligning variants of these. That'll again make for larger names,
> unless we went with e.g. an optional 2nd parameter which, if absent,
> means default alignment, while if present it would specify the alignment
> (which then can be used to effectively specify no alignment). E.g.
> 
> #define ALIGN(algn...) .balign algn
> 
> #define GLOBAL(name)                \
>     .globl name;                    \
>     .hidden name;                   \
>     name:
> 
> #define FUNC(name, algn...)         \
>     ALIGN(LAST(16, ## algn), 0x90); \
>     GLOBAL(name);                   \
>     .type name, @function
> 
> with these helpers (and count_args() as we already have it), or ideally
> something yet more simple (which right now I can't seem to be able to
> think of):
> 
> #define ARG1_(x, y...) (x)
> #define ARG2_(x, y...) (y)
> 
> #define LAST__(nr) ARG ## nr ## _
> #define LAST_(nr)  LAST__(nr)
> #define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y)

Would seem acceptable to me.  Would you like to make a proposal
(likely updating your previous patch) along this lines?

Thanks, Roger.