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

[Penny Zheng <Penny.Zheng@arm.com>]

Hi jan 

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, August 17, 2022 6:05 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v10 8/9] xen: retrieve reserved pages on
> populate_physmap
> 
> On 16.08.2022 04:36, Penny Zheng wrote:
> > @@ -2867,6 +2854,61 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >      return 0;
> >  }
> > +
> > +/*
> > + * Acquire nr_mfns contiguous pages, starting at #smfn, of static
> > +memory,
> > + * then assign them to one specific domain #d.
> > + */
> > +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> > +                                   unsigned int nr_mfns, unsigned int
> > +memflags) {
> > +    struct page_info *pg;
> > +
> > +    ASSERT_ALLOC_CONTEXT();
> > +
> > +    pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
> > +    if ( !pg )
> > +        return -ENOENT;
> > +
> > +    if ( assign_domstatic_pages(d, pg, nr_mfns, memflags) )
> > +        return -EINVAL;
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * 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);
> 
> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with

I got a bit confused about this flag MEMF_no_scrub, does it mean no need
to scrub? 
Since I saw that in alloc_domheap_pages(...)
    if ( assign_page(pg, order, d, memflags) )
    {
        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
        return NULL;
    }
It doesn't contain exclamation mark too...

> assignment having failed and with it being just a single page we're talking
> about, the page was not exposed to the guest at any point afaict. So I don't
> see the need for scrubbing in the first place.
> 
> Also I think the rename of the function would better be done first, since then
> you wouldn't need to touch this line again right in the next patch, and the
> prepare/unprepare pairing would also be visible right here. This would then
> also better fit with the introduction of prepare_*() in the previous patch
> (which, afaic, the name change could also be merged into; FTAOD I don't
> mind it to be separate).
> 
> Jan