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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 06/03/2020 13:44, Jan Beulich wrote:
> On 06.03.2020 13:35, Paul Durrant wrote:
> > > -----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.
> Oh, indeed. (I don't suppose we can ever do away with this legacy
> mechanism?)
>
> > > 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?
> To be honest, my question was mainly based on me being puzzled that
> Arm (or common) code doesn't need any such adjustment. As a result
> I'm wondering whether that's just "luck" (in which case I'd agree
> the placement could remain as is), or whether there's a deeper
> reason behind that, largely guaranteeing Arm would also never need
> it.
is_special_page() is used in features that are not supported on Arm yet 
(e.g migration). So we will need it in the future and therefore the 
helper should be defined in common header.
Cheers,

--
Julien Grall

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