Hi Julien > -----Original Message----- > From: Penny Zheng > Sent: Wednesday, May 19, 2021 1:24 PM > To: Julien Grall <jul...@xen.org>; xen-devel@lists.xenproject.org; > sstabell...@kernel.org > Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen > <wei.c...@arm.com>; nd <n...@arm.com> > Subject: RE: [PATCH 05/10] xen/arm: introduce alloc_staticmem_pages > > Hi Julien > > > -----Original Message----- > > From: Julien Grall <jul...@xen.org> > > Sent: Tuesday, May 18, 2021 6:15 PM > > To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org; > > sstabell...@kernel.org > > Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen > > <wei.c...@arm.com>; nd <n...@arm.com> > > Subject: Re: [PATCH 05/10] xen/arm: introduce alloc_staticmem_pages > > > > Hi Penny, > > > > On 18/05/2021 06: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.zh...@arm.com> > > > --- > > > 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, > > > > This wants to be nr_mfns. > > > > > + paddr_t start, > > > > I would prefer if this helper takes an mfn_t in parameter. > > > > Sure, I will change both. > > > > + unsigned int > > > +memflags) { > > > + bool need_tlbflush = false; > > > + uint32_t tlbflush_timestamp = 0; > > > + unsigned int i; > > > + 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); > > > > We should avoid to make the assumption the start address will be valid. > > So you want to call mfn_valid() first. > > > > At the same time, there is no guarantee that if the first page is > > valid, then the next nr_pfns will be. So the check should be performed for > > all > of them. > > > > Ok. I'll do validation check on both of them. > > > > + 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. > > > + } > > > + > > > + if ( !(memflags & MEMF_no_tlbflush) ) > > > + accumulate_tlbflush(&need_tlbflush, &pg[i], > > > + &tlbflush_timestamp); > > > + > > > + /* > > > + * Reserve flag PGC_reserved and change page state > > > + * 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); > > > + > > > + return pg; > > > +} > > > + > > > /* Remove any offlined page in the buddy pointed to by head. */ > > > static int reserve_offlined_page(struct page_info *head) > > > { > > > > > > > Cheers, > > > > -- > > Julien Grall > > Cheers, > > Penny Zheng Cheers Penny