In order to deal with the trouble of count-to-order conversion when page number
is not in a power-of-two, this commit re-define assign_pages for nr pages and
assign_page for original page with a single order.
Backporting confusion could be helped by altering the order of assign_page
parameters, such that the compiler would point out that adjustments at call
sites are needed.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 change:
- in all cases where order-0 pages get passed, prefer using assign_pages
to pass literal 1
- reconstruct the order of assign_pages parameters
- remove the unnecessary parentheses
---
xen/arch/x86/pv/dom0_build.c | 2 +-
xen/common/grant_table.c | 2 +-
xen/common/memory.c | 4 ++--
xen/common/page_alloc.c | 23 ++++++++++++++---------
xen/include/xen/mm.h | 6 ++++++
5 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index af47615b22..9142f359da 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(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
BUG();
}
initrd->mod_end = 0;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fab77ab9cc..1f6b89bff4 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2342,7 +2342,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(page, 1, e, MEMF_no_refcount)) )
{
bool drop_dom_ref;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index e07bd9a5ea..083e14b84f 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -728,7 +728,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_page(d, page, exch.out.extent_order,
MEMF_no_refcount) )
{
unsigned long dec_count;
@@ -797,7 +797,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(page, 1, d, 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 f51e406401..e279c6f713 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2282,9 +2282,9 @@ 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,
+ struct domain *d,
unsigned int memflags)
{
int rc = 0;
@@ -2304,7 +2304,7 @@ int assign_pages(
{
unsigned int extra_pages = 0;
- for ( i = 0; i < (1ul << order); i++ )
+ for ( i = 0; i < nr; i++ )
{
ASSERT(!(pg[i].count_info & ~PGC_extra));
if ( pg[i].count_info & PGC_extra )
@@ -2313,18 +2313,18 @@ int assign_pages(
ASSERT(!extra_pages ||
((memflags & MEMF_no_refcount) &&
- extra_pages == 1u << order));
+ extra_pages == nr));
}
#endif
if ( pg[0].count_info & PGC_extra )
{
- d->extra_pages += 1u << order;
+ d->extra_pages += nr;
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;
if ( unlikely(tot_pages > d->max_pages) )
{
@@ -2336,10 +2336,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) == nr) )
get_knownalive_domain(d);
- for ( i = 0; i < (1 << order); i++ )
+ for ( i = 0; i < nr; i++ )
{
ASSERT(page_get_owner(&pg[i]) == NULL);
page_set_owner(&pg[i], d);
@@ -2354,6 +2354,11 @@ int assign_pages(
return rc;
}
+int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
+ unsigned int memflags)
+{
+ return assign_pages(pg, 1UL << order, d, memflags);
+}
struct page_info *alloc_domheap_pages(
struct domain *d, unsigned int order, unsigned int memflags)
@@ -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages(
pg[i].count_info = PGC_extra;
}
}
- if ( assign_pages(d, pg, order, memflags) )
+ if ( assign_page(d, pg, 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 8e8fb5a615..2e75cdcbb7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
void heap_init_late(void);
int assign_pages(
+ struct page_info *pg,
+ unsigned long nr,
+ struct domain *d,
+ unsigned int memflags);
+
+int assign_page(
struct domain *d,
struct page_info *pg,
unsigned int order,
--
2.25.1
On 28.07.2021 12:27, Penny Zheng wrote: > In order to deal with the trouble of count-to-order conversion when page number > is not in a power-of-two, this commit re-define assign_pages for nr pages and > assign_page for original page with a single order. > > Backporting confusion could be helped by altering the order of assign_page > parameters, such that the compiler would point out that adjustments at call > sites are needed. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> Largely to indicate my main concerns were addressed, yet not so much to indicate I'm actually happy with the introduction of the 2nd function: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Penny,
On 28/07/2021 11:27, Penny Zheng wrote:
> In order to deal with the trouble of count-to-order conversion when page number
> is not in a power-of-two, this commit re-define assign_pages for nr pages and
> assign_page for original page with a single order.
>
> Backporting confusion could be helped by altering the order of assign_page
> parameters, such that the compiler would point out that adjustments at call
> sites are needed.
Looking at the code, you don't alter the order of assign_page()
parameters. So did you mean to refer to "assign_pages()"?
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v4 change:
> - in all cases where order-0 pages get passed, prefer using assign_pages
> to pass literal 1
> - reconstruct the order of assign_pages parameters
> - remove the unnecessary parentheses
> ---
> xen/arch/x86/pv/dom0_build.c | 2 +-
> xen/common/grant_table.c | 2 +-
> xen/common/memory.c | 4 ++--
> xen/common/page_alloc.c | 23 ++++++++++++++---------
> xen/include/xen/mm.h | 6 ++++++
> 5 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index af47615b22..9142f359da 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(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
> BUG();
> }
> initrd->mod_end = 0;
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index fab77ab9cc..1f6b89bff4 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2342,7 +2342,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(page, 1, e, MEMF_no_refcount)) )
> {
> bool drop_dom_ref;
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index e07bd9a5ea..083e14b84f 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -728,7 +728,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_page(d, page, exch.out.extent_order,
> MEMF_no_refcount) )
> {
> unsigned long dec_count;
> @@ -797,7 +797,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(page, 1, d, 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 f51e406401..e279c6f713 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2282,9 +2282,9 @@ 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,
> + struct domain *d,
> unsigned int memflags)
> {
> int rc = 0;
> @@ -2304,7 +2304,7 @@ int assign_pages(
> {
> unsigned int extra_pages = 0;
>
> - for ( i = 0; i < (1ul << order); i++ )
> + for ( i = 0; i < nr; i++ )
> {
> ASSERT(!(pg[i].count_info & ~PGC_extra));
> if ( pg[i].count_info & PGC_extra )
> @@ -2313,18 +2313,18 @@ int assign_pages(
>
> ASSERT(!extra_pages ||
> ((memflags & MEMF_no_refcount) &&
> - extra_pages == 1u << order));
> + extra_pages == nr));
> }
> #endif
>
> if ( pg[0].count_info & PGC_extra )
> {
> - d->extra_pages += 1u << order;
> + d->extra_pages += nr;
> 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;
>
> if ( unlikely(tot_pages > d->max_pages) )
> {
> @@ -2336,10 +2336,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) == nr) )
> get_knownalive_domain(d);
>
> - for ( i = 0; i < (1 << order); i++ )
> + for ( i = 0; i < nr; i++ )
> {
> ASSERT(page_get_owner(&pg[i]) == NULL);
> page_set_owner(&pg[i], d);
> @@ -2354,6 +2354,11 @@ int assign_pages(
> return rc;
> }
>
> +int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
> + unsigned int memflags)
> +{
> + return assign_pages(pg, 1UL << order, d, memflags);
> +}
>
> struct page_info *alloc_domheap_pages(
> struct domain *d, unsigned int order, unsigned int memflags)
> @@ -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages(
> pg[i].count_info = PGC_extra;
> }
> }
> - if ( assign_pages(d, pg, order, memflags) )
> + if ( assign_page(d, pg, 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 8e8fb5a615..2e75cdcbb7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
> void heap_init_late(void);
>
> int assign_pages(
> + struct page_info *pg,
> + unsigned long nr,
> + struct domain *d,
> + unsigned int memflags);
> +
> +int assign_page(
> struct domain *d,
> struct page_info *pg,
> unsigned int order,
I find a bit odd that the parameters are ordered differently between
assign_pages() and assign_page(). They are similar interface after all.
I don't think it would be a problem for backporting purpose if
assign_page() has a different order for the arguments.
Jan, what do you think?
Cheers,
--
Julien Grall
On 13.08.2021 14:27, Julien Grall wrote: > On 28/07/2021 11:27, Penny Zheng wrote: >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status); >> void heap_init_late(void); >> >> int assign_pages( >> + struct page_info *pg, >> + unsigned long nr, >> + struct domain *d, >> + unsigned int memflags); >> + >> +int assign_page( >> struct domain *d, >> struct page_info *pg, >> unsigned int order, > > I find a bit odd that the parameters are ordered differently between > assign_pages() and assign_page(). They are similar interface after all. > > I don't think it would be a problem for backporting purpose if > assign_page() has a different order for the arguments. > > Jan, what do you think? Having both functions with similar parameter arrangement would certainly seem better to me, too. Jan
Hi Julian and Jan
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 8:27 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 V4 07/10] xen: re-define assign_pages and introduce
> assign_page
>
> Hi Penny,
>
> On 28/07/2021 11:27, Penny Zheng wrote:
> > In order to deal with the trouble of count-to-order conversion when
> > page number is not in a power-of-two, this commit re-define
> > assign_pages for nr pages and assign_page for original page with a single
> order.
> >
> > Backporting confusion could be helped by altering the order of
> > assign_page parameters, such that the compiler would point out that
> > adjustments at call sites are needed.
>
> Looking at the code, you don't alter the order of assign_page() parameters. So
> did you mean to refer to "assign_pages()"?
>
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v4 change:
> > - in all cases where order-0 pages get passed, prefer using
> > assign_pages to pass literal 1
> > - reconstruct the order of assign_pages parameters
> > - remove the unnecessary parentheses
> > ---
> > xen/arch/x86/pv/dom0_build.c | 2 +-
> > xen/common/grant_table.c | 2 +-
> > xen/common/memory.c | 4 ++--
> > xen/common/page_alloc.c | 23 ++++++++++++++---------
> > xen/include/xen/mm.h | 6 ++++++
> > 5 files changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/x86/pv/dom0_build.c
> > b/xen/arch/x86/pv/dom0_build.c index af47615b22..9142f359da 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(mfn_to_page(_mfn(mfn++)), 1, d, 0)
> > + )
> > BUG();
> > }
> > initrd->mod_end = 0;
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index
> > fab77ab9cc..1f6b89bff4 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -2342,7 +2342,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(page, 1, e, MEMF_no_refcount)) )
> > {
> > bool drop_dom_ref;
> >
> > diff --git a/xen/common/memory.c b/xen/common/memory.c index
> > e07bd9a5ea..083e14b84f 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -728,7 +728,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_page(d, page, exch.out.extent_order,
> > MEMF_no_refcount) )
> > {
> > unsigned long dec_count; @@ -797,7 +797,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(page, 1, d, 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
> > f51e406401..e279c6f713 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2282,9 +2282,9 @@ 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,
> > + struct domain *d,
> > unsigned int memflags)
> > {
> > int rc = 0;
> > @@ -2304,7 +2304,7 @@ int assign_pages(
> > {
> > unsigned int extra_pages = 0;
> >
> > - for ( i = 0; i < (1ul << order); i++ )
> > + for ( i = 0; i < nr; i++ )
> > {
> > ASSERT(!(pg[i].count_info & ~PGC_extra));
> > if ( pg[i].count_info & PGC_extra ) @@ -2313,18 +2313,18
> > @@ int assign_pages(
> >
> > ASSERT(!extra_pages ||
> > ((memflags & MEMF_no_refcount) &&
> > - extra_pages == 1u << order));
> > + extra_pages == nr));
> > }
> > #endif
> >
> > if ( pg[0].count_info & PGC_extra )
> > {
> > - d->extra_pages += 1u << order;
> > + d->extra_pages += nr;
> > 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;
> >
> > if ( unlikely(tot_pages > d->max_pages) )
> > {
> > @@ -2336,10 +2336,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) == nr) )
> > get_knownalive_domain(d);
> >
> > - for ( i = 0; i < (1 << order); i++ )
> > + for ( i = 0; i < nr; i++ )
> > {
> > ASSERT(page_get_owner(&pg[i]) == NULL);
> > page_set_owner(&pg[i], d);
> > @@ -2354,6 +2354,11 @@ int assign_pages(
> > return rc;
> > }
> >
> > +int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
> > + unsigned int memflags) {
> > + return assign_pages(pg, 1UL << order, d, memflags); }
> >
> > struct page_info *alloc_domheap_pages(
> > struct domain *d, unsigned int order, unsigned int memflags) @@
> > -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages(
> > pg[i].count_info = PGC_extra;
> > }
> > }
> > - if ( assign_pages(d, pg, order, memflags) )
> > + if ( assign_page(d, pg, 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
> > 8e8fb5a615..2e75cdcbb7 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
> > void heap_init_late(void);
> >
> > int assign_pages(
> > + struct page_info *pg,
> > + unsigned long nr,
> > + struct domain *d,
> > + unsigned int memflags);
> > +
> > +int assign_page(
> > struct domain *d,
> > struct page_info *pg,
> > unsigned int order,
>
> I find a bit odd that the parameters are ordered differently between
> assign_pages() and assign_page(). They are similar interface after all.
>
I will change the order back and make them in the similar order.
> I don't think it would be a problem for backporting purpose if
> assign_page() has a different order for the arguments.
>
> Jan, what do you think?
>
> Cheers,
>
> --
> Julien Grall
© 2016 - 2026 Red Hat, Inc.