| Message ID | 20210715051819.3073628-9-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On 15.07.2021 07:18, Penny Zheng wrote: > @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages( > return pg; > } > > +#ifdef CONFIG_STATIC_MEMORY > +/* > + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of > + * static memory. > + */ > +static struct page_info *acquire_staticmem_pages(unsigned long nr_mfns, > + mfn_t smfn, > + unsigned int memflags) This and the other function not being __init, and there neither being any caller nor any freeing, a question is whether __init wants adding; patch 10 suggests so. The lack of freeing in particular means that domains created using static memory won't be restartable, requiring a host reboot instead. > +{ > + bool need_tlbflush = false; > + uint32_t tlbflush_timestamp = 0; > + unsigned long i; > + struct page_info *pg; > + > + /* For now, it only supports pre-configured static memory. */ > + if ( !mfn_valid(smfn) || !nr_mfns ) > + return NULL; > + > + spin_lock(&heap_lock); > + > + 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). > + */ > + if ( pg[i].count_info != (PGC_state_free | PGC_reserved) ) > + { > + printk(XENLOG_ERR > + "pg[%lu] Static 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(); > + } > + > + 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 = (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)); Indentation. > + } > + > + if ( need_tlbflush ) > + filtered_flush_tlb_mask(tlbflush_timestamp); > + > + spin_unlock(&heap_lock); I'm pretty sure I did comment before on the flush_page_to_ram() being inside the locked region here. While XSA-364 doesn't apply here because you don't defer scrubbing (yet), the flushing may still take too long to happen inside the locked region. Of course there's a dependency here on when exactly the call(s) to this code will happen. In particular if other DomU-s can already be running at that point, this may not be tolerable by some of them. Yet if this is intentional and deemed not a problem, then I'd have kind of expected the description to mention this difference from alloc_heap_pages(), which you say this is an equivalent of. > @@ -2411,6 +2483,42 @@ struct page_info *alloc_domheap_pages( > return pg; > } > > +#ifdef CONFIG_STATIC_MEMORY > +/* > + * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory, > + * then assign them to one specific domain #d. > + */ > +struct page_info *acquire_domstatic_pages( > + struct domain *d, unsigned long nr_mfns, mfn_t smfn, > + unsigned int memflags) > +{ > + struct page_info *pg = NULL; > + > + ASSERT(!in_irq()); > + > + pg = acquire_staticmem_pages(nr_mfns, smfn, memflags); > + if ( !pg ) > + return NULL; > + > + /* Right now, MEMF_no_owner case is meaningless here. */ > + ASSERT(d); > + if ( memflags & MEMF_no_refcount ) > + { > + unsigned long i; > + > + for ( i = 0; i < nr_mfns; i++ ) > + pg[i].count_info |= PGC_extra; > + } With MEMF_no_owner now called out as meaningless here, what about MEMF_no_refcount? Jan
Hi Jan, On 19/07/2021 10:26, Jan Beulich wrote: > On 15.07.2021 07:18, Penny Zheng wrote: >> @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages( >> return pg; >> } >> >> +#ifdef CONFIG_STATIC_MEMORY >> +/* >> + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of >> + * static memory. >> + */ >> +static struct page_info *acquire_staticmem_pages(unsigned long nr_mfns, >> + mfn_t smfn, >> + unsigned int memflags) > > This and the other function not being __init, and there neither being > any caller nor any freeing, a question is whether __init wants adding; > patch 10 suggests so. The lack of freeing in particular means that > domains created using static memory won't be restartable, requiring a > host reboot instead. I am open to request an host reboot in case of an issue. Although, it would be good to have code in place for that. Regardless the reboot part, I would still expect the domain to balloon in/out memory. This is pretty broken today because Xen would end up to give the memory to the heap allocator and the next allocation would be through the heap allocate. For runtime "free/allocate", we may want to follow the same behavior as direct-mapped domain (i.e. GFN == MFN): the page will not be given back to any allocator and we only check if the page belongs to the domain on allocation. So adding __init for acquire_staticmem_pages() is probably fine. On a side node, on v2, I have requested to keep track of the list of missing pieces. @Penny, where can I find the list? Cheers,
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, July 19, 2021 5:26 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH V3 08/10] xen/arm: introduce acquire_staticmem_pages > and acquire_domstatic_pages > > On 15.07.2021 07:18, Penny Zheng wrote: > > @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages( > > return pg; > > } > > > > +#ifdef CONFIG_STATIC_MEMORY > > +/* > > + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of > > + * static memory. > > + */ > > +static struct page_info *acquire_staticmem_pages(unsigned long nr_mfns, > > + mfn_t smfn, > > + unsigned int > > +memflags) > > This and the other function not being __init, and there neither being any caller > nor any freeing, a question is whether __init wants adding; patch 10 suggests > so. The lack of freeing in particular means that domains created using static > memory won't be restartable, requiring a host reboot instead. > Right now, all domain on static allocation get constructed through device tree, in boot-up time. "__init" is preferred. > > +{ > > + bool need_tlbflush = false; > > + uint32_t tlbflush_timestamp = 0; > > + unsigned long i; > > + struct page_info *pg; > > + > > + /* For now, it only supports pre-configured static memory. */ > > + if ( !mfn_valid(smfn) || !nr_mfns ) > > + return NULL; > > + > > + spin_lock(&heap_lock); > > + > > + 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). > > + */ > > + if ( pg[i].count_info != (PGC_state_free | PGC_reserved) ) > > + { > > + printk(XENLOG_ERR > > + "pg[%lu] Static 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(); > > + } > > + > > + 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 = (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)); > > Indentation. > > > + } > > + > > + if ( need_tlbflush ) > > + filtered_flush_tlb_mask(tlbflush_timestamp); > > + > > + spin_unlock(&heap_lock); > > I'm pretty sure I did comment before on the flush_page_to_ram() being inside > the locked region here. While XSA-364 doesn't apply here because you don't > defer scrubbing (yet), the flushing may still take too long to happen inside the > locked region. Of course there's a dependency here on when exactly the call(s) > to this code will happen. In particular if other DomU-s can already be running > at that point, this may not be tolerable by some of them. Yet if this is > intentional and deemed not a problem, then I'd have kind of expected the > description to mention this difference from alloc_heap_pages(), which you say > this is an equivalent of. > Sorry for missing the comments on the flush_page_to_ram() being inside the locked region. Taking a while reading the XSA-364, you're right, especially with asynchronous scrubbing in the to-do list, putting cleaning cache outside of the locked region is necessary here. > > @@ -2411,6 +2483,42 @@ struct page_info *alloc_domheap_pages( > > return pg; > > } > > > > +#ifdef CONFIG_STATIC_MEMORY > > +/* > > + * Acquire nr_mfns contiguous pages, starting at #smfn, of static > > +memory, > > + * then assign them to one specific domain #d. > > + */ > > +struct page_info *acquire_domstatic_pages( > > + struct domain *d, unsigned long nr_mfns, mfn_t smfn, > > + unsigned int memflags) > > +{ > > + struct page_info *pg = NULL; > > + > > + ASSERT(!in_irq()); > > + > > + pg = acquire_staticmem_pages(nr_mfns, smfn, memflags); > > + if ( !pg ) > > + return NULL; > > + > > + /* Right now, MEMF_no_owner case is meaningless here. */ > > + ASSERT(d); > > + if ( memflags & MEMF_no_refcount ) > > + { > > + unsigned long i; > > + > > + for ( i = 0; i < nr_mfns; i++ ) > > + pg[i].count_info |= PGC_extra; > > + } > > With MEMF_no_owner now called out as meaningless here, what about > MEMF_no_refcount? > I could not think of a case where "memflags & MEMF_no_refcount" is needed right now. All acquired pages are for guest RAM right now, extra pages like grant table, etc, are not supported on domain on static allocation, even more, any dom0-less domain. I'll remove this part and put a few comments on it. > Jan Cheers Penny
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Monday, July 19, 2021 6:00 PM > To: Jan Beulich <jbeulich@suse.com>; Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Subject: Re: [PATCH V3 08/10] xen/arm: introduce acquire_staticmem_pages > and acquire_domstatic_pages > > Hi Jan, > > On 19/07/2021 10:26, Jan Beulich wrote: > > On 15.07.2021 07:18, Penny Zheng wrote: > >> @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages( > >> return pg; > >> } > >> > >> +#ifdef CONFIG_STATIC_MEMORY > >> +/* > >> + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of > >> + * static memory. > >> + */ > >> +static struct page_info *acquire_staticmem_pages(unsigned long nr_mfns, > >> + mfn_t smfn, > >> + unsigned int > >> +memflags) > > > > This and the other function not being __init, and there neither being > > any caller nor any freeing, a question is whether __init wants adding; > > patch 10 suggests so. The lack of freeing in particular means that > > domains created using static memory won't be restartable, requiring a > > host reboot instead. > > I am open to request an host reboot in case of an issue. Although, it would be > good to have code in place for that. > > Regardless the reboot part, I would still expect the domain to balloon in/out > memory. This is pretty broken today because Xen would end up to give the > memory to the heap allocator and the next allocation would be through the > heap allocate. > > For runtime "free/allocate", we may want to follow the same behavior as > direct-mapped domain (i.e. GFN == MFN): the page will not be given back to > any allocator and we only check if the page belongs to the domain on > allocation. > > So adding __init for acquire_staticmem_pages() is probably fine. > > On a side node, on v2, I have requested to keep track of the list of missing > pieces. @Penny, where can I find the list? > Oh, sorry... I thought you were requesting a new mail list issue to track all missing pieces in ARM... And a second though here, you shall only mean the missing pieces for this patch serie. I'll do a quick sum-up here and put in the Patch v4 cover letter: TODO: - reboot domain on static allocation. - All memory-ops(hypercalls) regarding domain on static allocation to balloon in/out memory - asynchronously scrubbing PGC_reserved pages - consider domain on static allocation on NUMA-support scenario > Cheers, > > -- > Julien Grall Cheers, -- Penny Zheng
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 3414873679..c9702533f4 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -151,6 +151,10 @@ #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL) #endif +#ifndef CONFIG_STATIC_MEMORY +#define PGC_reserved 0 +#endif + /* * Comma-separated list of hexadecimal page numbers containing bad bytes. * e.g. 'badpage=0x3f45,0x8a321'. @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages( return pg; } +#ifdef CONFIG_STATIC_MEMORY +/* + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of + * static memory. + */ +static struct page_info *acquire_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 pre-configured static memory. */ + if ( !mfn_valid(smfn) || !nr_mfns ) + return NULL; + + spin_lock(&heap_lock); + + 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). + */ + if ( pg[i].count_info != (PGC_state_free | PGC_reserved) ) + { + printk(XENLOG_ERR + "pg[%lu] Static 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(); + } + + 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 = (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); + + spin_unlock(&heap_lock); + + return pg; +} +#endif + /* Remove any offlined page in the buddy pointed to by head. */ static int reserve_offlined_page(struct page_info *head) { @@ -2306,7 +2377,7 @@ int assign_pages( for ( i = 0; i < nr; i++ ) { - ASSERT(!(pg[i].count_info & ~PGC_extra)); + ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved))); if ( pg[i].count_info & PGC_extra ) extra_pages++; } @@ -2345,7 +2416,8 @@ int assign_pages( page_set_owner(&pg[i], d); smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ pg[i].count_info = - (pg[i].count_info & PGC_extra) | PGC_allocated | 1; + (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1; + page_list_add_tail(&pg[i], page_to_list(d, &pg[i])); } @@ -2411,6 +2483,42 @@ struct page_info *alloc_domheap_pages( return pg; } +#ifdef CONFIG_STATIC_MEMORY +/* + * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory, + * then assign them to one specific domain #d. + */ +struct page_info *acquire_domstatic_pages( + struct domain *d, unsigned long nr_mfns, mfn_t smfn, + unsigned int memflags) +{ + struct page_info *pg = NULL; + + ASSERT(!in_irq()); + + pg = acquire_staticmem_pages(nr_mfns, smfn, memflags); + if ( !pg ) + return NULL; + + /* Right now, MEMF_no_owner case is meaningless here. */ + ASSERT(d); + if ( memflags & MEMF_no_refcount ) + { + unsigned long i; + + for ( i = 0; i < nr_mfns; i++ ) + pg[i].count_info |= PGC_extra; + } + if ( assign_pages(d, nr_mfns, pg, memflags) ) + { + free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub); + return NULL; + } + + return pg; +} +#endif + void free_domheap_pages(struct page_info *pg, unsigned int order) { struct domain *d = page_get_owner(pg); diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 65ba1587ad..69e3586d8a 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -89,6 +89,9 @@ bool scrub_free_pages(void); /* These functions are for static memory */ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, bool need_scrub); +struct page_info *acquire_domstatic_pages(struct domain *d, + unsigned long nr_mfns, mfn_t smfn, + unsigned int memflags); #endif /* Map machine page range in Xen virtual address space. */
acquire_staticmem_pages aims to acquire nr_mfns contiguous pages of static memory. And it is the equivalent of alloc_heap_pages for static memory. Here only covers acquiring pre-configured static memory. 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. acquire_domstatic_pages is the equivalent of alloc_domheap_pages for static memory, and it is to acquire nr_mfns contiguous pages of static memory and assign them to one specific domain. It uses acquire_staticmem_pages to acquire nr_mfns pre-configured pages of static memory, then on success, it will use assign_pages to assign those pages to one specific domain. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v3 change: - Assuming caller knows the static memory range is reserved (and free), change name from alloc_staticmem_pages/alloc_domstatic_pages to acquire_staticmem_pages and acquire_domstatic_pages. - proper locking moved from the next commit to here. - remove and refine extra verbosity log - remove hunks' #ifdef-ary by introducing PGC_reserved = 0 - remove DMA restriction --- xen/common/page_alloc.c | 112 +++++++++++++++++++++++++++++++++++++++- xen/include/xen/mm.h | 3 ++ 2 files changed, 113 insertions(+), 2 deletions(-)