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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/10/2026 11:40 am, 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).  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>
> ---
> 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, 35 insertions(+), 37 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
> index 4602c32cff..0b0beca15a 100644
> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -901,7 +901,8 @@ 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];
> +    bool check[POD_SWEEP_STRIDE];
> +    const unsigned long *map;
>      struct domain *d = p2m->domain;
>      unsigned int i, j, max_ref = 1;

I agree with getting rid of the map batching, but your LLM has done a
poor job doing so.

An array of 16 bools is excessive.  You want "unsigned int check = 0;"
here, so ...

>  
> @@ -911,7 +912,12 @@ 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. */
> +    /*
> +     * First, get the gfn list, translate to mfns, and pick the pages to
> +     * check.  They are mapped one at a time below, as in
> +     * p2m_pod_zero_check_superpage(): a vCPU can hold only a few transient
> +     * mappings at once, and the p2m lookups and updates here need some too.
> +     */
>      for ( i = 0; i < count; i++ )
>      {
>          p2m_access_t a;
> @@ -921,18 +927,17 @@ 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;
> +        check[i] = false;

... this can be dropped, and ...

>          if ( p2m_is_ram(types[i]) )
>          {
>              const struct page_info *pg = mfn_to_page(mfns[i]);
>  
> -            if ( !is_special_page(pg) &&
> -                 (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[i] = !is_special_page(pg) &&
> +                       (pg->count_info & PGC_allocated) &&
> +                       !(pg->count_info & PGC_shadowed_pt) &&
> +                       ((pg->count_info & PGC_count_mask) <= max_ref);

... this can be put back to it's older if() form, with `check |= (1U <<
i);` instead, so ...

>          }
>      }
>  
> @@ -942,18 +947,24 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>       */
>      for ( i = 0; i < count; i++ )
>      {
> -        if ( !map[i] )
> +        if ( !check[i] )
>              continue;

... this and ...

>  
>          /* Quick zero-check */
> +        map = map_domain_page(mfns[i]);
>          for ( j = 0; j < 16; j++ )
> -            if ( *(map[i] + j) != 0 )
> -                goto skip;
> +            if ( map[j] != 0 )
> +                break;
> +        unmap_domain_page(map);
>  
>          /* Try to remove the page, restoring old mapping if it fails. */
> -        if ( p2m_set_entry(p2m, gfns[i], INVALID_MFN, PAGE_ORDER_4K,
> +        if ( j < 16 ||
> +             p2m_set_entry(p2m, gfns[i], INVALID_MFN, PAGE_ORDER_4K,
>                             p2m_populate_on_demand, p2m->default_access) )
> -            goto skip;
> +        {
> +            check[i] = false;
> +            continue;
> +        }
>  
>          /*
>           * See if the page was successfully unmapped.  (Allow one refcount
> @@ -961,6 +972,8 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>           */
>          if ( (mfn_to_page(mfns[i])->count_info & PGC_count_mask) > 1 )
>          {
> +            check[i] = false;
> +
>              /*
>               * If the previous p2m_set_entry call succeeded, this one 
> shouldn't
>               * be able to fail.  If it does, crashing the domain should be 
> safe.
> @@ -970,14 +983,8 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>              {
>                  ASSERT_UNREACHABLE();
>                  domain_crash(d);
> -                goto out_unmap;
> +                return;
>              }
> -
> -        skip:
> -            unmap_domain_page(map[i]);
> -            map[i] = NULL;
> -
> -            continue;
>          }
>      }
>  
> @@ -986,22 +993,20 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>      /* Now check each page for real */
>      for ( i = 0; i < count; i++ )
>      {
> -        if ( !map[i] )
> +        if ( !check[i] )
>              continue;

... this can turn into for_each_set_bit() loops.

~Andrew