Re: [PATCH v4 2/3] xen/mm: allow deferred scrub of physmap populate allocated pages

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

On Wed, Jan 28, 2026 at 12:44:26PM +0000, Andrew Cooper wrote:
> On 28/01/2026 12:03 pm, Roger Pau Monne wrote:
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 376351b528c9..123202f2c025 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -710,6 +710,23 @@ static int domain_teardown(struct domain *d)
> >      return 0;
> >  }
> >  
> > +/*
> > + * Called multiple times during domain destruction, to attempt to early 
> > free
> > + * any stashed pages to be scrubbed.  The call from _domain_destroy() is 
> > done
> > + * when the toolstack can no longer stash any pages.
> > + */
> > +static void domain_free_pending_scrub(struct domain *d)
> > +{
> > +    rspin_lock(&d->page_alloc_lock);
> > +    if ( d->pending_scrub )
> > +    {
> > +        FREE_DOMHEAP_PAGES(d->pending_scrub, d->pending_scrub_order);
> > +        d->pending_scrub_order = 0;
> > +        d->pending_scrub_index = 0;
> > +    }
> > +    rspin_unlock(&d->page_alloc_lock);
> > +}
> > +
> >  /*
> >   * Destroy a domain once all references to it have been dropped.  Used 
> > either
> >   * from the RCU path, or from the domain_create() error path before the 
> > domain
> > @@ -722,6 +739,8 @@ static void _domain_destroy(struct domain *d)
> >  
> >      XVFREE(d->console);
> >  
> > +    domain_free_pending_scrub(d);
> > +
> >      argo_destroy(d);
> >  
> >      rangeset_domain_destroy(d);
> > @@ -1286,6 +1305,8 @@ int domain_kill(struct domain *d)
> >          rspin_barrier(&d->domain_lock);
> >          argo_destroy(d);
> >          vnuma_destroy(d->vnuma);
> > +        domain_free_pending_scrub(d);
> > +        rspin_unlock(&d->page_alloc_lock);
> 
> This is a double unlock, isn't it?

Bah, this was a leftover from the previous version, sorry.

> 
> The freeing wants to be in domain_kill() (ish), or _domain_destroy() but
> not both.

Jan specifically asked for the cleanup to be in both.
_domain_destroy() is the must have one, as that's done once the domain
is unhooked from the domain list and hence can no longer be the target
of populate physmap hypercalls.  Jan asked for the domain_kill()
instance to attempt to free the pending page as early as possible.

> In this case we can't have anything using pending scrubbing until the
> domain is the domlist (i.e. can be the target of other hypercalls), so
> this wants to be in domain_relinquish_resources()  (rather than
> domain_kill() which we're trying to empty).

domain_relinquish_resources() is arch-specific, while the
pending_scrub stuff is common with all arches.  It seemed better
placed in domain_kill() because that's generic code shared by all
arches.

I could place it in domain_teardown() instead if that's better than
domain_kill().

> In principle we could assert that it's already NULL in _domain_destroy()
> which might help catch an error if it gets set early but the domain
> destroyed before getting into the domlist, but that seems like a rather
> slim case.

Given my understanding of the logic in the XENMEM_ hypercalls, I think
toolstack can still target domains in the process of being destroyed,
at which point we need to keep a final cleanup instance
_domain_destroy(), or otherwise adjust XENMEM_populate_physmap
hypercall (and others?) so they can't target dying domains.

Thanks, Roger.