| Message ID | 20210518052113.725808-7-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On 18.05.2021 07:21, Penny Zheng wrote: > Function parameter order in assign_pages is always used as 1ul << order, > referring to 2@order pages. > > Now, for better compatibility with new static memory, order shall > be replaced with nr_pfns pointing to page count with no constraint, > like 250MB. While I'm not entirely opposed, I'm also not convinced. The new user could as well break up the range into suitable power-of-2 chunks. In no case do I view the wording "compatibility" here as appropriate. There's no incompatibility at present. Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, May 18, 2021 3:28 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 06/10] xen: replace order with nr_pfns in assign_pages > for better compatibility > > On 18.05.2021 07:21, Penny Zheng wrote: > > Function parameter order in assign_pages is always used as 1ul << > > order, referring to 2@order pages. > > > > Now, for better compatibility with new static memory, order shall be > > replaced with nr_pfns pointing to page count with no constraint, like > > 250MB. > > While I'm not entirely opposed, I'm also not convinced. The new user could > as well break up the range into suitable power-of-2 chunks. In no case do I > view the wording "compatibility" here as appropriate. There's no > incompatibility at present. > Yes, maybe the incompatibility is not the good choice here. Sure, the new user definitely could choose the workaround to break up the range, while it may cost extra time. And while considering MPU system, memory range size is often not in the power-of-2. > Jan Thanks Penny
Hi Penny, On 18/05/2021 06:21, Penny Zheng wrote: > Function parameter order in assign_pages is always used as 1ul << order, > referring to 2@order pages. > > Now, for better compatibility with new static memory, order shall > be replaced with nr_pfns pointing to page count with no constraint, > like 250MB. We have similar requirements for LiveUpdate because are preserving the memory with a number of pages (rather than a power-of-two). With the current interface would be need to split the range in a power of 2 which is a bit of pain. However, I think I would prefer if we introduce a new interface (maybe assign_pages_nr()) rather than change the meaning of the field. This is for two reasons: 1) We limit the risk to make mistake when backporting a patch touch assign_pages(). 2) Adding (1UL << order) for pretty much all the caller is not nice. Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Tuesday, May 18, 2021 6:21 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com> > Subject: Re: [PATCH 06/10] xen: replace order with nr_pfns in assign_pages > for better compatibility > > Hi Penny, > > On 18/05/2021 06:21, Penny Zheng wrote: > > Function parameter order in assign_pages is always used as 1ul << > > order, referring to 2@order pages. > > > > Now, for better compatibility with new static memory, order shall be > > replaced with nr_pfns pointing to page count with no constraint, like > > 250MB. > > We have similar requirements for LiveUpdate because are preserving the > memory with a number of pages (rather than a power-of-two). With the > current interface would be need to split the range in a power of 2 which is a > bit of pain. > > However, I think I would prefer if we introduce a new interface (maybe > assign_pages_nr()) rather than change the meaning of the field. This is for > two reasons: > 1) We limit the risk to make mistake when backporting a patch touch > assign_pages(). > 2) Adding (1UL << order) for pretty much all the caller is not nice. > Ok. I will create a new interface assign_pages_nr(), and let assign_pages to call it with 2@order. > Cheers, > > -- > Julien Grall Cheers Penny Zheng
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index e0801a9e6d..4e57836763 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d, else { while ( count-- ) - if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) + if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 1, 0) ) BUG(); } initrd->mod_end = 0; diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index ab30e2e8cf..925bf924bd 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -2354,7 +2354,7 @@ gnttab_transfer( * is respected and speculative execution is blocked accordingly */ if ( unlikely(!evaluate_nospec(okay)) || - unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) ) + unlikely(assign_pages(e, page, 1, MEMF_no_refcount)) ) { bool drop_dom_ref; diff --git a/xen/common/memory.c b/xen/common/memory.c index b5c70c4b85..2dca23aa7f 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -722,7 +722,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) /* Assign each output page to the domain. */ for ( j = 0; (page = page_list_remove_head(&out_chunk_list)); ++j ) { - if ( assign_pages(d, page, exch.out.extent_order, + if ( assign_pages(d, page, 1UL << exch.out.extent_order, MEMF_no_refcount) ) { unsigned long dec_count; @@ -791,7 +791,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) * cleared PGC_allocated. */ while ( (page = page_list_remove_head(&in_chunk_list)) ) - if ( assign_pages(d, page, 0, MEMF_no_refcount) ) + if ( assign_pages(d, page, 1, MEMF_no_refcount) ) { BUG_ON(!d->is_dying); free_domheap_page(page); diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index adf2889e76..0eb9f22a00 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2388,7 +2388,7 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) int assign_pages( struct domain *d, struct page_info *pg, - unsigned int order, + unsigned long nr_pfns, unsigned int memflags) { int rc = 0; @@ -2408,7 +2408,7 @@ int assign_pages( { unsigned int extra_pages = 0; - for ( i = 0; i < (1ul << order); i++ ) + for ( i = 0; i < nr_pfns; i++ ) { ASSERT(!(pg[i].count_info & ~PGC_extra)); if ( pg[i].count_info & PGC_extra ) @@ -2417,18 +2417,18 @@ int assign_pages( ASSERT(!extra_pages || ((memflags & MEMF_no_refcount) && - extra_pages == 1u << order)); + extra_pages == nr_pfns)); } #endif if ( pg[0].count_info & PGC_extra ) { - d->extra_pages += 1u << order; + d->extra_pages += nr_pfns; memflags &= ~MEMF_no_refcount; } else if ( !(memflags & MEMF_no_refcount) ) { - unsigned int tot_pages = domain_tot_pages(d) + (1 << order); + unsigned int tot_pages = domain_tot_pages(d) + nr_pfns; if ( unlikely(tot_pages > d->max_pages) ) { @@ -2440,10 +2440,10 @@ int assign_pages( } if ( !(memflags & MEMF_no_refcount) && - unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) ) + unlikely(domain_adjust_tot_pages(d, nr_pfns) == nr_pfns) ) get_knownalive_domain(d); - for ( i = 0; i < (1 << order); i++ ) + for ( i = 0; i < nr_pfns; i++ ) { ASSERT(page_get_owner(&pg[i]) == NULL); page_set_owner(&pg[i], d); @@ -2499,7 +2499,7 @@ struct page_info *alloc_domheap_pages( pg[i].count_info = PGC_extra; } } - if ( assign_pages(d, pg, order, memflags) ) + if ( assign_pages(d, pg, 1ul << order, memflags) ) { free_heap_pages(pg, order, memflags & MEMF_no_scrub); return NULL; diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 8b1a2207b2..dcf9daaa46 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -131,7 +131,7 @@ void heap_init_late(void); int assign_pages( struct domain *d, struct page_info *pg, - unsigned int order, + unsigned long nr_pfns, unsigned int memflags); /* Dump info to serial console */
Function parameter order in assign_pages is always used as 1ul << order, referring to 2@order pages. Now, for better compatibility with new static memory, order shall be replaced with nr_pfns pointing to page count with no constraint, like 250MB. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/arch/x86/pv/dom0_build.c | 2 +- xen/common/grant_table.c | 2 +- xen/common/memory.c | 4 ++-- xen/common/page_alloc.c | 16 ++++++++-------- xen/include/xen/mm.h | 2 +- 5 files changed, 13 insertions(+), 13 deletions(-)