Re: [PATCH v4 01/30] x86/PoD: map one page at a time in p2m_pod_zero_check()

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Mon, Oct 19, 2026 at 08:42:32PM +0100, George Dunlap wrote:
> p2m_pod_zero_check() maps every page of its batch that passes its
> checks, up to POD_SWEEP_STRIDE (16) of them, and keeps them mapped
> across the p2m lookups and updates which follow, each of which maps
> page-table pages of its own.  Its callers hold mappings too: the p2m
> lookup which found the entry to populate still has its page-table page
> mapped, and a PV mmu_update() the L1 page it is writing.
> 
> map_domain_page() cannot always provide that many at once.  It uses the
> mapcache on behalf of PV vCPUs only: in debug builds for every page, in
> release builds for pages beyond the reach of the directmap in PV guests'
> page-tables (5TiB, 3.5TiB with CONFIG_BIGMEM).

This is IMO too verbose.  It doesn't add any value to the
justification here what's the memory limit at which the mapcache starts
to be used.  I also don't think it's relevant to list what causes
mapcache to be used in general.  It just distracts from the
justification of the change, by making the patch description unwieldy
long.

> The mapcache has
> MAPCACHE_VCPU_ENTRIES (16) slots per vCPU of the domain, shared by its
> vCPUs, and a mapping which finds no slot free or reclaimable hits the
> BUG_ON() in map_domain_page(): the host crashes.
> 
> The sweep runs in the context of whichever vCPU populates a PoD entry
> while the domain's PoD cache is empty.  The guest's own vCPUs, being
> HVM, map through the directmap; a PV vCPU gets there when its domain
> maps or copies the guest's memory: a device model in dom0 or in a stub
> domain, a backend doing grant copies.  A PV domain with a single vCPU,
> as a device-model stub domain or a dom0 booted with dom0_max_vcpus=1
> is, has 16 slots, and a sweep over 16 candidate pages needs more.
> 
> The sweep has mapped its whole batch at once since 9ad9a1609fbe ("PoD
> memory 5/9: emergency scan"), when x86-64 reached all memory through the
> directmap.  The per-vCPU budget came with the mapcache's return to
> x86-64 in 4b28bf6ae90b ("x86: re-introduce map_domain_page() et al").
> 
> Map one page at a time instead, as p2m_pod_zero_check_superpage()
> already does: map a page for the quick check and unmap it again, and
> map it anew for the full check after the p2m update and the TLB flush.
> The checks and their order are unchanged.  Each page is mapped twice,
> which costs little next to scanning it, and the error paths no longer
> have mappings to undo.
> 
> Fixes: 4b28bf6ae90b ("x86: re-introduce map_domain_page() et al")
> Assisted-by: Claude Code:claude-opus-5-5
> Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>

Acked-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>

I've got one suggestion below, but doesn't change the logic.

> ---
> Changes in v4:
> - Track the pages to check in a bitmask walked with for_each_set_bit(),
>   not an array of bools (Andrew).
> - Drop the new comment above the first loop; edit the old one's "and map
>   the pages" instead (Andrew).
> 
> NB this issue was raised in review and verified only by code
> inspection; it hasn't been demonstrated.
> ---
>  xen/arch/x86/mm/p2m-pod.c | 72 +++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 41 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
> index 4602c32cff..ed298fa7d2 100644
> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -901,9 +901,10 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>  {
>      mfn_t mfns[POD_SWEEP_STRIDE];
>      p2m_type_t types[POD_SWEEP_STRIDE];
> -    unsigned long *map[POD_SWEEP_STRIDE];
> +    unsigned int check = 0;
> +    const unsigned long *map;
>      struct domain *d = p2m->domain;
> -    unsigned int i, j, max_ref = 1;
> +    unsigned int j, max_ref = 1;
>  
>      BUG_ON(count > POD_SWEEP_STRIDE);
>  
> @@ -911,8 +912,11 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>      if ( paging_mode_shadow(d) )
>          max_ref++;
>  
> -    /* First, get the gfn list, translate to mfns, and map the pages. */
> -    for ( i = 0; i < count; i++ )
> +    /*
> +     * First, get the gfn list, translate to mfns, and check for being a
> +     * plausible candidate.
> +     */
> +    for ( unsigned int i = 0; i < count; i++ )
>      {
>          p2m_access_t a;
>  
> @@ -921,9 +925,8 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>  
>          /*
>           * If this is ram, and not a pagetable or a special page, and
> -         * probably not mapped elsewhere, map it; otherwise, skip.
> +         * probably not mapped elsewhere, check it; otherwise, skip.
>           */
> -        map[i] = NULL;
>          if ( p2m_is_ram(types[i]) )
>          {
>              const struct page_info *pg = mfn_to_page(mfns[i]);
> @@ -932,7 +935,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>                   (pg->count_info & PGC_allocated) &&
>                   !(pg->count_info & PGC_shadowed_pt) &&
>                   ((pg->count_info & PGC_count_mask) <= max_ref) )
> -                map[i] = map_domain_page(mfns[i]);
> +                check |= 1U << i;

Not that I have a strong opinion, but I think it's clearer if you use
__{set,clear}_bit().

Thanks, Roger.