| Message ID | 20200309102304.1251-6-paul@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remove one more shared xenheap page: shared_info | expand |
On 09.03.2020 11:23, paul@xen.org wrote: > v4: > - Use inline function instead of macro > - Add missing conversions from is_xen_heap_page() Among these also one conversion of is_xen_heap_mfn(). I'm still curious why others wouldn't need converting - the description doesn't mention there are more, see p2m_add_foreign() for an example (may warrant introduction of is_special_mfn() then). It would probably be beneficial if the description gave some generic criteria for cases where conversion is (not) needed. But there are issues beyond this, as there are also open-coded instances of PGC_xen_heap checks, and that's the other possible regression I notice from the APIC assist MFN page conversion: PoD code, to avoid doing two separate checks on ->count_info [1], uses two instances of a construct like this one !(pg->count_info & (PGC_page_table | PGC_xen_heap)) && (and again I didn't do a complete audit for further occurrences). This means the APIC assist page right now might be a candidate for getting converted to PoD (possibly others of the constraints actually prohibit this, but I'm not sure). [1] I'm unconvinced PGC_page_table pages can actually appear there, so the open-coding may in fact be an optimization of dead code. > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn) > * The qemu helper process has an untyped mapping of this dom's RAM > * and the HVM restore program takes another. > * Also allow one typed refcount for > - * - Xen heap pages, to match share_xen_page_with_guest(), > - * - ioreq server pages, to match prepare_ring_for_helper(). > + * - special pages, which are explicitly referenced and mapped by > + * Xen. > + * - ioreq server pages, which may be special pages or normal > + * guest pages with an extra reference taken by > + * prepare_ring_for_helper(). > */ > if ( !(shadow_mode_external(d) > && (page->count_info & PGC_count_mask) <= 3 > && ((page->u.inuse.type_info & PGT_count_mask) > - == (is_xen_heap_page(page) || > + == (is_special_page(page) || > (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) > printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn > - " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n", > + " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", > mfn_x(gmfn), gfn_x(gfn), > page->count_info, page->u.inuse.type_info, > - !!is_xen_heap_page(page), > + !!is_special_page(page), The reason for me to ask to switch to an inline function was to see this !! go away. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 09 March 2020 13:28 > To: paul@xen.org > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Tamas K Lengyel > <tamas@tklengyel.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau > Monné <roger.pau@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson > <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim Deegan <tim@xen.org> > Subject: Re: [PATCH v5 5/6] mm: add 'is_special_page' inline function... > > On 09.03.2020 11:23, paul@xen.org wrote: > > v4: > > - Use inline function instead of macro > > - Add missing conversions from is_xen_heap_page() > > Among these also one conversion of is_xen_heap_mfn(). I'm still > curious why others wouldn't need converting - the description > doesn't mention there are more, see p2m_add_foreign() for an > example (may warrant introduction of is_special_mfn() then). It > would probably be beneficial if the description gave some > generic criteria for cases where conversion is (not) needed. > OK. Basically it’s to cover the case where is_xen_heap_page() is used to mean 'isn't normal guest memory'. I'll expand the commit comment to say that. > But there are issues beyond this, as there are also open-coded > instances of PGC_xen_heap checks, and that's the other possible > regression I notice from the APIC assist MFN page conversion: > PoD code, to avoid doing two separate checks on ->count_info [1], > uses two instances of a construct like this one > > !(pg->count_info & (PGC_page_table | PGC_xen_heap)) && > > (and again I didn't do a complete audit for further > occurrences). This means the APIC assist page right now might > be a candidate for getting converted to PoD (possibly others of > the constraints actually prohibit this, but I'm not sure). > > [1] I'm unconvinced PGC_page_table pages can actually appear > there, so the open-coding may in fact be an optimization of > dead code. Ok, I'll audit occurrences of PGC_xen_heap. Paul > > > --- a/xen/arch/x86/mm/shadow/common.c > > +++ b/xen/arch/x86/mm/shadow/common.c > > @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn) > > * The qemu helper process has an untyped mapping of this dom's RAM > > * and the HVM restore program takes another. > > * Also allow one typed refcount for > > - * - Xen heap pages, to match share_xen_page_with_guest(), > > - * - ioreq server pages, to match prepare_ring_for_helper(). > > + * - special pages, which are explicitly referenced and mapped by > > + * Xen. > > + * - ioreq server pages, which may be special pages or normal > > + * guest pages with an extra reference taken by > > + * prepare_ring_for_helper(). > > */ > > if ( !(shadow_mode_external(d) > > && (page->count_info & PGC_count_mask) <= 3 > > && ((page->u.inuse.type_info & PGT_count_mask) > > - == (is_xen_heap_page(page) || > > + == (is_special_page(page) || > > (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) > > printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn > > - " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n", > > + " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", > > mfn_x(gmfn), gfn_x(gfn), > > page->count_info, page->u.inuse.type_info, > > - !!is_xen_heap_page(page), > > + !!is_special_page(page), > > The reason for me to ask to switch to an inline function was to > see this !! go away. > > Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index ed86762fa6..add70126b9 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -394,7 +394,7 @@ long arch_do_domctl( page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC); if ( unlikely(!page) || - unlikely(is_xen_heap_page(page)) ) + unlikely(is_special_page(page)) ) { if ( unlikely(p2m_is_broken(t)) ) type = XEN_DOMCTL_PFINFO_BROKEN; diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index ba7563ed3c..353bde5c2c 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1014,7 +1014,7 @@ get_page_from_l1e( unsigned long cacheattr = pte_flags_to_cacheattr(l1f); int err; - if ( is_xen_heap_page(page) ) + if ( is_special_page(page) ) { if ( write ) put_page_type(page); @@ -2447,7 +2447,7 @@ static int cleanup_page_mappings(struct page_info *page) { page->count_info &= ~PGC_cacheattr_mask; - BUG_ON(is_xen_heap_page(page)); + BUG_ON(is_special_page(page)); rc = update_xen_mappings(mfn, 0); } @@ -2477,7 +2477,7 @@ static int cleanup_page_mappings(struct page_info *page) rc = rc2; } - if ( likely(!is_xen_heap_page(page)) ) + if ( likely(!is_special_page(page)) ) { ASSERT((page->u.inuse.type_info & (PGT_type_mask | PGT_count_mask)) == PGT_writable_page); @@ -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; /* diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c index 50768f2547..c091b03ea3 100644 --- a/xen/arch/x86/mm/altp2m.c +++ b/xen/arch/x86/mm/altp2m.c @@ -77,7 +77,7 @@ int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn) * pageable() predicate for this, due to it having the same properties * that we want. */ - if ( !p2m_is_pageable(p2mt) || is_xen_heap_page(pg) ) + if ( !p2m_is_pageable(p2mt) || is_special_page(pg) ) { rc = -EINVAL; goto err; diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 3835bc928f..f49f27a3ef 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -840,9 +840,8 @@ static int nominate_page(struct domain *d, gfn_t gfn, if ( !p2m_is_sharable(p2mt) ) goto out; - /* Skip xen heap pages */ page = mfn_to_page(mfn); - if ( !page || is_xen_heap_page(page) ) + if ( !page || is_special_page(page) ) goto out; /* Check if there are mem_access/remapped altp2m entries for this page */ diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index cba3ab1eba..e835940d86 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn) * The qemu helper process has an untyped mapping of this dom's RAM * and the HVM restore program takes another. * Also allow one typed refcount for - * - Xen heap pages, to match share_xen_page_with_guest(), - * - ioreq server pages, to match prepare_ring_for_helper(). + * - special pages, which are explicitly referenced and mapped by + * Xen. + * - ioreq server pages, which may be special pages or normal + * guest pages with an extra reference taken by + * prepare_ring_for_helper(). */ if ( !(shadow_mode_external(d) && (page->count_info & PGC_count_mask) <= 3 && ((page->u.inuse.type_info & PGT_count_mask) - == (is_xen_heap_page(page) || + == (is_special_page(page) || (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn - " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n", + " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", mfn_x(gmfn), gfn_x(gfn), page->count_info, page->u.inuse.type_info, - !!is_xen_heap_page(page), + !!is_special_page(page), (is_hvm_domain(d) && is_ioreq_server_page(d, page))); } diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 26798b317c..ac19d203d7 100644 --- 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)) ) { int type; diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c index 6cc020cb71..2fd7ce5305 100644 --- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c @@ -189,7 +189,7 @@ static void update_pagetable_mac(vmac_ctx_t *ctx) if ( !mfn_valid(_mfn(mfn)) ) continue; - if ( is_page_in_use(page) && !is_xen_heap_page(page) ) + if ( is_page_in_use(page) && !is_special_page(page) ) { if ( page->count_info & PGC_page_table ) { @@ -294,7 +294,7 @@ static void tboot_gen_xenheap_integrity(const uint8_t key[TB_KEY_SIZE], + 3 * PAGE_SIZE)) ) continue; /* skip tboot and its page tables */ - if ( is_page_in_use(page) && is_xen_heap_page(page) ) + if ( is_page_in_use(page) && is_special_page(page) ) { void *pg; diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index d0d095d9c7..373de59969 100644 --- 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(struct page_info *page) +{ + return is_xen_heap_page(page) || (page->count_info & PGC_extra); +} + #ifndef page_list_entry struct page_list_head {