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

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

[Roger Pau Monne]

In order to be able to livepatch code from assembly files we need:

 * Proper function symbols from assembly code, including the size.
 * Separate sections for each function.

However assembly code doesn't really have the concept of a function,
and hence the code tends to chain different labels that can also be
entry points.

In order to be able to livepatch such code we need to enclose the
assembly code into isolated function-like blocks, so they can be
handled by livepatch.  Introduce two new macros to do so,
{START,END}_LP() that take a unique function like name, create the
function symbol and put the code into a separate text section.  Note
that START_LP() requires a preceding jump before the section change,
so that any preceding code that fallthrough correctly continues
execution, as sections can be reordered.  Chaining of consecutive
livepatchable blocks will also require that the previous section
jumps into the next one if required.

A couple of shortcomings:

 * We don't check that the size of the section is enough to fit a jump
   instruction (ARCH_PATCH_INSN_SIZE).  Some logic from the
   alternatives framework should be used to pad sections if required.
 * Any labels inside of a {START,END}_LP() section must not be
   referenced from another section, as the patching would break those.
   I haven't figured out a way to detect such references.  We
   already use .L to denote local labels, but we would have to be
   careful.

Some of the assembly entry points cannot be safely patched until it's
safe to use jmp, as livepatch can replace a whole block with a jmp to
a new address, and that won't be safe until speculative mitigations
have been applied.

I could also look into allowing livepatch of sections where jmp
replacement is not safe by requesting in-place code replacement only,
we could then maybe allow adding some nop padding to those sections in
order to cope with the size increasing in further livepatches.

So far this patch only contains two switched functions:
restore_all_xen and common_interrupt.  I don't really want to switch
more code until we agree on the approach, so take this as a kind of
RFC patch.  Obviously conversion doesn't need to be done in one go,
neither all assembly code need to be 'transformed' in this way.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/config.h | 14 ++++++++++++++
 xen/arch/x86/x86_64/entry.S       |  5 ++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index fbc4bb3416bd..68e7fdfe3517 100644
--- 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;                                        \
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 7675a59ff057..c204634910c4 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -660,7 +660,7 @@ ENTRY(early_page_fault)
 
         ALIGN
 /* No special register assumptions. */
-restore_all_xen:
+START_LP(restore_all_xen)
         /*
          * Check whether we need to switch to the per-CPU page tables, in
          * case we return to late PV exit code (from an NMI or #MC).
@@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
 
         RESTORE_ALL adj=8
         iretq
+END_LP(restore_all_xen)
 
 ENTRY(common_interrupt)
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
@@ -687,6 +688,7 @@ ENTRY(common_interrupt)
         SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: acd */
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
+START_LP(common_interrupt_lp)
         mov   STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
         mov   STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl
         mov   %rcx, %r15
@@ -707,6 +709,7 @@ ENTRY(common_interrupt)
         mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
         mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
         jmp ret_from_intr
+END_LP(common_interrupt_lp)
 
 ENTRY(page_fault)
         ENDBR64
-- 
2.40.0

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

[Jan Beulich]

On 18.04.2023 11:24, Roger Pau Monne wrote:
> Some of the assembly entry points cannot be safely patched until it's
> safe to use jmp, as livepatch can replace a whole block with a jmp to
> a new address, and that won't be safe until speculative mitigations
> have been applied.

Isn't the issue only with indirect JMP, whereas livepatch uses only
direct ones?

> --- 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;     \

In how far is livepatch susceptible to two .text.* sections of the same
name? This can result here and perhaps also for static C functions.

> +  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;                                        \

Do these really need to go into config.h, instead of e.g. asm_defns.h?
I'd prefer if stuff like this was moved out of here, rather than more
things accumulating. (Perhaps these also would better be assembler
macros, in which case asm-defns.h might be the place to put them, but I
guess that's fairly much a matter of taste.)

Couldn't END_LP() set type and size unconditionally? (But see also
below.)

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
>  
>          ALIGN
>  /* No special register assumptions. */
> -restore_all_xen:
> +START_LP(restore_all_xen)
>          /*
>           * Check whether we need to switch to the per-CPU page tables, in
>           * case we return to late PV exit code (from an NMI or #MC).
> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
>  
>          RESTORE_ALL adj=8
>          iretq
> +END_LP(restore_all_xen)

While I'm fine with this conversion, ...

>  ENTRY(common_interrupt)
>          ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
> @@ -687,6 +688,7 @@ ENTRY(common_interrupt)
>          SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: acd */
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
> +START_LP(common_interrupt_lp)
>          mov   STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
>          mov   STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl
>          mov   %rcx, %r15
> @@ -707,6 +709,7 @@ ENTRY(common_interrupt)
>          mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
>          mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
>          jmp ret_from_intr
> +END_LP(common_interrupt_lp)

... this one's odd, as it doesn't cover the entire "function". How would
you envision we sensibly add ELF metadata also for common_interrupt?

Jan

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

[Andrew Cooper]

On 18/04/2023 12:00 pm, Jan Beulich wrote:
> On 18.04.2023 11:24, Roger Pau Monne wrote:
>> Some of the assembly entry points cannot be safely patched until it's
>> safe to use jmp, as livepatch can replace a whole block with a jmp to
>> a new address, and that won't be safe until speculative mitigations
>> have been applied.
> Isn't the issue only with indirect JMP, whereas livepatch uses only
> direct ones?

We already have direct jumps prior to speculation safety logic. 
Livepatching putting more in doesn't change our safety.

>> --- 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;     \
> In how far is livepatch susceptible to two .text.* sections of the same
> name? This can result here and perhaps also for static C functions.

Well - the section is the unit of binary-diffing noticing a difference.

If we have a naming collision here, then I expect the linker will merge
the two section, and the livepatch will end up bigger than it strictly
needs to be.

~Andrew

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

[Roger Pau Monné]

On Tue, Apr 18, 2023 at 01:00:53PM +0200, Jan Beulich wrote:
> On 18.04.2023 11:24, Roger Pau Monne wrote:
> > Some of the assembly entry points cannot be safely patched until it's
> > safe to use jmp, as livepatch can replace a whole block with a jmp to
> > a new address, and that won't be safe until speculative mitigations
> > have been applied.
> 
> Isn't the issue only with indirect JMP, whereas livepatch uses only
> direct ones?

Oh, I see, livepatch uses a relative JMP, so that's not affected.

> > --- 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;     \
> 
> In how far is livepatch susceptible to two .text.* sections of the same
> name? This can result here and perhaps also for static C functions.

This is all fine, as long as they are not in the same translation
unit.  Livepatch creation operates against object files, so as long as
there are no section names clashes in a translation unit it should be
able to deal with it.

> > +  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;                                        \
> 
> Do these really need to go into config.h, instead of e.g. asm_defns.h?

I've just put them together to the ENTRY() macros, but yes, I see no
reason to not move them to asm{_,-}defns.h.

> I'd prefer if stuff like this was moved out of here, rather than more
> things accumulating. (Perhaps these also would better be assembler
> macros, in which case asm-defns.h might be the place to put them, but I
> guess that's fairly much a matter of taste.)
> 
> 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.

> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
> >  
> >          ALIGN
> >  /* No special register assumptions. */
> > -restore_all_xen:
> > +START_LP(restore_all_xen)
> >          /*
> >           * Check whether we need to switch to the per-CPU page tables, in
> >           * case we return to late PV exit code (from an NMI or #MC).
> > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
> >  
> >          RESTORE_ALL adj=8
> >          iretq
> > +END_LP(restore_all_xen)
> 
> While I'm fine with this conversion, ...

So I take that overall you would agree to adding this extra
information using a pair of macros similar to the proposed ones.

> >  ENTRY(common_interrupt)
> >          ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
> > @@ -687,6 +688,7 @@ ENTRY(common_interrupt)
> >          SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: acd */
> >          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> >  
> > +START_LP(common_interrupt_lp)
> >          mov   STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
> >          mov   STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl
> >          mov   %rcx, %r15
> > @@ -707,6 +709,7 @@ ENTRY(common_interrupt)
> >          mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> >          mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
> >          jmp ret_from_intr
> > +END_LP(common_interrupt_lp)
> 
> ... this one's odd, as it doesn't cover the entire "function". How would
> you envision we sensibly add ELF metadata also for common_interrupt?

That was done as to avoid patching the first part of the function that
does the speculative mitigations, but since the jmp introduced by
livepatch is a relative one we are safe and could indeed patch the
whole function.

Thanks, Roger.

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

[Jan Beulich]

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

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.

>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
>>>  
>>>          ALIGN
>>>  /* No special register assumptions. */
>>> -restore_all_xen:
>>> +START_LP(restore_all_xen)
>>>          /*
>>>           * Check whether we need to switch to the per-CPU page tables, in
>>>           * case we return to late PV exit code (from an NMI or #MC).
>>> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
>>>  
>>>          RESTORE_ALL adj=8
>>>          iretq
>>> +END_LP(restore_all_xen)
>>
>> While I'm fine with this conversion, ...
> 
> So I take that overall you would agree to adding this extra
> information using a pair of macros similar to the proposed ones.

Yes (with the above in mind, though).

Jan

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

[Roger Pau Monné]

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?

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

> >>> --- a/xen/arch/x86/x86_64/entry.S
> >>> +++ b/xen/arch/x86/x86_64/entry.S
> >>> @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
> >>>  
> >>>          ALIGN
> >>>  /* No special register assumptions. */
> >>> -restore_all_xen:
> >>> +START_LP(restore_all_xen)
> >>>          /*
> >>>           * Check whether we need to switch to the per-CPU page tables, in
> >>>           * case we return to late PV exit code (from an NMI or #MC).
> >>> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
> >>>  
> >>>          RESTORE_ALL adj=8
> >>>          iretq
> >>> +END_LP(restore_all_xen)
> >>
> >> While I'm fine with this conversion, ...
> > 
> > So I take that overall you would agree to adding this extra
> > information using a pair of macros similar to the proposed ones.
> 
> Yes (with the above in mind, though).

Sure, thanks for the feedback.

Roger.

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

[Jan Beulich]

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.

Jan

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

[Roger Pau Monné]

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.

I would ideally like to be able to make progress on this before the
XenSummit.  I think we all agree we want those annotations, being
blocked on the names of the helpers seems absurd.

Thanks, Roger.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Andrew Cooper]

On 18/04/2023 10:24 am, Roger Pau Monne wrote:

> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 7675a59ff057..c204634910c4 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
>  
>          ALIGN
>  /* No special register assumptions. */
> -restore_all_xen:
> +START_LP(restore_all_xen)
>          /*
>           * Check whether we need to switch to the per-CPU page tables, in
>           * case we return to late PV exit code (from an NMI or #MC).
> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
>  
>          RESTORE_ALL adj=8
>          iretq
> +END_LP(restore_all_xen)


While it's useful to have a concrete idea of what is necessary to fix
all of this, I do not wish to put in markers like this.  This isn't
about livepatching - it's about getting sane ELF metadata.

This is why I had Jane work on using the Linux macros.  They account for
*all* interesting ELF metadata, as well as taking care of things like
the global function alignment settings, CFI patching space, etc.

Putting functions in separate sections should be hidden in the normal
SYM_FUNC_START(), and dependent on CONFIG_SPLIT_SECTIONS behind the
scenes seeing as we have that as a option already.

~Andrew

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

[Roger Pau Monné]

On Tue, Apr 18, 2023 at 01:17:55PM +0100, Andrew Cooper wrote:
> On 18/04/2023 10:24 am, Roger Pau Monne wrote:
> 
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index 7675a59ff057..c204634910c4 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
> >  
> >          ALIGN
> >  /* No special register assumptions. */
> > -restore_all_xen:
> > +START_LP(restore_all_xen)
> >          /*
> >           * Check whether we need to switch to the per-CPU page tables, in
> >           * case we return to late PV exit code (from an NMI or #MC).
> > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
> >  
> >          RESTORE_ALL adj=8
> >          iretq
> > +END_LP(restore_all_xen)
> 
> 
> While it's useful to have a concrete idea of what is necessary to fix
> all of this, I do not wish to put in markers like this.  This isn't
> about livepatching - it's about getting sane ELF metadata.

Right, Jan has expressed a similar opinion.

> This is why I had Jane work on using the Linux macros.  They account for
> *all* interesting ELF metadata, as well as taking care of things like
> the global function alignment settings, CFI patching space, etc.

I don't see much issue in using the Linux macros, if we agree we want
those.  Some of those would likely be unused, do we want to import it
wholesale, or just introduce the ones required?  Initially I might
just introduce SYM_FUNC_START{,_LOCAL}() and SYM_FUNC_END().

> Putting functions in separate sections should be hidden in the normal
> SYM_FUNC_START(), and dependent on CONFIG_SPLIT_SECTIONS behind the
> scenes seeing as we have that as a option already.

Sure.

Thanks, Roger.