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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, July 8, 2022 9:06 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 v8 9/9] xen: retrieve reserved pages on
> populate_physmap
> 
> On 07.07.2022 11:22, Penny Zheng wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -245,6 +245,29 @@ static void populate_physmap(struct memop_args
> > *a)
> >
> >                  mfn = _mfn(gpfn);
> >              }
> > +            else if ( is_domain_using_staticmem(d) )
> > +            {
> > +                /*
> > +                 * No easy way to guarantee the retrieved pages are 
> > contiguous,
> > +                 * so forbid non-zero-order requests here.
> > +                 */
> > +                if ( a->extent_order != 0 )
> > +                {
> > +                    gdprintk(XENLOG_WARNING,
> > +                             "Cannot allocate static order-%u pages for 
> > static %pd\n",
> > +                             a->extent_order, d);
> 
> I'm probably wrong in thinking that I did point out before that there's no 
> real
> reason to have "static" twice in the message. Or am I mistaken in my
> understanding that only static domains can ever have static pages?
> 

Sorry for omitting the comment, I'll only keep one static here.
You're right, only static domains can have static pages at the moment.

> > @@ -2818,6 +2805,55 @@ 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;
> 
> Use ASSERT_ALLOC_CONTEXT() here as well?
> 

Sure,

> > +    /* 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;
> 
> Don't you need to undo what this did if ...
> 
> > +    if ( assign_domstatic_pages(d, page, 1, memflags) )
> > +        goto fail;
> 
> ... this fails?

Yes, thanks for pointing out.
free_staticmem_pages is needed to do the reversal when it fails

> 
> Jan