Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> Beulich
> Sent: 06 March 2020 12:20
> To: pdurrant@xxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall 
> <julien@xxxxxxx>; Wei Liu <wl@xxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>; Paul
> Durrant <pdurrant@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; 
> George Dunlap
> <george.dunlap@xxxxxxxxxx>; Tim Deegan <tim@xxxxxxx>; Tamas K Lengyel 
> <tamas@xxxxxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> On 05.03.2020 13:45, pdurrant@xxxxxxxx wrote:
> > --- 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))))) )
> 
> Shouldn't you delete most of this line, after the previous patch
> converted the ioreq server pages to PGC_extra ones?

I thought that too originally but then I realise we still have to cater for the 
'legacy' emulators that still require IOREQ server pages to be mapped through 
the p2m, in which case they will not be PGC_extra pages.

> 
> >              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 !! would be nice to go away at this occasion:
> 

Ok.

> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -285,6 +285,9 @@ extern struct domain *dom_cow;
> >
> >  #include <asm/mm.h>
> >
> > +#define is_special_page(page) \
> > +    (is_xen_heap_page(page) || ((page)->count_info & PGC_extra))
> 
> Can this become an inline function returning bool?
> 

I guess so. Hopefully there are no header inclusion orderings that would bite.

> Also I notice this construct is used by x86 code only - is there
> a particular reason it doesn't get placed in an x86 header (at
> least for the time being)?
> 

PGC_extra pages are common so maybe it is better off defined here so it is 
available to ARM code?

> Further I notice you neither take care of is_xen_heap_mfn(), nor
> does the description explain why that would not also need at
> least considering conversion. _sh_propagate(), for example, has
> an instance that I think would need changing.
> 

Ok; I'd simply not spotted any users that were vulnerable... I'll fix that one 
and re-check.

> Finally I notice there are two is_xen_heap_page() uses in tboot.c,
> both of which look like they also want converting.
> 

OK; I'd seen those so no idea why I didn't do the conversion. I'll fix.

  Paul

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


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