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

[Julien Grall <julien@xen.org>]

Hi Penny,

Title: s/intruduce/introduce/

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@arm.com>
> ---
>   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?

>           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.

> @@ -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/

> + * 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.

> +        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