RE: [PATCH v11 6/6] xen: retrieve reserved pages on populate_physmap

From: Penny Zheng <Penny.Zheng@arm.com>
To: Julien Grall <julien@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Wei Chen <Wei.Chen@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: RE: [PATCH v11 6/6] xen: retrieve reserved pages on populate_physmap
Date: Fri, 2 Sep 2022 07:10:34 +0000	[thread overview]
Message-ID: <AM0PR08MB45306B56E3664A09F3A6BC38F77A9@AM0PR08MB4530.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <9d328b4d-ef7a-e457-428d-83a2b2351465@xen.org>

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, September 1, 2022 9:53 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@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