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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2023 17:57, Roger Pau Monné wrote:
> 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?

I wouldn't mind doing so, as long as there was at least a vague chance
that this also comes somewhat close to meet Andrew's expectations.
Andrew?

Jan