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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 06 March 2020 13:44
> To: Paul Durrant <xadimgnik@xxxxxxxxx>
> Cc: pdurrant@xxxxxxxx; 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; 'Julien 
> Grall' <julien@xxxxxxx>;
> 'Wei Liu' <wl@xxxxxxx>; 'Konrad Rzeszutek Wilk' <konrad.wilk@xxxxxxxxxx>; 
> 'Andrew Cooper'
> <andrew.cooper3@xxxxxxxxxx>; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; '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: [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> 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?)

It's tricky because it would either mean breaking older (pre resource-mapping) 
QEMUs, or allowing the toolstack to allocate the 'special' pages with an extra 
flag to make them PGC_extra.

> 
> >> 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.
> 

I'll have a chat with Julien and see what he thinks.

  Paul



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