Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.06.2021 04:43, Penny Zheng wrote:
> alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of
> static memory. And it is the equivalent of alloc_heap_pages for static
> memory. Here only covers allocating at specified starting address.
> 
> For each page, it shall check if the page is reserved(PGC_reserved)
> and free. It shall also do a set of necessary initialization, which are
> mostly the same ones in alloc_heap_pages, like, following the same
> cache-coherency policy and turning page status into PGC_state_inuse, etc.
> 
> alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
> static mmeory, and it is to allocate nr_mfns pages of static memory
> and assign them to one specific domain.
> 
> It uses alloc_staticmen_pages to get nr_mfns pages of static memory,
> then on success, it will use assign_pages_nr to assign those pages to
> one specific domain.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> changes v2:
> - use mfn_valid() to do validation
> - change pfn-named to mfn-named
> - put CONFIG_STATIC_ALLOCATION around to remove dead codes
> - correct off-by-one indentation
> - remove meaningless MEMF_no_owner case
> - leave zone concept out of DMA limitation check
> ---
>  xen/common/page_alloc.c | 129 ++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/mm.h    |   2 +
>  2 files changed, 131 insertions(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index e244d2e52e..a0eea5f1a4 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/*
> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
> + * It is the equivalent of alloc_heap_pages for static memory
> + */
> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns,
> +                                               mfn_t smfn,
> +                                               unsigned int memflags)
> +{
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
> +    unsigned long i;
> +    struct page_info *pg;
> +
> +    /* For now, it only supports allocating at specified address. */
> +    if ( !mfn_valid(smfn) || !nr_mfns )
> +    {
> +        printk(XENLOG_ERR
> +               "Invalid %lu static memory starting at %"PRI_mfn"\n",

Reading a log containing e.g. "Invalid 0 static memory starting at
..." I don't think I would recognize that the "0" is the count of
pages.

> +               nr_mfns, mfn_x(smfn));
> +        return NULL;
> +    }
> +    pg = mfn_to_page(smfn);
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +    {
> +        /*
> +         * Reference count must continuously be zero for free pages
> +         * of static memory(PGC_reserved).
> +         */
> +        ASSERT(pg[i].count_info & PGC_reserved);

What logic elsewhere guarantees that this will hold? ASSERT()s are
to verify that assumptions are met. But I don't think you can
sensibly assume the caller knows the range is reserved (and free),
or else you could get away without any allocation function.

> +        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
> +        {
> +            printk(XENLOG_ERR
> +                   "Reference count must continuously be zero for free pages"
> +                   "pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x\n",
> +                   i, mfn_x(page_to_mfn(pg + i)),
> +                   pg[i].count_info, pg[i].tlbflush_timestamp);
> +            BUG();
> +        }

The same applies here at least until proper locking gets added,
which I guess is happening in the next patch when really it would
need to happen right here.

Furthermore I don't see why you don't fold ASSERT() and if into

        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )

After all PGC_reserved is not similar to PGC_need_scrub, which
alloc_heap_pages() masks out the way you also have it here.

As to the printk() - the extra verbosity compared to the original
isn't helpful or necessary imo. The message needs to be
distinguishable from the other one, yes, so it would better
mention "static" in some way. But the prefix you have is too long
for my taste (and lacks a separating blank anyway).

As a separate matter - have you given up on the concept of
reserving particular memory ranges for _particular_ guests? The
cover letter, saying "Static Allocation refers to system or
sub-system(domains) for which memory areas are pre-defined by
configuration using physical address ranges" as the very first
thing, doesn't seem to suggest so.

> +        if ( !(memflags & MEMF_no_tlbflush) )
> +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> +                                &tlbflush_timestamp);
> +
> +        /*
> +         * Preserve flag PGC_reserved and change page state
> +         * to PGC_state_inuse.
> +         */
> +        pg[i].count_info = (pg[i].count_info & PGC_reserved) | 
> PGC_state_inuse;

Why not

        pg[i].count_info = PGC_state_inuse | PGC_reserved;

? Again, PGC_reserved is sufficiently different from PGC_need_scrub.

> +        /* Initialise fields which have other uses for free pages. */
> +        pg[i].u.inuse.type_info = 0;
> +        page_set_owner(&pg[i], NULL);
> +
> +        /*
> +         * Ensure cache and RAM are consistent for platforms where the
> +         * guest can control its own visibility of/through the cache.
> +         */
> +        flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])),
> +                            !(memflags & MEMF_no_icache_flush));
> +    }
> +
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);

Depending on whether static pages have a designated owner, this may
(as suggested before) not be necessary.

> @@ -2326,7 +2395,11 @@ int assign_pages_nr(
>  
>          for ( i = 0; i < nr_pfns; i++ )
>          {
> +#ifdef CONFIG_STATIC_ALLOCATION
> +            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved)));
> +#else
>              ASSERT(!(pg[i].count_info & ~PGC_extra));
> +#endif
>              if ( pg[i].count_info & PGC_extra )
>                  extra_pages++;
>          }
> @@ -2365,7 +2438,12 @@ int assign_pages_nr(
>          page_set_owner(&pg[i], d);
>          smp_wmb(); /* Domain pointer must be visible before updating refcnt. 
> */
>          pg[i].count_info =
> +#ifdef CONFIG_STATIC_ALLOCATION
> +            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated 
> | 1;
> +#else
>              (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> +#endif

Both hunks' #ifdef-ary needs to be avoided, e.g. by

#ifndef CONFIG_STATIC_ALLOCATION
# define PGC_reserved 0
#endif

near the top of the file.

> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/*
> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory,
> + * 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_mfns, mfn_t smfn,
> +        unsigned int memflags)
> +{
> +    struct page_info *pg = NULL;
> +    unsigned long dma_size;
> +
> +    ASSERT(!in_irq());
> +
> +    if ( !dma_bitsize )
> +        memflags &= ~MEMF_no_dma;
> +    else
> +    {
> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
> +        {
> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
> +            /* Starting address shall meet the DMA limitation. */
> +            if ( mfn_x(smfn) < dma_size )
> +                return NULL;

I think I did ask this on v1 already: Why the first page? Static
memory regions, unlike buddy allocator zones, can cross power-of-2
address boundaries. Hence it ought to be the last page that gets
checked for fitting address width restriction requirements.

And then - is this necessary at all? Shouldn't "pre-defined by
configuration using physical address ranges" imply the memory
designated for a guest fits its DMA needs?

Jan