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

[Julien Grall <julien@xxxxxxx>]

On 24/05/2021 11:10, Penny Zheng wrote:
> Hi Julien
Hi Penny,

> > > > +    if ( !pg )
> > > > +        return NULL;
> > > > +
> > > > +    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);
> > > > +            BUG();
> > > So we would crash Xen if the caller pass a wrong range. Is it what we want?
> > >
> > > Also, who is going to prevent concurrent access?
> > Sure, to fix concurrency issue, I may need to add one spinlock like `static
> > DEFINE_SPINLOCK(staticmem_lock);`
> >
> > In current alloc_heap_pages, it will do similar check, that pages in free state
> > MUST have zero reference count. I guess, if condition not met, there is no need
> > to proceed.
> Another thought on concurrency problem, when constructing patch v2, do we need 
> to
> consider concurrency here?
> heap_lock is to take care concurrent allocation on the one heap, but static 
> memory is
> always reserved for only one specific domain.
In theory yes, but you are relying on the admin to correctly write the 
device-tree nodes.
You are probably not going to hit the problem today because the domains 
are created one by one. But, as you may want to allocate memory at 
runtime, this is quite important to get the code protected from 
concurrent access.
Here, you will likely want to use the heaplock rather than a new lock. 
So you are also protect against concurrent access to count_info from 
other part of Xen.

Cheers,

--
Julien Grall