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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)

Jan