On 10.03.2020 18:49, paul@xxxxxxx wrote:
In auditing open-coded tests of PGC_xen_heap, I am unsure if offline_page()
needs to check for PGC_extra pages too.
"Extra" pages being the designated replacement for Xen heap ones,
I think it should. Then again the earlier
if ( (owner = page_get_owner_and_reference(pg)) )
should succeed on them (as much as it should for Xen heap pages
shared with a domain), so perhaps simply say something to this
effect in the description?
@@ -4216,8 +4216,7 @@ int steal_page(
if ( !(owner = page_get_owner_and_reference(page)) )
goto fail;
- if ( owner != d || is_xen_heap_page(page) ||
- (page->count_info & PGC_extra) )
+ if ( owner != d || is_special_page(page) )
goto fail_put;
/*
A few hundred lines down from here in xenmem_add_to_physmap_one()
there is a use of is_xen_heap_mfn(). Any reason that doesn't get
converted? Same question - because of the code being similar -
then goes for mm/p2m.c:p2m_add_foreign().
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -749,8 +749,9 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, gfn_t
gfn)
n = 1UL << min(cur_order, SUPERPAGE_ORDER + 0U);
for ( k = 0, page = mfn_to_page(mfn); k < n; ++k, ++page )
- if ( !(page->count_info & PGC_allocated) ||
- (page->count_info & (PGC_page_table | PGC_xen_heap)) ||
+ if ( is_special_page(page) ||
+ !(page->count_info & PGC_allocated) ||
+ (page->count_info & PGC_page_table) ||
(page->count_info & PGC_count_mask) > max_ref )
goto out;
}
@@ -886,8 +887,9 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t
*gfns, unsigned int count
* If this is ram, and not a pagetable or from the xen heap, and
* probably not mapped elsewhere, map it; otherwise, skip.
*/
- if ( p2m_is_ram(types[i]) && (pg->count_info & PGC_allocated) &&
- !(pg->count_info & (PGC_page_table | PGC_xen_heap)) &&
+ if ( p2m_is_ram(types[i]) && !is_special_page(pg) &&
+ (pg->count_info & PGC_allocated) &&
+ !(pg->count_info & PGC_page_table) &&
((pg->count_info & PGC_count_mask) <= max_ref) )
map[i] = map_domain_page(mfns[i]);
else
I appreciate your desire to use the inline function you add, and
I also appreciate that you likely prefer to not make the other
suggested change (at least not right here), but then I think the
commit message would better gain a remark regarding the
suspicious uses of PGC_page_table here. I continue to think that
they should be dropped as being pointless. In any event I fear
the resulting code will be less efficient, as I'm unconvinced
that the compiler will fold the now split bit checks.
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -559,7 +559,7 @@ _sh_propagate(struct vcpu *v,
* caching attributes in the shadows to match what was asked for.
*/
if ( (level == 1) && is_hvm_domain(d) &&
- !is_xen_heap_mfn(target_mfn) )
+ !is_special_page(mfn_to_page(target_mfn)) )
Careful - is_xen_heap_mfn() also includes an mfn_valid() check.
Code a few lines up from here suggests that MMIO MFNs can make
it here.
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -285,6 +285,11 @@ extern struct domain *dom_cow;
#include <asm/mm.h>
+static inline bool is_special_page(const struct page_info *page)
+{
+ return is_xen_heap_page(page) || (page->count_info & PGC_extra);
Seeing Arm32's implementation I understand why you need to use
|| here; it's a pity the two checks can't be folded. Hopefully
at least here the compiler recognizes the opportunity.
