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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 09 March 2020 13:28
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; Tamas 
> K Lengyel
> <tamas@xxxxxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Ian 
> Jackson
> <ian.jackson@xxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Konrad Rzeszutek 
> Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim 
> Deegan <tim@xxxxxxx>
> Subject: Re: [PATCH v5 5/6] mm: add 'is_special_page' inline function...
> 
> On 09.03.2020 11:23, paul@xxxxxxx 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


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