Re: [Xen-devel] [PATCH v5 5/6] mm: add 'is_special_page' inline function...

From: Jan Beulich <jbeulich@suse.com>
To: paul@xen.org
Cc: "Tamas K Lengyel" <tamas@tklengyel.com>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Paul Durrant" <pdurrant@amazon.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Tim Deegan" <tim@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v5 5/6] mm: add 'is_special_page' inline function...
Date: Mon, 9 Mar 2020 14:28:26 +0100	[thread overview]
Message-ID: <41a8e134-bb6e-0437-536b-48afa3fa0ac9@suse.com> (raw)
In-Reply-To: <20200309102304.1251-6-paul@xen.org>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel