| Message ID | 20210910025215.1671073-4-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Domain on Static Allocation | expand |
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
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 ) {