RE: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, May 18, 2021 6:30 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
> 
> Hi Penny,
> 
> Title: s/intruduce/introduce/
> 

Thx~

> On 18/05/2021 06:21, Penny Zheng wrote:
> > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
> > static mmeory, and it is to allocate nr_pfns pages of static memory
> > and assign them to one specific domain.
> >
> > It uses alloc_staticmen_pages to get nr_pages pages of static memory,
> > then on success, it will use assign_pages to assign those pages to one
> > specific domain, including using page_set_reserved_owner to set its
> > reserved domain owner.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   xen/common/page_alloc.c | 53
> +++++++++++++++++++++++++++++++++++++++++
> >   xen/include/xen/mm.h    |  4 ++++
> >   2 files changed, 57 insertions(+)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 0eb9f22a00..f1f1296a61 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2447,6 +2447,9 @@ int assign_pages(
> >       {
> >           ASSERT(page_get_owner(&pg[i]) == NULL);
> >           page_set_owner(&pg[i], d);
> > +        /* use page_set_reserved_owner to set its reserved domain owner.
> */
> > +        if ( (pg[i].count_info & PGC_reserved) )
> > +            page_set_reserved_owner(&pg[i], d);
> 
> I have skimmed through the rest of the series and couldn't find anyone
> calling page_get_reserved_owner(). The value is also going to be the exact
> same as page_set_owner().
> 
> So why do we need it?
> 

In my first intent, This two helper page_get_reserved_owner/ 
page_set_reserved_owner
and the new field `reserved` in page_info are all for rebooting domain on 
static allocation. 

I was considering that, when implementing rebooting domain on static 
allocation, memory
will be relinquished and right now, all freed back to heap, which is not 
suitable for static memory here.
` relinquish_memory(d, &d->page_list)  --> put_page -->  free_domheap_page`

For pages in PGC_reserved, now, I am considering that, other than giving it 
back to heap, maybe creating
a new global `struct page_info*[DOMID]` value to hold.

So it is better to have a new field in struct page_info, as follows, to hold 
such info.

/* Page is reserved. */
struct {
    /*
     * Reserved Owner of this page,
     * if this page is reserved to a specific domain.
     */
    domid_t reserved_owner;
} reserved;

But this patch Serie is not going to include this feature, and I will delete 
related helpers and values.

> >           smp_wmb(); /* Domain pointer must be visible before updating
> refcnt. */
> >           pg[i].count_info =
> >               (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> 
> This will clobber PGC_reserved.
> 

related changes have been included into the commit of 
"0008-xen-arm-introduce-reserved_page_list.patch".
 
> > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
> >       return pg;
> >   }
> >
> > +/*
> > + * Allocate nr_pfns contiguous pages, starting at #start, of static
> > +memory,
> 
> s/nr_pfns/nr_mfns/
> 

Sure.

> > + * then assign them to one specific domain #d.
> > + * It is the equivalent of alloc_domheap_pages for static memory.
> > + */
> > +struct page_info *alloc_domstatic_pages(
> > +        struct domain *d, unsigned long nr_pfns, paddr_t start,
> 
> s/nr_pfns/nf_mfns/. Also, I would the third parameter to be an mfn_t.
> 

Sure.

> > +        unsigned int memflags)
> > +{
> > +    struct page_info *pg = NULL;
> > +    unsigned long dma_size;
> > +
> > +    ASSERT(!in_irq());
> > +
> > +    if ( memflags & MEMF_no_owner )
> > +        memflags |= MEMF_no_refcount;
> > +
> > +    if ( !dma_bitsize )
> > +        memflags &= ~MEMF_no_dma;
> > +    else
> > +    {
> > +        dma_size = 1ul << bits_to_zone(dma_bitsize);
> > +        /* Starting address shall meet the DMA limitation. */
> > +        if ( dma_size && start < dma_size )
> > +            return NULL;
> > +    }
> > +
> > +    pg = alloc_staticmem_pages(nr_pfns, start, memflags);
> > +    if ( !pg )
> > +        return NULL;
> > +
> > +    if ( d && !(memflags & MEMF_no_owner) )
> > +    {
> > +        if ( memflags & MEMF_no_refcount )
> > +        {
> > +            unsigned long i;
> > +
> > +            for ( i = 0; i < nr_pfns; i++ )
> > +                pg[i].count_info = PGC_extra;
> > +        }
> > +        if ( assign_pages(d, pg, nr_pfns, memflags) )
> > +        {
> > +            free_staticmem_pages(pg, nr_pfns, memflags & MEMF_no_scrub);
> > +            return NULL;
> > +        }
> > +    }
> > +
> > +    return pg;
> > +}
> > +
> >   void free_domheap_pages(struct page_info *pg, unsigned int order)
> >   {
> >       struct domain *d = page_get_owner(pg); diff --git
> > a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > dcf9daaa46..e45987f0ed 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -111,6 +111,10 @@ unsigned long __must_check
> domain_adjust_tot_pages(struct domain *d,
> >   int domain_set_outstanding_pages(struct domain *d, unsigned long
> pages);
> >   void get_outstanding_claims(uint64_t *free_pages, uint64_t
> > *outstanding_pages);
> >
> > +/* Static Memory */
> > +struct page_info *alloc_domstatic_pages(struct domain *d,
> > +        unsigned long nr_pfns, paddr_t start, unsigned int memflags);
> > +
> >   /* Domain suballocator. These functions are *not* interrupt-safe.*/
> >   void init_domheap_pages(paddr_t ps, paddr_t pe);
> >   struct page_info *alloc_domheap_pages(
> >
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

Penny