RE: [PATCH 05/10] xen/arm: introduce alloc_staticmem_pages

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, May 18, 2021 3:24 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH 05/10] xen/arm: introduce alloc_staticmem_pages
> 
> On 18.05.2021 07:21, Penny Zheng wrote:
> > alloc_staticmem_pages is designated to allocate nr_pfns contiguous
> > pages of static memory. And it is the equivalent of alloc_heap_pages
> > for static memory.
> > This commit 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_used, etc.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >  xen/common/page_alloc.c | 64
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 58b53c6ac2..adf2889e76 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1068,6 +1068,70 @@ static struct page_info *alloc_heap_pages(
> >      return pg;
> >  }
> >
> > +/*
> > + * Allocate nr_pfns contiguous pages, starting at #start, of static memory.
> > + * It is the equivalent of alloc_heap_pages for static memory  */
> > +static struct page_info *alloc_staticmem_pages(unsigned long nr_pfns,
> > +                                                paddr_t start,
> > +                                                unsigned int
> > +memflags)
> 
> This is surely breaking the build (at this point in the series - recall that 
> a series
> should build fine at every patch boundary), for introducing an unused static
> function, which most compilers will warn about.
>

Sure, I'll combine it with other commits

> Also again - please avoid introducing code that's always dead for certain
> architectures. Quite likely you want a Kconfig option to put a suitable #ifdef
> around such functions.
> 

Sure, sorry for all the missing #ifdefs.

> And a nit: Please correct the apparently off-by-one indentation.
>

Sure, I'll check through the code more carefully.

> > +{
> > +    bool need_tlbflush = false;
> > +    uint32_t tlbflush_timestamp = 0;
> > +    unsigned int i;
> 
> This variable's type should (again) match nr_pfns'es (albeit I think that
> parameter really wants to be nr_mfns).
> 

Correct if I understand you wrongly, you mean that parameters in 
alloc_staticmem_pages
are better be named after unsigned long nr_mfns, right?

> > +    struct page_info *pg;
> > +    mfn_t s_mfn;
> > +
> > +    /* For now, it only supports allocating at specified address. */
> > +    s_mfn = maddr_to_mfn(start);
> > +    pg = mfn_to_page(s_mfn);
> > +    if ( !pg )
> > +        return NULL;
> 
> Under what conditions would mfn_to_page() return NULL?

Right, my mistake.

>
> > +    for ( i = 0; i < nr_pfns; i++)
> > +    {
> > +        /*
> > +         * Reference count must continuously be zero for free pages
> > +         * of static memory(PGC_reserved).
> > +         */
> > +        ASSERT(pg[i].count_info & PGC_reserved);
> > +        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
> > +        {
> > +            printk(XENLOG_ERR
> > +                    "Reference count must continuously be zero for free 
> > pages"
> > +                    "pg[%u] MFN %"PRI_mfn" c=%#lx t=%#x\n",
> > +                    i, mfn_x(page_to_mfn(pg + i)),
> > +                    pg[i].count_info, pg[i].tlbflush_timestamp);
> 
> Nit: Indentation again.
>
 
Thx

> > +            BUG();
> > +        }
> > +
> > +        if ( !(memflags & MEMF_no_tlbflush) )
> > +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> > +                                &tlbflush_timestamp);
> > +
> > +        /*
> > +         * Reserve flag PGC_reserved and change page state
> 
> DYM "Preserve ..."?
> 

Sure, thx

> > +         * to PGC_state_inuse.
> > +         */
> > +        pg[i].count_info = (pg[i].count_info & PGC_reserved) |
> PGC_state_inuse;
> > +        /* 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);
> 
> With reserved pages dedicated to a specific domain, in how far is it possible
> that stale mappings from a prior use can still be around, making such TLB
> flushing necessary?
> 

Yes, you're right.

> Jan