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

From: Penny Zheng <Penny.Zheng@arm.com>
To: Jan Beulich <jbeulich@suse.com>, Julien Grall <julien@xen.org>
Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Wei Chen <Wei.Chen@arm.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>
Subject: RE: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages
Date: Tue, 6 Jul 2021 05:58:57 +0000	[thread overview]
Message-ID: <VE1PR08MB5215E2802F3DE22F1F244023F71B9@VE1PR08MB5215.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <c31a85c3-89ea-76a4-3b29-a411d419fb59@suse.com>

Hi

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, June 10, 2021 6:23 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; julien@xen.org
> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
> alloc_domstatic_pages
> 
> 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@arm.com>
> > ---
> > 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.
> 

Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?

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

The caller shall only call alloc_staticmem_pages when it knows range is reserved,
like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages
for now.

Normal domain uses alloc_heap_pages/alloc_domheap_pages to do the allocation.

Proper initialization must happen before allocation. Init_staticmem_pages shall be
called before alloc_staticmem_pages. And we set PGC_reserved in init_staticmem_pages.

So here I use ASSERT()s to check whether above proper initialization is met.

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

Ok, I will combine two commits together, and add locking here. 
I thought the content of this commit is a little bit too much, so maybe
adding the proper lock shall be created as a new patch. 😉

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

I understand that you prefer if condition is phrased as follows:
 	if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
Agree that PGC_reserved shall has the same position as PGC_state_free.
Hmmm, for why I don't fold ASSERT(), do you mean that 
ASSERT(pg[i].count_info == (PGC_state_free | PGC_reserved))?

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

If you don't like the extra verbosity, maybe just
" Static pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x.\n"?

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

Yeah, I may take suggestion from Julien to retrieve reserved mem info from device tree
on rebooting. So it may not need store domain info in struct page_info.

And also for rebooting domain on static allocation, it will not be introduced in this patch serie.
This patch serie may only focus on initialization and allocation part.

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

Sure. Thanks. You're right.

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

This has also been discussed in patch v1~ Julien has replied on it, here may just refer
what he said:

"
I would rather not make the assumption. I can see future where we just 
want to allocate memory from a static pool that may be shared with 
multiple domains.
"

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

Ok, it may also help remove the #ifdefs in make_page_free, thx!

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

Hmmm, In my understanding, here is the DMA restriction when using buddy allocator:
    else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
        pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);
dma_zone is restricting the starting buddy allocator zone, so I am thinking that here, it shall
restrict the first page.

imo, if let user define, it also could be missing DMA restriction? If you all think its not
necessary, I'll remove it in v3~~~

> Jan

Cheers
Penny Zheng