Re: [RFC] xen/arm64: livepatch: enable attaching callbacks

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

Hi Roger and Andrew,

On Tue, 30 Jun 2026 08:47:14 +0000, Roger Pau Monné wrote:
>On Mon, Jun 29, 2026 at 11:01:28AM +0900, Ryo Takakura wrote:
>> Linux ftrace allows registering callbacks which is useful
>> for debugging and tracing events. On Linux, it is done by
>> reserving function entry points at compile time which can
>> later be patched to branch to a trampoline.
>> 
>> This patch implements similar callback feature, but with
>> different approach using existing livepatch infrastructure.
>> Instead of reserving function entry points at compile time,
>> the traced function will be livepatched so that it branches
>> to the trampoline.
>
>While this is an interesting usage of the livepatch logic in new ways,
>may I ask why not do as Linux and add an empty function preamble that
>can be replaced at run-time with calls to hooks?
>
>You could still re-use most of the livepatch logic for handling the
>addition of the hook calls, but it would be nicer in that we won't
>need to move the original function.

I agree that we can reuse a lot of livepatch logic.

>> The role of the trampoline(illustrated below) is to preserve
>> the context while jumping to the tracer function, and return
>> back to the traced function with its context restored.
>
>Alternatively - why not use livepatch-build-tools against a build with
>the added hooks to generate a proper livepatch?  This looks a bit
>fragile to me (see the question from Andrew about fixing up
>instruction pointer relative references).

Yes, I think proper livepatching would still be preferred
given all the concerns Andrew and Roger raised.

>On x86 at least we would also need to adjust the bug frames and
>exception table contents, and the contents of the symbol table to
>account for the function being moved.
>
>IOW: it looks like overall this is a lot more work than possibly
>reserving a function preamble to add hook calls?

Yes, I agree.
(I wasn't aware of this additional work when I replied
to Andrew yesterday, thanks!)

And if I were to summarize the discussion so far, assuming we
still want to add a tracing-feature, I think we are in agreement
adding an empty function preamble like Linux.
(I personally would still like to see a framework on Xen
that is more convenient and tracing-friendly which can be used
reliably at the same time)

If this sounds reasonable, I will try preparing one based on
the feedbacks I was given so far:
- Use of Linux-like reserved function preamble
- Use of __attribute__((no_caller_saved_registers))
- Split replacement and preamble-hook handling in common code

Let me know your thoughts!

>> trampoline:
>>     Save regs
>>     Call tracer function
>>     Restore regs
>>     old_addr
>>     return old_addr + 4
>> 
>> One can request the feature by setting @trampoline_buf to 1
>> which will allocate a buffer for trampoline.
>> 
>> Signed-off-by: Ryo Takakura <takakura@xxxxxxxxxxxxx>
>> ---
>> 
>> Hi!
>> 
>> For the future, I'm thinking of linux-like extensions
>> which help tracing and debugging by passing:
>> - saved registers
>> - caller information
>> - private data
>> - and so on ...
>> 
>> I would appreciate any advice or suggestion.
>> Thanks!
>> 
>> Example payload file:
>> 
>> #include <xen/lib.h>
>> #include <xen/livepatch.h>
>> 
>> static void my_tracer(void)
>> {
>>     printk("livepatch: do_domctl was called\n");
>> }
>> 
>> static struct livepatch_func funcs[]
>>     __attribute__((section(".livepatch.funcs"))) =
>> {
>>     {
>>         .name = "do_domctl",
>>         .old_size = 4572,
>>         .new_addr = my_tracer,
>>         .new_size = 32,
>>         .trampoline_buf = (void *)1,
>>         .version = LIVEPATCH_PAYLOAD_VERSION,
>>     }
>> };
>> 
>> Sample output:
>> 
>> $ tools/misc/xen-livepatch list
>>  ID                                     | status     | metadata
>> ----------------------------------------+------------+---------------
>> trace_do_domctl                         | APPLIED    |
>> $ xl vcpu-list Domain-0
>> Name                                ID  VCPU   CPU State   Time(s) Affinity 
>> (Hard / Soft)
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> (XEN) livepatch: do_domctl was called
>> Domain-0                             0     0    1   -b-      67.7  all / all
>> Domain-0                             0     1    3   -b-     457.2  all / all
>> Domain-0                             0     2    2   -b-      42.4  all / all
>> Domain-0                             0     3    0   r--      32.4  all / all
>> 
>> Sincerely,
>> Ryo Takakura
>> 
>> ---
>>  xen/arch/arm/arm64/livepatch.c      | 104 +++++++++++++++++++++++++++-
>>  xen/common/livepatch.c              |  40 +++++++++--
>>  xen/include/public/sysctl.h         |   3 +-
>>  xen/include/xen/livepatch.h         |  13 +++-
>>  xen/include/xen/livepatch_payload.h |   2 +
>>  5 files changed, 150 insertions(+), 12 deletions(-)
>> 
>> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
>> index e135bd5bf9..b7c9aba94e 100644
>> --- a/xen/arch/arm/arm64/livepatch.c
>> +++ b/xen/arch/arm/arm64/livepatch.c
>> @@ -15,6 +15,29 @@
>>  #include <asm/insn.h>
>>  #include <asm/livepatch.h>
>>  
>> +
>> +#define AARCH64_REG_SP 31
>> +
>> +static uint32_t aarch64_insn_gen_stp_pre(unsigned int rt,
>> +                                         unsigned int rt2)
>> +{
>> +    return 0xa9800000 |
>> +           (((-16 / 8) & 0x7f) << 15) |
>> +           (rt2 << 10) |
>> +           (AARCH64_REG_SP << 5) |
>> +           rt;
>> +}
>> +
>> +static uint32_t aarch64_insn_gen_ldp_post(unsigned int rt,
>> +                                          unsigned int rt2)
>> +{
>> +    return 0xa8c00000 |
>> +           (((16 / 8) & 0x7f) << 15) |
>> +           (rt2 << 10) |
>> +           (AARCH64_REG_SP << 5) |
>> +           rt;
>> +}
>> +
>>  void arch_livepatch_apply(const struct livepatch_func *func,
>>                            struct livepatch_fstate *state)
>>  {
>> @@ -34,12 +57,87 @@ void arch_livepatch_apply(const struct livepatch_func 
>> *func,
>>      /* Save old ones. */
>>      memcpy(state->insn_buffer, func->old_addr, len);
>>  
>> -    if ( func->new_addr )
>> +    if ( !func->new_addr )
>> +    {
>> +        insn = aarch64_insn_gen_nop();
>> +    }
>> +    else if ( func->trampoline_buf )
>> +    {
>> +        int rc;
>> +        uint32_t *trampoline = func->trampoline_buf;
>> +        uint32_t *tp = trampoline;
>> +        void *orig_cont_addr = (void *)func->old_addr + len;
>> +        unsigned int trampoline_code_size = len + 12 * ARCH_PATCH_INSN_SIZE;
>> +        unsigned long trampoline_start = (unsigned long)trampoline & 
>> PAGE_MASK;
>> +        unsigned long trampoline_end =
>> +            PAGE_ALIGN((unsigned long)trampoline + trampoline_code_size);
>> +
>> +        /*
>> +         * Make the payload text area writeable while generating
>> +         * the trampoline instructions.
>> +         */
>> +        rc = modify_xen_mappings(trampoline_start, trampoline_end,
>> +                                 PAGE_HYPERVISOR);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR LIVEPATCH
>> +                   "Failed to make trampoline writable: %d\n", rc);
>> +            return;
>> +        }
>> +
>> +        /* Save state before calling the tracer. */
>> +        *tp++ = aarch64_insn_gen_stp_pre(0, 1);
>> +        *tp++ = aarch64_insn_gen_stp_pre(2, 3);
>> +        *tp++ = aarch64_insn_gen_stp_pre(4, 5);
>> +        *tp++ = aarch64_insn_gen_stp_pre(6, 7);
>> +        *tp++ = aarch64_insn_gen_stp_pre(29, 30);
>> +
>> +        /* Call user's tracing function. */
>> +        insn = aarch64_insn_gen_branch_imm(
>> +            (unsigned long)tp,
>> +            (unsigned long)func->new_addr,
>> +            AARCH64_INSN_BRANCH_LINK);
>> +        *tp++ = insn;
>> +
>> +        /* Restore state before continuing original function. */
>> +        *tp++ = aarch64_insn_gen_ldp_post(29, 30);
>> +        *tp++ = aarch64_insn_gen_ldp_post(6, 7);
>> +        *tp++ = aarch64_insn_gen_ldp_post(4, 5);
>> +        *tp++ = aarch64_insn_gen_ldp_post(2, 3);
>> +        *tp++ = aarch64_insn_gen_ldp_post(0, 1);
>> +
>> +        /* Original instruction. */
>> +        memcpy(tp, state->insn_buffer, len);
>> +        tp += len / ARCH_PATCH_INSN_SIZE;
>> +
>> +        /* Branch back to original function. */
>> +        insn = aarch64_insn_gen_branch_imm(
>> +            (unsigned long)tp,
>> +            (unsigned long)orig_cont_addr,
>> +            AARCH64_INSN_BRANCH_NOLINK);
>> +        *tp++ = insn;
>> +
>> +        clean_and_invalidate_dcache_va_range(trampoline, 
>> trampoline_code_size);
>> +
>> +        rc = modify_xen_mappings(trampoline_start, trampoline_end,
>> +                                 PAGE_HYPERVISOR_RX);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR LIVEPATCH
>> +                   "Failed to restore trampoline RX mapping: %d\n", rc);
>> +            return;
>> +        }
>> +
>> +        /* Branch from original function to trampoline. */
>> +        insn = aarch64_insn_gen_branch_imm(
>> +            (unsigned long)func->old_addr,
>> +            (unsigned long)func->trampoline_buf,
>> +            AARCH64_INSN_BRANCH_NOLINK);
>> +    }
>> +    else if ( func->new_addr )
>>          insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
>>                                             (unsigned long)func->new_addr,
>>                                             AARCH64_INSN_BRANCH_NOLINK);
>> -    else
>> -        insn = aarch64_insn_gen_nop();
>
>If we want to go this route, and use livepatching for this purpose, we
>need to branch the use-cases in common code, and have arches provide
>both a replacement and a preface addition hooks IMO.

I think that is a good idea.
I'll take this into account for the next.

>>  
>>      /* Verified in livepatch_verify_distance. */
>>      ASSERT(insn != AARCH64_BREAK_FAULT);
>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>> index 7515a040ad..8863ad5ca3 100644
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -280,10 +280,30 @@ static int move_payload(struct payload *payload, 
>> struct 
>> livepatch_elf *elf)
>>  {
>>      void *text_buf, *ro_buf, *rw_buf;
>>      unsigned int i, rw_buf_sec, rw_buf_cnt = 0;
>> -    size_t size = 0;
>> +    const struct livepatch_elf_sec *sec;
>> +    const struct livepatch_func *funcs;
>> +    unsigned int nfuncs, trampolines_needed = 0;
>> +    size_t size = 0, trampoline_size = 0;
>>      unsigned int *offset;
>>      int rc = 0;
>>  
>> +    sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
>> +    if ( sec )
>> +    {
>> +        funcs = sec->addr;
>> +        nfuncs = sec->sec->sh_size / sizeof(*funcs);
>> +
>> +        for ( i = 0; i < nfuncs; ++i )
>> +            if ( funcs[i].trampoline_buf == (void *)1 )
>> +                trampolines_needed++;
>> +
>> +        if ( trampolines_needed )
>> +        {
>> +            payload->n_trampolines = trampolines_needed;
>> +            trampoline_size = trampolines_needed * 
>> LIVEPATCH_TRAMPOLINE_SIZE;
>> +        }
>> +    }
>> +
>>      offset = xmalloc_array(unsigned int, elf->hdr->e_shnum);
>>      if ( !offset )
>>          return -ENOMEM;
>> @@ -323,8 +343,8 @@ static int move_payload(struct payload *payload, struct 
>> livepatch_elf *elf)
>>       * them on separate pages. The last one will by default fall on its
>>       * own page.
>>       */
>> -    size = PAGE_ALIGN(payload->text_size) + PAGE_ALIGN(payload->rw_size) +
>> -                      payload->ro_size;
>> +    size = PAGE_ALIGN(payload->text_size + trampoline_size) +
>> +           PAGE_ALIGN(payload->rw_size) + payload->ro_size;
>>  
>>      size = PFN_UP(size); /* Nr of pages. */
>>      text_buf = vmalloc_xen(size * PAGE_SIZE);
>> @@ -335,9 +355,12 @@ static int move_payload(struct payload *payload, struct 
>> livepatch_elf *elf)
>>          rc = -ENOMEM;
>>          goto out;
>>      }
>> -    rw_buf = text_buf + PAGE_ALIGN(payload->text_size);
>> +    rw_buf = text_buf + PAGE_ALIGN(payload->text_size + trampoline_size);
>>      ro_buf = rw_buf + PAGE_ALIGN(payload->rw_size);
>>  
>> +    if ( trampoline_size )
>> +        payload->trampoline_addr = text_buf + payload->text_size;
>> +
>>      payload->pages = size;
>>      payload->text_addr = text_buf;
>>      payload->rw_addr = rw_buf;
>> @@ -690,7 +713,7 @@ static int prepare_payload(struct payload *payload,
>>  {
>>      const struct livepatch_elf_sec *sec;
>>      const struct payload *data;
>> -    unsigned int i;
>> +    unsigned int i, trampoline_idx = 0;
>>      struct livepatch_func *funcs;
>>      struct livepatch_func *f;
>>      struct virtual_region *region;
>> @@ -737,6 +760,13 @@ static int prepare_payload(struct payload *payload,
>>              if ( rc )
>>                  return rc;
>>  
>> +            if ( f->trampoline_buf == (void *)1 )
>> +            {
>> +                f->trampoline_buf = (char *)payload->trampoline_addr +
>
>You don't need to cast to char *, the type of trampoline_addr is void
>*, and we use the GNU extension to allow void pointer arithmetic by
>treating the size of a void or of a function as 1.

Oh, thanks for the advice!
I'll keep it in mind.

>> +                                    trampoline_idx * 
>> LIVEPATCH_TRAMPOLINE_SIZE;
>> +                trampoline_idx++;
>> +            }
>> +
>>              rc = livepatch_verify_distance(f);
>>              if ( rc )
>>                  return rc;
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index c7cd9b4eb0..e79615d7c9 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1010,10 +1010,11 @@ struct livepatch_func {
>>      const char *name;       /* Name of function to be patched. */
>>      void *new_addr;
>>      void *old_addr;
>> +    void *trampoline_buf;   /* Trampoline buffer when set to (void *)1. */
>>      uint32_t new_size;
>>      uint32_t old_size;
>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>> -    uint8_t _pad[39];
>> +    uint8_t _pad[31];
>
>New fields should be preferably added at the tail of the structure,
>and the change here needs to be propagated into livepatch-build-tools
>livepatch_patch_func structure.  See:
>
>https://xenbits.xen.org/gitweb/?p=livepatch-build-tools.git;a=blob;f=common.h;h=7f3a82ffdb29d2d1d117c1ccb20cc328bdb0529a;hb=HEAD#l135
>
>This is sadly all very fragile.

Thanks. I will make sure to update livepatch-build-tools if needed.
(But I believe the change here won't be needed as pointed by Andrew [1])

>>      livepatch_expectation_t expect;
>>  };
>>  typedef struct livepatch_func livepatch_func_t;
>> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
>> index 45c8924f34..7a81763cf2 100644
>> --- a/xen/include/xen/livepatch.h
>> +++ b/xen/include/xen/livepatch.h
>> @@ -48,6 +48,8 @@ struct xen_sysctl_livepatch_op;
>>  #define ELF_LIVEPATCH_POSTREVERT_HOOK ".livepatch.hooks.postrevert"
>>  /* Arbitrary limit for payload size and .bss section size. */
>>  #define LIVEPATCH_MAX_SIZE     MB(2)
>> +/* Size of a trampoline used for function tracing */
>> +#define LIVEPATCH_TRAMPOLINE_SIZE 128
>>  
>>  struct livepatch_symbol {
>>      const char *name;
>> @@ -109,13 +111,18 @@ unsigned int livepatch_insn_len(const struct 
>> livepatch_func *func,
>>  
>>  static inline int livepatch_verify_distance(const struct livepatch_func 
>> *func)
>>  {
>> +    const void *target;
>>      long offset;
>>      long range = ARCH_LIVEPATCH_RANGE;
>>  
>> -    if ( !func->new_addr ) /* Ignore NOPs. */
>> -        return 0;
>> +    if ( func->trampoline_buf )
>> +     target = func->trampoline_buf;
>> +    else if ( func->new_addr )
>> +     target = func->new_addr;
>> +    else
>> +     return 0; /* Ignore NOPs. */
>
>FWIW, indentation is wrong here, you are adding hard tabs.

My mistake. I'll fix this.

Sincerely,
Ryo Takakura

[1] https://lists.xen.org/archives/html/xen-devel/2026-06/msg01583.html

>Thanks, Roger.