Re: [RFC 5/5] xen/x86: livepatch: enable attaching callbacks

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

On Mon, 26 Oct 2026 11:42:37 +0000, Andrew Cooper wrote:
>On 23/10/2026 5:58 am, Ryo Takakura wrote:
>> diff --git a/xen/arch/x86/livepatch-asm.S b/xen/arch/x86/livepatch-asm.S
>> new file mode 100644
>> index 0000000000..005765b476
>> --- /dev/null
>> +++ b/xen/arch/x86/livepatch-asm.S
>> @@ -0,0 +1,60 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#include <asm/asm_defns.h>
>> +
>> +        .text
>> +
>
>You need ".file __FILE__" too.  IIRC this is a workaround for a binutils
>bug.

Understood. I'll add ".file __FILE__".

>> +/*
>> + * xen/arch/x86/livepatch-asm.S
>> + *
>> + * Trampoline for livepatch tracer.
>> + *
>> + * If livepatch tracer is enabled, function entries are placed
>> + * with five NOPs. arch_livepatch_apply_trace() will be livepatching
>> + * those NOPs to:
>> + *
>> + *     CALL      xen_livepatch_trace_caller
>> + *
>> + * 0(%rsp): traced function + 5
>> + * 8(%rsp): return address to the traced function's caller
>
>Somewhere in this comment needs to say that all GPRs need preserving.

Ok. I'll add a comment.

>Except...  Looking at arch_generic_hweightl(), my notes about
>__attribute__((no_caller_saved_registers)) say GCC >= 7 and 8 is already
>a minimum for the (broken) patchable function entry, let alone the fixed
>one.

I see.

>So I think you don't need an ASM trampoline at all.  I think you can
>merge xen_livepatch_trace_caller() and xen_livepatch_trace_dispatcher()
>and just tell the compiler to spill all ABI registers.

Yes, it would be nice to merge the two...
But xen_livepatch_trace_dispatcher() is also shared with arm64,
and GCC does not appear to support no_caller_saved_registers on arm64.
That makes arm64 still need its ASM trampoline to preserve
the registers and common C dispatcher.
Do you think rewriting livepatch_trace_caller() in C while keeping
livepatch_trace_dispatcher() as common dispatcher would be reasonable?

I'm also thinking of adding the attribute force_align_arg_pointer
to address the Bernhard's feedback on stack alignment:
https://lore.kernel.org/xen-devel/179285259082.1706038.15527286146285697701.b4-review@b4/

>Also, the xen_ prefixes are redundant.  Just name the function
>livepatch_trace_caller().

Ok. I'll remove them.

>> + */
>> +FUNC(xen_livepatch_trace_caller)
>> +        pushq   %rbp
>> +        movq    %rsp, %rbp
>> +        pushfq
>> +
>> +        /* Preserve general-purpose registers. */
>> +        pushq   %rax
>> +        pushq   %rcx
>> +        pushq   %rdx
>> +        pushq   %rsi
>> +        pushq   %rdi
>> +        pushq   %r8
>> +        pushq   %r9
>> +        pushq   %r10
>> +        pushq   %r11
>> +
>> +        /*
>> +         * xen_livepatch_trace_dispatcher(ip, parent_ip)
>> +         *
>> +         * ip: Address of the traced function.
>> +         * parent_ip: Address where the traced function was called.
>> +         */
>> +        movq    8(%rbp), %rdi
>> +        subq    $5, %rdi
>> +        movq    16(%rbp), %rsi
>> +        call    xen_livepatch_trace_dispatcher
>> +
>> +        /* Restore the saved state. */
>> +        popq    %r11
>> +        popq    %r10
>> +        popq    %r9
>> +        popq    %r8
>> +        popq    %rdi
>> +        popq    %rsi
>> +        popq    %rdx
>> +        popq    %rcx
>> +        popq    %rax
>> +        popfq
>> +        popq    %rbp
>> +        ret
>> +END(xen_livepatch_trace_caller)
>
>Just in case there's a problem doing this all in C...
>
>No need for pushf or popf at all.  Flags are undefined at function
>boundaries.
>
>Please drop the q suffixes; they're all unnecessary here.
>
>The ret needs to be RET for speculative mitigations.  (It's emitted as
>JMP which will be turned back into a RET on safe system.)

Understood.
I'll get them fixed in case if we need a trampoline.

>> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
>> index 5158e91f7e..1dfcc3e750 100644
>> --- a/xen/arch/x86/livepatch.c
>> +++ b/xen/arch/x86/livepatch.c
>> @@ -19,6 +19,10 @@
>>  #include <asm/nmi.h>
>>  #include <asm/setup.h>
>>  
>> +#ifdef CONFIG_X86_PATCHABLE_FUNCTION_ENTRY
>> +extern void xen_livepatch_trace_caller(void);
>> +#endif
>
>You don't need ifdefary around declarations.

Ok. I'll remove them.

>> +
>>  static bool has_active_waitqueue(const struct vm_event_domain *ved)
>>  {
>>      /* ved may be xzalloc()'d without INIT_LIST_HEAD() yet. */
>> @@ -365,6 +369,35 @@ int arch_livepatch_secure(const void *va, unsigned int 
>> pages, enum va_type type)
>>      return modify_xen_mappings(start, start + pages * PAGE_SIZE, flag);
>>  }
>>  
>> +#ifdef CONFIG_X86_PATCHABLE_FUNCTION_ENTRY
>> +unsigned long arch_livepatch_trace_entry(unsigned long addr)
>> +{
>> +    if ( (is_endbr64((const void *)addr) || is_endbr64_poison((const void 
>> *)addr)) )
>> +        addr += ENDBR64_LEN;
>> +
>> +    return addr;
>> +}
>> +
>> +void noinline arch_livepatch_apply_trace(const struct livepatch_func *trace)
>> +{
>> +    void *site = (void *)arch_livepatch_trace_entry((unsigned 
>> long)trace->old_addr);
>> +    const void *new_addr = xen_livepatch_trace_caller;
>> +    uint8_t insn[ARCH_PATCH_INSN_SIZE];
>> +    int32_t val = new_addr - (site + ARCH_PATCH_INSN_SIZE);
>> +
>> +    insn[0] = 0xe8; /* CALL rel32. */
>> +    memcpy(&insn[1], &val, sizeof(val));
>> +    memcpy(site, insn, sizeof(insn));
>> +}
>
>Why noinline?  These are out-of-line anyway by being cross-TU, but in a
>theoretical world where we had working LTO, I don't see any problem with
>them being inlined.

I see. Thanks for explaining.
Here I added noinline following arch_livepatch_apply().
I'll remove them.

>> +
>> +void noinline arch_livepatch_revert_trace(const struct livepatch_func 
>> *trace)
>> +{
>> +    void *site = (void *)arch_livepatch_trace_entry((unsigned 
>> long)trace->old_addr);
>> +
>> +    memset(site, 0x90, ARCH_PATCH_INSN_SIZE);
>> +}
>> +#endif
>
>This wants to be a 5 byte NOP, not 5x 1-byte NOPs.
>
>I see that GCC is still doing the slow thing.  Clang is doing it's code
>generation more sensibly.
>
>Use the add_nops() function.

Understood. I'll use add_nops() in the next version.

Sincerely,
Ryo Takakura