On Mon, Jan 19, 2026 at 02:00:49PM +0100, Jan Beulich wrote:
> On 15.01.2026 12:18, Roger Pau Monne wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -722,6 +722,15 @@ static void _domain_destroy(struct domain *d)
> >
> > XVFREE(d->console);
> >
> > + if ( d->pending_scrub )
> > + {
> > + BUG_ON(d->creation_finished);
> > + free_domheap_pages(d->pending_scrub, d->pending_scrub_order);
> > + d->pending_scrub = NULL;
> > + d->pending_scrub_order = 0;
> > + d->pending_scrub_index = 0;
> > + }
>
> Because of the other zeroing wanted (it's not strictly needed, is it?),
> it may be a little awkward to use FREE_DOMHEAP_PAGES() here. Yet I would
> still have recommended to avoid its open-coding, if only we had such a
> wrapper already.
I don't mind introducing a FREE_DOMHEAP_PAGES() wrapper in this same
patch, if you are OK with it.
> Would this better be done earlier, in domain_kill(), to avoid needlessly
> holding back memory that isn't going to be used by this domain anymore?
> Would require the spinlock be acquired to guard against a racing
> stash_allocation(), I suppose. In fact freeing right in
> domain_unpause_by_systemcontroller() might be yet better (albeit without
> eliminating the need to do it here or in domain_kill()).
Even with a lock taken moving to domain_kill() would be racy. A rogue
toolstack could keep trying to issue populate_physmap hypercalls which
would fail in the assign_pages() call, but it could still leave
pending pages in d->pending_scrub, as the assign_pages() call happens
strictly after the scrubbing is done.
> > @@ -1678,6 +1687,14 @@ int domain_unpause_by_systemcontroller(struct domain
> > *d)
> > */
> > if ( new == 0 && !d->creation_finished )
> > {
> > + if ( d->pending_scrub )
> > + {
> > + printk(XENLOG_ERR
> > + "%pd: cannot be started with pending dirty pages,
> > destroying\n",
>
> s/dirty/unscrubbed/ to avoid ambiguity with "dirty" as in "needing writeback"?
>
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -159,6 +159,74 @@ static void increase_reservation(struct memop_args *a)
> > a->nr_done = i;
> > }
> >
> > +/*
> > + * Temporary storage for a domain assigned page that's not been fully
> > scrubbed.
> > + * Stored pages must be domheap ones.
> > + *
> > + * The stashed page can be freed at any time by Xen, the caller must pass
> > the
> > + * order and NUMA node requirement to the fetch function to ensure the
> > + * currently stashed page matches it's requirements.
> > + */
> > +static void stash_allocation(struct domain *d, struct page_info *page,
> > + unsigned int order, unsigned int scrub_index)
> > +{
> > + BUG_ON(d->creation_finished);
>
> Is this valid here and ...
>
> > + rspin_lock(&d->page_alloc_lock);
> > +
> > + /*
> > + * Drop any stashed allocation to accommodated the current one. This
> > + * interface is designed to be used for single-threaded domain
> > creation.
> > + */
> > + if ( d->pending_scrub )
> > + free_domheap_pages(d->pending_scrub, d->pending_scrub_order);
> > +
> > + d->pending_scrub_index = scrub_index;
> > + d->pending_scrub_order = order;
> > + d->pending_scrub = page;
> > +
> > + rspin_unlock(&d->page_alloc_lock);
> > +}
> > +
> > +static struct page_info *get_stashed_allocation(struct domain *d,
> > + unsigned int order,
> > + nodeid_t node,
> > + unsigned int *scrub_index)
> > +{
> > + struct page_info *page = NULL;
> > +
> > + BUG_ON(d->creation_finished && d->pending_scrub);
>
> ... here? A badly behaved toolstack could do a populate in parallel with
> the initial unpause, couldn't it?
Oh, I think I've forgot to refresh before sending the patch . I have
those as ASSERTs in my local copy anyway. But yes, you are right,
populate_physmap() is not done while holding the domctl lock, so it
can race with an unpause. I will remove the ASSERTs.
> > + rspin_lock(&d->page_alloc_lock);
> > +
> > + /*
> > + * If there's a pending page to scrub check it satisfies the current
> > + * request. If it doesn't keep it stashed and return NULL.
> > + */
> > + if ( !d->pending_scrub || d->pending_scrub_order != order ||
> > + (node != NUMA_NO_NODE && node != page_to_nid(d->pending_scrub)) )
>
> Ah, and MEMF_exact_node is handled in the caller.
>
> > + goto done;
> > + else
> > + {
> > + page = d->pending_scrub;
> > + *scrub_index = d->pending_scrub_index;
> > + }
> > +
> > + /*
> > + * The caller now owns the page, clear stashed information. Prevent
> > + * concurrent usages of get_stashed_allocation() from returning the
> > same
> > + * page to different contexts.
> > + */
> > + d->pending_scrub_index = 0;
> > + d->pending_scrub_order = 0;
> > + d->pending_scrub = NULL;
> > +
> > + done:
> > + rspin_unlock(&d->page_alloc_lock);
> > +
> > + return page;
> > +}
>
> Hmm, you free the earlier allocation only in stash_allocation(), i.e. that
> memory isn't available to fulfill the present request. (I do understand
> that the freeing there can't be dropped, to deal with possible races
> caused by the toolstack.)
Since we expect populate_physmap(9 to be executed sequentially by the
toolstack I would argue it's fine to hold onto that memory. Otherwise
I could possibly free in get_stashed_allocation() when the request
doesn't match what's stashed. I opted for freeing later in
stash_allocation() to maybe give time for the other parallel caller to
finish the scrubbing.
> The use of "goto" here also looks a little odd, as it would be easy to get
> away without. Or else I'd like to ask that the "else" be dropped.
Hm, OK, let me use an unlock + return and also drop the else then. I
think that's clearer.
> > @@ -286,6 +365,30 @@ static void populate_physmap(struct memop_args *a)
> > goto out;
> > }
> >
> > + if ( !d->creation_finished )
> > + {
> > + unsigned int dirty_cnt = 0, j;
>
> Declaring (another) j here is going to upset Eclair, I fear, as ...
>
> > + /* Check if there's anything to scrub. */
> > + for ( j = scrub_start; j < (1U << a->extent_order);
> > j++ )
> > + {
> > + if ( !test_and_clear_bit(_PGC_need_scrub,
> > + &page[j].count_info) )
> > + continue;
> > +
> > + scrub_one_page(&page[j], true);
> > +
> > + if ( (j + 1) != (1U << a->extent_order) &&
> > + !(++dirty_cnt & 0xff) &&
> > + hypercall_preempt_check() )
> > + {
> > + a->preempted = 1;
> > + stash_allocation(d, page, a->extent_order,
> > ++j);
>
> Better j + 1, as j's value isn't supposed to be used any further?
>
> > + goto out;
> > + }
> > + }
> > + }
> > +
> > if ( unlikely(a->memflags & MEMF_no_tlbflush) )
> > {
> > for ( j = 0; j < (1U << a->extent_order); j++ )
>
> ... for this to work there must already be one available from an outer scope.
Indeed, I've removed that outrageous j variable.
Thanks, Roger.
