Re: [PATCH v3 2/2] xen/x86: Change stub page allocation/free

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 08, 2026 at 08:06:38PM -0400, Jason Andryuk wrote:
> Today the inline tracking of the stub page is problematic.  0xcc is used
> to indicate unused, but it is also a "clear value."  A !CONFIG_PV build
> with smt=0 will bring up CPU0, bring up CPU1, bring down CPU1, and free
> the in-use stub page.  Subsequent CPU onlining can write to the re-used
> page.
> 
> The new approach uses a global, CPU-indexed array of stub pages.
> However, to handle NUMA aware allocations, we cannot allocate all the
> pages in advance because the NUMA information is not available.  Keep
> track of 1 current page for each NUMA node, allocated on demand, and
> allocate the stub buffers out of those pages.
> 
> The current NUMA allocation approach is opportunistic sharing among the
> groups of 32 processors.  The new approach will allocate buffers densely
> in a NUMA node.
> 
> stub pages are no longer freed.  They remain referenced in the global
> CPU-indexed array and are re-used if the CPU is re-onlined.
> 
> stubs and node_stubs don't have an explicit lock.  During boot they are
> accessed single threaded.  During runtime, &cpu_add_remove_lock
> serializes access.
> 
> Fixes: 7a66ac8d1633 ("x86: move syscall trampolines off the stack")
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
> I'm not sure how to test the NUMA part - I don't have an NUMA system.
> Also, if NUMA is active, is a cpu node of NUMA_NO_NODE still possible?
> I used the MAX_NUMNODES + 1 array sizing to handle that, but it's not
> obvious to me if that is necessary.
> 
> Roger mentioned removing the per-cpu stubs.mfn.  We'd need to replace
> that with exposing the stubs array for traps and the emulator.  I have
> no idea if that will be an improvement and am looking for agreement on
> this patch before attempting.
> ---
>  xen/arch/x86/include/asm/stubs.h |   2 +-
>  xen/arch/x86/setup.c             |   3 +-
>  xen/arch/x86/smpboot.c           | 110 +++++++++++++++++++++----------
>  3 files changed, 77 insertions(+), 38 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/stubs.h 
> b/xen/arch/x86/include/asm/stubs.h
> index a520928e9a..9d776f81dd 100644
> --- a/xen/arch/x86/include/asm/stubs.h
> +++ b/xen/arch/x86/include/asm/stubs.h
> @@ -32,6 +32,6 @@ struct stubs {
>  };
>  
>  DECLARE_PER_CPU(struct stubs, stubs);
> -unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn);
> +unsigned long assign_stub_page(unsigned int cpu);
>  
>  #endif /* X86_ASM_STUBS_H */
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 19ee857abf..0cac94cbdb 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -2089,8 +2089,7 @@ void asmlinkage __init noreturn __start_xen(void)
>  
>      init_idle_domain();
>  
> -    this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
> -                                           &this_cpu(stubs).mfn);
> +    this_cpu(stubs.addr) = assign_stub_page(0);

Given stub pages is first used quite late in the boot process, the above
arrays would better be dynamically allocated using xvmalloc_array().

>      BUG_ON(!this_cpu(stubs.addr));
>  
>      bsp_traps_reinit(); /* Needs stubs allocated, must be before 
> presmp_initcalls. */
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index d7619f534b..d9cd90389d 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -641,41 +641,96 @@ static int do_boot_cpu(int apicid, int cpu)
>      return rc;
>  }
>  
> -#define STUB_BUF_CPU_OFFS(cpu) (((cpu) & (STUBS_PER_PAGE - 1)) * 
> STUB_BUF_SIZE)
> +/*
> + * Indexed by CPU.  `pg` may be shared by up to STUBS_PER_PAGE CPUs.  Offset
> + * is the byte offset into the stub page for the CPU's stub buffer.
> + */
> +struct stub_info {
> +    struct page_info *pg;
> +    unsigned int offset;
> +};
> +struct stub_info __read_mostly stubs[NR_CPUS];
>  
> -unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn)
> +/*
> + * Index by NUMA node.
> + *
> + * `pg` is the current stub page for the node.
> + * `next` is the next available stub index (STUBS_PER_PAGE available).
> + *
> + * if `pg` is NULL, allocate a new one.
> + * if `pg` is !NULL, use `pg` and stub `next`
> + * When STUBS_PER_PAGE are all assigned, clear `pg` and `next`.
> + */
> +struct stub_node {
> +    struct page_info *pg;
> +    unsigned int next;
> +};
> +struct stub_node stub_nodes[MAX_NUMNODES + 1];

I think we could get away with a single array, that uses the CPU as
the index and stores the physical address of the stub.

We could also simplify the allocation logic, assuming that CPUs
belonging to the same NUMA node are packed contiguously in the common
case.  I've given a try at this, and adjusted your original commit.  I
however only tested this in QEMU so far.  If you think it's OK I can
test it on XenRT and see how that goes.

Sorry I took over the patch, I didn't want to force you into another
direction without knowing whether it would be OK, as it wasn't clear
to me this approach would be fine (seem so, but still needs further
testing).

One thing that would simplify the logic greatly, which Andrew brought
up, is foregoing the NUMA memory affinity for the allocated stubs page, and
allocate and map them contiguously in both the physical and the linear
address spaces, so that you would find the VA using:

XEN_VIRT_END - FIXADDR_X_SIZE - (cpu + 1) * STUB_BUF_SIZE

This would possibly allow to simply populate the whole range up to
num_present_cpus() at boot and get done with it.  However that's a
bigger change that should likely be done after 4.22 is out.

Thanks, Roger.
---
diff --git a/xen/arch/x86/include/asm/stubs.h b/xen/arch/x86/include/asm/stubs.h
index a520928e9a50..d575f1eb0631 100644
--- a/xen/arch/x86/include/asm/stubs.h
+++ b/xen/arch/x86/include/asm/stubs.h
@@ -32,6 +32,7 @@ struct stubs {
 };
 
 DECLARE_PER_CPU(struct stubs, stubs);
-unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn);
+unsigned long assign_stub_page(unsigned int cpu);
+void init_bsp_stub(void);
 
 #endif /* X86_ASM_STUBS_H */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4192edf635b6..cddf8806c877 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -2089,9 +2089,7 @@ void asmlinkage __init noreturn __start_xen(void)
 
     init_idle_domain();
 
-    this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
-                                           &this_cpu(stubs).mfn);
-    BUG_ON(!this_cpu(stubs.addr));
+    init_bsp_stub();
 
     bsp_traps_reinit(); /* Needs stubs allocated, must be before 
presmp_initcalls. */
 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index b3045eac5b5e..dd0972a3025e 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -20,6 +20,7 @@
 #include <xen/serial.h>
 #include <xen/softirq.h>
 #include <xen/tasklet.h>
+#include <xen/xvmalloc.h>
 
 #include <asm/apic.h>
 #include <asm/cpuidle.h>
@@ -641,41 +642,61 @@ static int do_boot_cpu(int apicid, int cpu)
     return rc;
 }
 
-#define STUB_BUF_CPU_OFFS(cpu) (((cpu) & (STUBS_PER_PAGE - 1)) * STUB_BUF_SIZE)
+/* Dynamically allocated, indexed by CPU.  Store physical address of stubs. */
+static paddr_t *__ro_after_init stubs;
 
-unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn)
+unsigned long assign_stub_page(unsigned int cpu)
 {
     unsigned long stub_va;
-    struct page_info *pg;
+    paddr_t addr = stubs[cpu];
 
-    BUILD_BUG_ON(STUBS_PER_PAGE & (STUBS_PER_PAGE - 1));
-
-    if ( *mfn )
-        pg = mfn_to_page(_mfn(*mfn));
-    else
+    if ( addr == INVALID_PADDR )
     {
-        nodeid_t node = cpu_to_node(cpu);
-        unsigned int memflags = node != NUMA_NO_NODE ? MEMF_node(node) : 0;
+        nodeid_t nid = cpu_to_node(cpu);
 
-        pg = alloc_domheap_page(NULL, memflags);
-        if ( !pg )
-            return 0;
+        /*
+         * Attempt to use the same page as the previous CPU if possible,
+         * otherwise allocate a new one.
+         */
+        if ( cpu && nid == cpu_to_node(cpu - 1) &&
+             PAGE_OFFSET(stubs[cpu - 1] + STUB_BUF_SIZE) )
+            addr = stubs[cpu - 1] + STUB_BUF_SIZE;
+        else
+        {
+            struct page_info *pg = alloc_domheap_page(NULL, MEMF_node(nid));
 
-        unmap_domain_page(memset(__map_domain_page(pg), 0xcc, PAGE_SIZE));
+            if ( !pg )
+                return 0;
+            addr = page_to_maddr(pg);
+        }
+        stubs[cpu] = addr;
     }
 
     stub_va = XEN_VIRT_END - FIXADDR_X_SIZE - (cpu + 1) * PAGE_SIZE;
-    if ( map_pages_to_xen(stub_va, page_to_mfn(pg), 1,
+    if ( map_pages_to_xen(stub_va, maddr_to_mfn(addr), 1,
                           PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) )
-    {
-        if ( !*mfn )
-            free_domheap_page(pg);
-        stub_va = 0;
-    }
-    else if ( !*mfn )
-        *mfn = mfn_x(page_to_mfn(pg));
+        return 0;
+
+    per_cpu(stubs.mfn, cpu) = PFN_DOWN(addr);
+    return stub_va + PAGE_OFFSET(addr);
+}
+
+void __init init_bsp_stub(void)
+{
+    const unsigned int num_cpus = num_present_cpus();
+    unsigned int i;
+
+    ASSERT(!stubs);
+    stubs = xvmalloc_array(typeof(*stubs), num_cpus);
+    if ( !stubs )
+        panic("Unable to allocate stub array");
+
+    for ( i = 0; i < num_cpus; i++ )
+        stubs[i] = INVALID_PADDR;
 
-    return stub_va ? stub_va + STUB_BUF_CPU_OFFS(cpu) : 0;
+    this_cpu(stubs.addr) = assign_stub_page(0);
+    if ( !this_cpu(stubs.addr) )
+        panic("Unable to initialize BSP stub region");
 }
 
 void cpu_exit_clear(unsigned int cpu)
@@ -990,19 +1011,12 @@ static void cpu_smpboot_free(unsigned int cpu, bool 
remove)
     {
         mfn_t mfn = _mfn(per_cpu(stubs.mfn, cpu));
         unsigned char *stub_page = map_domain_page(mfn);
-        unsigned int i;
 
-        memset(stub_page + STUB_BUF_CPU_OFFS(cpu), 0xcc, STUB_BUF_SIZE);
-        for ( i = 0; i < STUBS_PER_PAGE; ++i )
-            if ( stub_page[i * STUB_BUF_SIZE] != 0xcc )
-                break;
+        memset(stub_page + PAGE_OFFSET(stubs[cpu]), 0xcc, STUB_BUF_SIZE);
         unmap_domain_page(stub_page);
         destroy_xen_mappings(per_cpu(stubs.addr, cpu) & PAGE_MASK,
                              (per_cpu(stubs.addr, cpu) | ~PAGE_MASK) + 1);
         per_cpu(stubs.addr, cpu) = 0;
-        per_cpu(stubs.mfn, cpu) = 0;
-        if ( i == STUBS_PER_PAGE )
-            free_domheap_page(mfn_to_page(mfn));
     }
 
     if ( IS_ENABLED(CONFIG_PV32) )
@@ -1041,7 +1055,7 @@ void *cpu_alloc_stack(unsigned int cpu)
 static int cpu_smpboot_alloc(unsigned int cpu)
 {
     struct cpu_info *info;
-    unsigned int i, memflags = 0;
+    unsigned int memflags = 0;
     nodeid_t node = cpu_to_node(cpu);
     seg_desc_t *gdt;
     unsigned long stub_va;
@@ -1092,15 +1106,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     memcpy(per_cpu(idt, cpu), bsp_idt, sizeof(bsp_idt));
     disable_each_ist(per_cpu(idt, cpu));
 
-    for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
-          i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
-        if ( cpu_online(i) && cpu_to_node(i) == node )
-        {
-            per_cpu(stubs.mfn, cpu) = per_cpu(stubs.mfn, i);
-            break;
-        }
-    BUG_ON(i == cpu);
-    stub_va = alloc_stub_page(cpu, &per_cpu(stubs.mfn, cpu));
+    stub_va = assign_stub_page(cpu);
     if ( !stub_va )
         goto out;
     per_cpu(stubs.addr, cpu) = stub_va;