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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> +/*
> + * 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.

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.

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.

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

> + */
> +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.)

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

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

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

~Andrew