This commit defines a new helper mark_page_free to extract common code,
like following the same cache/TLB coherency policy, between free_heap_pages
and the new function free_staticmem_pages, which will be introduced later.
The PDX compression makes that conversion between the MFN and the page can
be potentially non-trivial. As the function is internal, pass the MFN and
the page. They are both expected to match.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
xen/common/page_alloc.c | 89 ++++++++++++++++++++++-------------------
1 file changed, 48 insertions(+), 41 deletions(-)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 958ba0cd92..a3ee5eca9e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
return node_to_scrub(false) != NUMA_NO_NODE;
}
+static void mark_page_free(struct page_info *pg, mfn_t mfn)
+{
+ ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));
+
+ /*
+ * Cannot assume that count_info == 0, as there are some corner cases
+ * where it isn't the case and yet it isn't a bug:
+ * 1. page_get_owner() is NULL
+ * 2. page_get_owner() is a domain that was never accessible by
+ * its domid (e.g., failed to fully construct the domain).
+ * 3. page was never addressable by the guest (e.g., it's an
+ * auto-translate-physmap guest and the page was never included
+ * in its pseudophysical address space).
+ * In all the above cases there can be no guest mappings of this page.
+ */
+ switch ( pg->count_info & PGC_state )
+ {
+ case PGC_state_inuse:
+ BUG_ON(pg->count_info & PGC_broken);
+ pg->count_info = PGC_state_free;
+ break;
+
+ case PGC_state_offlining:
+ pg->count_info = (pg->count_info & PGC_broken) |
+ PGC_state_offlined;
+ tainted = 1;
+ break;
+
+ default:
+ printk(XENLOG_ERR
+ "pg MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
+ mfn_x(mfn),
+ pg->count_info, pg->v.free.order,
+ pg->u.free.val, pg->tlbflush_timestamp);
+ BUG();
+ }
+
+ /* If a page has no owner it will need no safety TLB flush. */
+ pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL);
+ if ( pg->u.free.need_tlbflush )
+ page_set_tlbflush_timestamp(pg);
+
+ /* This page is not a guest frame any more. */
+ page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */
+ set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
+}
+
/* Free 2^@order set of pages. */
static void free_heap_pages(
struct page_info *pg, unsigned int order, bool need_scrub)
@@ -1392,47 +1439,7 @@ static void free_heap_pages(
for ( i = 0; i < (1 << order); i++ )
{
- /*
- * Cannot assume that count_info == 0, as there are some corner cases
- * where it isn't the case and yet it isn't a bug:
- * 1. page_get_owner() is NULL
- * 2. page_get_owner() is a domain that was never accessible by
- * its domid (e.g., failed to fully construct the domain).
- * 3. page was never addressable by the guest (e.g., it's an
- * auto-translate-physmap guest and the page was never included
- * in its pseudophysical address space).
- * In all the above cases there can be no guest mappings of this page.
- */
- switch ( pg[i].count_info & PGC_state )
- {
- case PGC_state_inuse:
- BUG_ON(pg[i].count_info & PGC_broken);
- pg[i].count_info = PGC_state_free;
- break;
-
- case PGC_state_offlining:
- pg[i].count_info = (pg[i].count_info & PGC_broken) |
- PGC_state_offlined;
- tainted = 1;
- break;
-
- default:
- printk(XENLOG_ERR
- "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
- i, mfn_x(mfn) + i,
- pg[i].count_info, pg[i].v.free.order,
- pg[i].u.free.val, pg[i].tlbflush_timestamp);
- BUG();
- }
-
- /* If a page has no owner it will need no safety TLB flush. */
- pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL);
- if ( pg[i].u.free.need_tlbflush )
- page_set_tlbflush_timestamp(&pg[i]);
-
- /* This page is not a guest frame any more. */
- page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */
- set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
+ mark_page_free(&pg[i], mfn_add(mfn, i));
if ( need_scrub )
{
--
2.25.1
On 10.09.2021 04:52, Penny Zheng wrote:
> This commit defines a new helper mark_page_free to extract common code,
> like following the same cache/TLB coherency policy, between free_heap_pages
> and the new function free_staticmem_pages, which will be introduced later.
>
> The PDX compression makes that conversion between the MFN and the page can
> be potentially non-trivial. As the function is internal, pass the MFN and
> the page. They are both expected to match.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/common/page_alloc.c | 89 ++++++++++++++++++++++-------------------
> 1 file changed, 48 insertions(+), 41 deletions(-)
>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 958ba0cd92..a3ee5eca9e 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
> return node_to_scrub(false) != NUMA_NO_NODE;
> }
>
> +static void mark_page_free(struct page_info *pg, mfn_t mfn)
> +{
> + ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));
> +
> + /*
> + * Cannot assume that count_info == 0, as there are some corner cases
> + * where it isn't the case and yet it isn't a bug:
> + * 1. page_get_owner() is NULL
> + * 2. page_get_owner() is a domain that was never accessible by
> + * its domid (e.g., failed to fully construct the domain).
> + * 3. page was never addressable by the guest (e.g., it's an
> + * auto-translate-physmap guest and the page was never included
> + * in its pseudophysical address space).
> + * In all the above cases there can be no guest mappings of this page.
> + */
> + switch ( pg->count_info & PGC_state )
> + {
> + case PGC_state_inuse:
> + BUG_ON(pg->count_info & PGC_broken);
> + pg->count_info = PGC_state_free;
> + break;
> +
> + case PGC_state_offlining:
> + pg->count_info = (pg->count_info & PGC_broken) |
> + PGC_state_offlined;
> + tainted = 1;
You've broken two things at the same time here: You write to the global
variable of this name now, while ...
> @@ -1392,47 +1439,7 @@ static void free_heap_pages(
>
> for ( i = 0; i < (1 << order); i++ )
> {
> - /*
> - * Cannot assume that count_info == 0, as there are some corner cases
> - * where it isn't the case and yet it isn't a bug:
> - * 1. page_get_owner() is NULL
> - * 2. page_get_owner() is a domain that was never accessible by
> - * its domid (e.g., failed to fully construct the domain).
> - * 3. page was never addressable by the guest (e.g., it's an
> - * auto-translate-physmap guest and the page was never included
> - * in its pseudophysical address space).
> - * In all the above cases there can be no guest mappings of this page.
> - */
> - switch ( pg[i].count_info & PGC_state )
> - {
> - case PGC_state_inuse:
> - BUG_ON(pg[i].count_info & PGC_broken);
> - pg[i].count_info = PGC_state_free;
> - break;
> -
> - case PGC_state_offlining:
> - pg[i].count_info = (pg[i].count_info & PGC_broken) |
> - PGC_state_offlined;
> - tainted = 1;
... the local variable here doesn't get written anymore. Which Coverity
was kind enough to point out - please reference Coverity ID 1491872 in
the fix that I hope you will be able to provide quickly. (The easiest
change would seem to be to make mark_page_free() return bool, and set
"tainted" to 1 here accordingly.)
I understand that the two variables having the same name isn't very
helpful. I certainly wouldn't mind if you renamed the local one
suitably.
Jan
Hi Jan
Sorry for the broken.
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, September 15, 2021 9:56 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; julien@xen.org
> Subject: Re: [PATCH v7 3/7] xen: introduce mark_page_free
>
> On 10.09.2021 04:52, Penny Zheng wrote:
> > This commit defines a new helper mark_page_free to extract common
> > code, like following the same cache/TLB coherency policy, between
> > free_heap_pages and the new function free_staticmem_pages, which will be
> introduced later.
> >
> > The PDX compression makes that conversion between the MFN and the page
> > can be potentially non-trivial. As the function is internal, pass the
> > MFN and the page. They are both expected to match.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Acked-by: Jan Beulich <jbeulich@suse.com>
> > Reviewed-by: Julien Grall <jgrall@amazon.com>
> > ---
> > xen/common/page_alloc.c | 89
> > ++++++++++++++++++++++-------------------
> > 1 file changed, 48 insertions(+), 41 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 958ba0cd92..a3ee5eca9e 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
> > return node_to_scrub(false) != NUMA_NO_NODE; }
> >
> > +static void mark_page_free(struct page_info *pg, mfn_t mfn) {
> > + ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));
> > +
> > + /*
> > + * Cannot assume that count_info == 0, as there are some corner cases
> > + * where it isn't the case and yet it isn't a bug:
> > + * 1. page_get_owner() is NULL
> > + * 2. page_get_owner() is a domain that was never accessible by
> > + * its domid (e.g., failed to fully construct the domain).
> > + * 3. page was never addressable by the guest (e.g., it's an
> > + * auto-translate-physmap guest and the page was never included
> > + * in its pseudophysical address space).
> > + * In all the above cases there can be no guest mappings of this page.
> > + */
> > + switch ( pg->count_info & PGC_state )
> > + {
> > + case PGC_state_inuse:
> > + BUG_ON(pg->count_info & PGC_broken);
> > + pg->count_info = PGC_state_free;
> > + break;
> > +
> > + case PGC_state_offlining:
> > + pg->count_info = (pg->count_info & PGC_broken) |
> > + PGC_state_offlined;
> > + tainted = 1;
>
> You've broken two things at the same time here: You write to the global
> variable of this name now, while ...
>
> > @@ -1392,47 +1439,7 @@ static void free_heap_pages(
> >
> > for ( i = 0; i < (1 << order); i++ )
> > {
> > - /*
> > - * Cannot assume that count_info == 0, as there are some corner cases
> > - * where it isn't the case and yet it isn't a bug:
> > - * 1. page_get_owner() is NULL
> > - * 2. page_get_owner() is a domain that was never accessible by
> > - * its domid (e.g., failed to fully construct the domain).
> > - * 3. page was never addressable by the guest (e.g., it's an
> > - * auto-translate-physmap guest and the page was never included
> > - * in its pseudophysical address space).
> > - * In all the above cases there can be no guest mappings of this page.
> > - */
> > - switch ( pg[i].count_info & PGC_state )
> > - {
> > - case PGC_state_inuse:
> > - BUG_ON(pg[i].count_info & PGC_broken);
> > - pg[i].count_info = PGC_state_free;
> > - break;
> > -
> > - case PGC_state_offlining:
> > - pg[i].count_info = (pg[i].count_info & PGC_broken) |
> > - PGC_state_offlined;
> > - tainted = 1;
>
> ... the local variable here doesn't get written anymore. Which Coverity was
> kind enough to point out - please reference Coverity ID 1491872 in the fix that
> I hope you will be able to provide quickly. (The easiest change would seem to
> be to make mark_page_free() return bool, and set "tainted" to 1 here
> accordingly.)
>
Sure. I'll fix it today and let mark_page_free() return the status of "tainted".
> I understand that the two variables having the same name isn't very helpful. I
> certainly wouldn't mind if you renamed the local one suitably.
>
I'll rename the local one to "pg_tainted" to tell the difference.
> Jan
Penny
© 2016 - 2026 Red Hat, Inc.