RE: [PATCH v9 8/8] xen: retrieve reserved pages on populate_physmap

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

Sorry for the delayed response.

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan
> Beulich
> Sent: Monday, July 25, 2022 11:44 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v9 8/8] xen: retrieve reserved pages on
> populate_physmap
> 
> On 20.07.2022 07:46, Penny Zheng wrote:
> > When a static domain populates memory through populate_physmap at
> > runtime, it shall retrieve reserved pages from resv_page_list to make
> > sure that guest RAM is still restricted in statically configured memory
> regions.
> > This commit also introduces a new helper acquire_reserved_page to make
> it work.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> > v9 changes:
> > - Use ASSERT_ALLOC_CONTEXT() in acquire_reserved_page
> > - Add free_staticmem_pages to undo prepare_staticmem_pages when
> > assign_domstatic_pages fails
> 
> May I suggest to re-consider naming of the various functions? Undoing what
> "prepare" did by "free" is, well, counterintuitive.
> 

How about change the name "prepare_staticmem_pages" to 
"allocate_staticmem_pages"?

> > +/*
> > + * 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:
> > +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
> > + fail:
> > +    page_list_add_tail(page, &d->resv_page_list);
> > +    return INVALID_MFN;
> > +}
> 
> What about locking on this error path?
> 

Right, locking is needed here too:
fail:
   spin_lock(&d->page_alloc_lock);
   page_list_add_tail(page, &d->resv_page_list);
   spin_unlock(&d->page_alloc_lock);

> Jan