Hi Julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Thursday, September 1, 2022 9:53 PM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Jan Beulich <jbeul...@suse.com>; Stefano Stabellini <sstabell...@kernel.org>;
> Wei Liu <w...@xen.org>
> Subject: Re: [PATCH v11 6/6] xen: retrieve reserved pages on
> populate_physmap
>
> Hi Penny,
>
> On 31/08/2022 03:40, Penny Zheng wrote:
> > +/*
> > + * Acquire a page from reserved page list(resv_page_list), when
> > +populating
> > + * memory for static domain on runtime.
> > + */
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> > +{
> > + struct page_info *page;
> > +
> > + ASSERT_ALLOC_CONTEXT();
> > +
> > + /* Acquire a page from reserved page list(resv_page_list). */
> > + spin_lock(&d->page_alloc_lock);
> > + page = page_list_remove_head(&d->resv_page_list);
> > + spin_unlock(&d->page_alloc_lock);
> > + if ( unlikely(!page) )
> > + return INVALID_MFN;
> > +
> > + if ( !prepare_staticmem_pages(page, 1, memflags) )
> > + goto fail;
> > +
> > + if ( assign_domstatic_pages(d, page, 1, memflags) )
> > + goto fail_assign;
> > +
> > + return page_to_mfn(page);
> > +
> > + fail_assign:
> > + unprepare_staticmem_pages(page, 1, false);
>
> Looking at assign_domstatic_pages(). It will already call
> unprepare_staticmem_pages() in one of the error path. It doesn't look like
> the latter can be called twice on a page.
>
> To be honest, I find a bit odd that assign_domstatic_pages() is calling
> unprepare_staticmem_pages() because the former doesn't call the "prepare"
> function.
>
> AFAICT, this is an issue introduced in this patch. So I would remove the call
> from assign_domstatic_pages() and then let the caller calls
> unprepare_staticmem_pages() (this would need to be added in
> acquire_domstatic_pages()).
>
True, true, thanks for pointing out!!
> Also, I think it would be good to explain why we don't need to scrub.
> Something like:
>
> "The page was never accessible by the domain. So scrubbing can be skipped".
>
Ok, I'll add in-code comment
> Cheers,
>
> --
> Julien Grall
