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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 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.

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.

Jan