Re: [PATCH v2 3/3] xen/x86: Change stub page allocation/free logic

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jun 04, 2026 at 07:18:37PM -0400, Jason Andryuk wrote:
> Today the inine 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.
> 
> Each stub page accomodates 32 stub regions, and each CPU uses an offset
> into its portion of the page.  Each CPU used a CPU-specific mapping of
> the whole page.  The virtual address of the CPU-specific mapping is
> fixed, so it can be used to track the stub page.
> 
> Remove the actual free-ing from cpu_smpboot_free().  Use the stub_va PTE
> to track the underlying page.  destroy_xen_mapping() would clear the
> mapping, so replace it with modify_xen_mappings() to retain the PFN in
> the PTE (with NX set).
> 
> In alloc_stub_page(), check for a valid PFN in the stub_va PTE.  When
> found, it will be used.  This handles re-onlining a CPU.  Otherwise the
> existing logic is retained to use a passed in mfn or allocate one.
> These paths handle to bringing up new CPUs.
> 
> If all CPUs for a stub page are offlined, the page will be dangling and
> unusable.  But it will be re-used if CPUs are re-onlined.

I think there's a corner case with this approach where Xen will end up
allocating multiple pages for a chunk of 32 CPUs: consider the first
CPU of a stub page is onlined and then offlined, and afterwards the
second CPU of the same chunk is onlined.  That second CPU would see
stubs.mfn as 0 from its sibling if the first CPU is not parked, and
hence allocate a new stubs page when there's already one allocated for
the chunk.

FWIW, I would take a way more simple approach and allocate all stub
pages (using possible cpus as the max number) in smp_prepare_cpus()
and store them in an array.  Then from cpu_smpboot_alloc() the
to-be-onlined AP can gets the page to use and map it.  We could also
remove stubs.mfn at that point, since the mfns will be immutable for
the lifetime of Xen, and stored in a global array.

> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

You want to add a Fixes: tag above the SoB.

> ---
>  xen/arch/x86/smpboot.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 7241dba621..11937175a9 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -647,11 +647,21 @@ unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn)
>  {
>      unsigned long stub_va;
>      struct page_info *pg;
> +    mfn_t stub_mfn;
>  
>      BUILD_BUG_ON(STUBS_PER_PAGE & (STUBS_PER_PAGE - 1));
>  
> -    if ( *mfn )
> +    stub_va = XEN_VIRT_END - FIXADDR_X_SIZE - (cpu + 1) * PAGE_SIZE;
> +    stub_mfn = page_walk_mfn(virt_to_mfn(idle_pg_table), stub_va);
> +    if ( mfn_valid(stub_mfn) )
> +    {
> +        *mfn = mfn_x(stub_mfn);

Before overwriting what's possibly in mfn, we might want to sanity
check it matches the mfn recovered from the page-tables, ie:

ASSERT(!*mfn || *mfn == mfn_x(stub_mfn));

Thanks, Roger.