[v3,5/6] mm: add 'is_special_page' macro...

Commit Message

[e-mail] March 5, 2020, 12:45 p.m. UTC

From: Paul Durrant <[e-mail]>

... to cover xenheap and PGC_extra pages.

PGC_extra pages are intended to hold data structures that are associated
with a domain and my be mapped by that domain. They should not be treated
as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
where code currently tests is_xen_heap_page() it should also check for
the PGC_extra bit in 'count_info'.

This patch therefore defines is_special_page() to cover both cases and
converts tests if is_xen_heap_page() to is_special_page() where
appropriate.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Ian Jackson <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Konrad Rzeszutek Wilk <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Tamas K Lengyel <[e-mail]>
Cc: Tim Deegan <[e-mail]>

v3:
 - Delete obsolete comment.

v2:
 - New in v2
---
 xen/arch/x86/domctl.c           |  2 +-
 xen/arch/x86/mm.c               |  9 ++++-----
 xen/arch/x86/mm/altp2m.c        |  2 +-
 xen/arch/x86/mm/mem_sharing.c   |  3 +--
 xen/arch/x86/mm/shadow/common.c | 13 ++++++++-----
 xen/include/xen/mm.h            |  3 +++
 6 files changed, 18 insertions(+), 14 deletions(-)

Comments

Tamas K Lengyel March 5, 2020, 3:09 p.m. UTC | #1

On Thu, Mar 5, 2020 at 5:45 AM <[e-mail]> wrote:
>
> From: Paul Durrant <[e-mail]>
>
> ... to cover xenheap and PGC_extra pages.
>
> PGC_extra pages are intended to hold data structures that are associated
> with a domain and my be mapped by that domain. They should not be treated
> as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> where code currently tests is_xen_heap_page() it should also check for
> the PGC_extra bit in 'count_info'.
>
> This patch therefore defines is_special_page() to cover both cases and
> converts tests if is_xen_heap_page() to is_special_page() where
> appropriate.

In context of VM forking, are these pages only used by some type of PV
mechanism? If not, would we need to get them copied somehow or are
these setup during the regular createdomain routine? Can they be
copied on-demand, ie. do these pages pass a p2m_is_ram() check?

Thanks,
Tamas

Durrant, Paul March 5, 2020, 3:38 p.m. UTC | #2

> -----Original Message-----
> From: Tamas K Lengyel <[e-mail]>
> Sent: 05 March 2020 15:10
> To: [e-mail]
> Cc: Xen-devel <[e-mail]>; Durrant, Paul <[e-mail]>; Jan Beulich
> <[e-mail]>; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné
> <[e-mail]>; George Dunlap <[e-mail]>; Ian Jackson
> <[e-mail]>; Julien Grall <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Stefano Stabellini <[e-mail]>; Tim Deegan <[e-mail]>
> Subject: RE: [EXTERNAL][PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Thu, Mar 5, 2020 at 5:45 AM <[e-mail]> wrote:
> >
> > From: Paul Durrant <[e-mail]>
> >
> > ... to cover xenheap and PGC_extra pages.
> >
> > PGC_extra pages are intended to hold data structures that are associated
> > with a domain and my be mapped by that domain. They should not be treated
> > as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> > where code currently tests is_xen_heap_page() it should also check for
> > the PGC_extra bit in 'count_info'.
> >
> > This patch therefore defines is_special_page() to cover both cases and
> > converts tests if is_xen_heap_page() to is_special_page() where
> > appropriate.
> 
> In context of VM forking, are these pages only used by some type of PV
> mechanism? If not, would we need to get them copied somehow or are
> these setup during the regular createdomain routine? Can they be
> copied on-demand, ie. do these pages pass a p2m_is_ram() check?

PGC_extra domheap pages are intended as direct replacements for shared xenheap pages and should be treated the same way. Thus they do not form part of the migration stream. Their p2m type depends entirely on how they are added to the p2m, as it is for any other page.

  Paul

> 
> Thanks,
> Tamas

Tamas K Lengyel March 5, 2020, 3:58 p.m. UTC | #3

On Thu, Mar 5, 2020 at 8:38 AM Durrant, Paul <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Tamas K Lengyel <[e-mail]>
> > Sent: 05 March 2020 15:10
> > To: [e-mail]
> > Cc: Xen-devel <[e-mail]>; Durrant, Paul <[e-mail]>; Jan Beulich
> > <[e-mail]>; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné
> > <[e-mail]>; George Dunlap <[e-mail]>; Ian Jackson
> > <[e-mail]>; Julien Grall <[e-mail]>; Konrad Rzeszutek Wilk
> > <[e-mail]>; Stefano Stabellini <[e-mail]>; Tim Deegan <[e-mail]>
> > Subject: RE: [EXTERNAL][PATCH v3 5/6] mm: add 'is_special_page' macro...
> >
> > CAUTION: This email originated from outside of the organization. Do not click links or open
> > attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Thu, Mar 5, 2020 at 5:45 AM <[e-mail]> wrote:
> > >
> > > From: Paul Durrant <[e-mail]>
> > >
> > > ... to cover xenheap and PGC_extra pages.
> > >
> > > PGC_extra pages are intended to hold data structures that are associated
> > > with a domain and my be mapped by that domain. They should not be treated
> > > as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> > > where code currently tests is_xen_heap_page() it should also check for
> > > the PGC_extra bit in 'count_info'.
> > >
> > > This patch therefore defines is_special_page() to cover both cases and
> > > converts tests if is_xen_heap_page() to is_special_page() where
> > > appropriate.
> >
> > In context of VM forking, are these pages only used by some type of PV
> > mechanism? If not, would we need to get them copied somehow or are
> > these setup during the regular createdomain routine? Can they be
> > copied on-demand, ie. do these pages pass a p2m_is_ram() check?
>
> PGC_extra domheap pages are intended as direct replacements for shared xenheap pages and should be treated the same way. Thus they do not form part of the migration stream. Their p2m type depends entirely on how they are added to the p2m, as it is for any other page.

OK, thanks. For the mem_sharing bits:

Acked-by: Tamas K Lengyel <[e-mail]>

Alan Robinson March 6, 2020, 7:02 a.m. UTC | #4

A typo...

On Thu, Mar 05, 2020 at 01:45:03PM +0100, [e-mail] wrote:
> 
> PGC_extra pages are intended to hold data structures that are associated
> with a domain and my be mapped by that domain. They should not be treated

s/my/may/

> as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> where code currently tests is_xen_heap_page() it should also check for
> the PGC_extra bit in 'count_info'.
> 
> This patch therefore defines is_special_page() to cover both cases and
> converts tests if is_xen_heap_page() to is_special_page() where
> appropriate.
> 

Alan

Durrant, Paul March 6, 2020, 9:22 a.m. UTC | #5

> -----Original Message-----
> From: [e-mail] <[e-mail]> On Behalf Of Alan Robinson
> Sent: 06 March 2020 07:02
> To: [e-mail]
> Cc: [e-mail]; Stefano Stabellini <[e-mail]>; Julien Grall
> <[e-mail]>; Wei Liu <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper
> <[e-mail]>; Durrant, Paul <[e-mail]>; Ian Jackson
> <[e-mail]>; George Dunlap <[e-mail]>; Tim Deegan <[e-mail]>; Tamas
> K Lengyel <[e-mail]>; Jan Beulich <[e-mail]>; Roger Pau Monné
> <[e-mail]>
> Subject: RE: [EXTERNAL][Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> A typo...
> 
> On Thu, Mar 05, 2020 at 01:45:03PM +0100, [e-mail] wrote:
> >
> > PGC_extra pages are intended to hold data structures that are associated
> > with a domain and my be mapped by that domain. They should not be treated
> 
> s/my/may/

Good spot. Will fix. Thanks,

  Paul

Jan Beulich March 6, 2020, 12:20 p.m. UTC | #6

On 05.03.2020 13:45, [e-mail] 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?

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

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

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)?

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.

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

Jan

Paul Durrant March 6, 2020, 12:35 p.m. UTC | #7

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> Sent: 06 March 2020 12:20
> To: [e-mail]
> Cc: Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>;
> Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Paul
> Durrant <[e-mail]>; Ian Jackson <[e-mail]>; George Dunlap
> <[e-mail]>; Tim Deegan <[e-mail]>; Tamas K Lengyel <[e-mail]>; xen-
> [e-mail]; Roger Pau Monné <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> On 05.03.2020 13:45, [e-mail] 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
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Jan Beulich March 6, 2020, 1:44 p.m. UTC | #8

On 06.03.2020 13:35, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>> Sent: 06 March 2020 12:20
>> To: [e-mail]
>> Cc: Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>;
>> Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Paul
>> Durrant <[e-mail]>; Ian Jackson <[e-mail]>; George Dunlap
>> <[e-mail]>; Tim Deegan <[e-mail]>; Tamas K Lengyel <[e-mail]>; xen-
>> [e-mail]; Roger Pau Monné <[e-mail]>
>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>
>> On 05.03.2020 13:45, [e-mail] 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.

Jan

Paul Durrant March 6, 2020, 1:48 p.m. UTC | #9

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 06 March 2020 13:44
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; 'Stefano Stabellini' <[e-mail]>; 'Julien Grall' <[e-mail]>;
> 'Wei Liu' <[e-mail]>; 'Konrad Rzeszutek Wilk' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; Durrant, Paul <[e-mail]>; 'Ian Jackson'
> <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Tim Deegan' <[e-mail]>;
> 'Tamas K Lengyel' <[e-mail]>; [e-mail]; 'Roger Pau Monné'
> <[e-mail]>
> 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 <[e-mail]> On Behalf Of Jan Beulich
> >> Sent: 06 March 2020 12:20
> >> To: [e-mail]
> >> Cc: Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> <[e-mail]>;
> >> Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Paul
> >> Durrant <[e-mail]>; Ian Jackson <[e-mail]>; George Dunlap
> >> <[e-mail]>; Tim Deegan <[e-mail]>; Tamas K Lengyel <[e-mail]>; xen-
> >> [e-mail]; Roger Pau Monné <[e-mail]>
> >> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> >>
> >> On 05.03.2020 13:45, [e-mail] 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

Jan Beulich March 6, 2020, 1:52 p.m. UTC | #10

On 06.03.2020 14:48, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 06 March 2020 13:44
>> To: Paul Durrant <[e-mail]>
>> Cc: [e-mail]; 'Stefano Stabellini' <[e-mail]>; 'Julien Grall' <[e-mail]>;
>> 'Wei Liu' <[e-mail]>; 'Konrad Rzeszutek Wilk' <[e-mail]>; 'Andrew Cooper'
>> <[e-mail]>; Durrant, Paul <[e-mail]>; 'Ian Jackson'
>> <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Tim Deegan' <[e-mail]>;
>> 'Tamas K Lengyel' <[e-mail]>; [e-mail]; 'Roger Pau Monné'
>> <[e-mail]>
>> 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 <[e-mail]> On Behalf Of Jan Beulich
>>>> Sent: 06 March 2020 12:20
>>>> To: [e-mail]
>>>> Cc: Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
>> <[e-mail]>;
>>>> Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Paul
>>>> Durrant <[e-mail]>; Ian Jackson <[e-mail]>; George Dunlap
>>>> <[e-mail]>; Tim Deegan <[e-mail]>; Tamas K Lengyel <[e-mail]>; xen-
>>>> [e-mail]; Roger Pau Monné <[e-mail]>
>>>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>>>
>>>> On 05.03.2020 13:45, [e-mail] 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,

Didn't even qemu-trad get switched? (Anyway, not a big deal here,
just would have been nice if this large conditional could have
been shrunk a little in size.)

> or allowing the toolstack to allocate the 'special' pages with
> an extra flag to make them PGC_extra.

Doesn't sound impossible, but also not something we want to eagerly
go for.

Jan

Paul Durrant March 6, 2020, 1:57 p.m. UTC | #11

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 06 March 2020 13:52
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; 'Stefano Stabellini' <[e-mail]>; 'Julien Grall' <[e-mail]>;
> 'Wei Liu' <[e-mail]>; 'Konrad Rzeszutek Wilk' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'George Dunlap'
> <[e-mail]>; 'Tim Deegan' <[e-mail]>; 'Tamas K Lengyel' <[e-mail]>; xen-
> [e-mail]; 'Roger Pau Monné' <[e-mail]>
> Subject: Re: [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> On 06.03.2020 14:48, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: 06 March 2020 13:44
> >> To: Paul Durrant <[e-mail]>
> >> Cc: [e-mail]; 'Stefano Stabellini' <[e-mail]>; 'Julien Grall'
> <[e-mail]>;
> >> 'Wei Liu' <[e-mail]>; 'Konrad Rzeszutek Wilk' <[e-mail]>; 'Andrew Cooper'
> >> <[e-mail]>; Durrant, Paul <[e-mail]>; 'Ian Jackson'
> >> <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Tim Deegan'
> <[e-mail]>;
> >> 'Tamas K Lengyel' <[e-mail]>; [e-mail]; 'Roger Pau Monné'
> >> <[e-mail]>
> >> 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 <[e-mail]> On Behalf Of Jan Beulich
> >>>> Sent: 06 March 2020 12:20
> >>>> To: [e-mail]
> >>>> Cc: Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> >> <[e-mail]>;
> >>>> Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Paul
> >>>> Durrant <[e-mail]>; Ian Jackson <[e-mail]>; George Dunlap
> >>>> <[e-mail]>; Tim Deegan <[e-mail]>; Tamas K Lengyel <[e-mail]>; xen-
> >>>> [e-mail]; Roger Pau Monné <[e-mail]>
> >>>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> >>>>
> >>>> On 05.03.2020 13:45, [e-mail] 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,
> 
> Didn't even qemu-trad get switched? (Anyway, not a big deal here,
> just would have been nice if this large conditional could have
> been shrunk a little in size.)

Yes, trad is switched but I thought our position was that we supported use of arbitrary distro versions of QEMU in which case any compat code really does have to stick around for a very long time :-(

> 
> > or allowing the toolstack to allocate the 'special' pages with
> > an extra flag to make them PGC_extra.
> 
> Doesn't sound impossible, but also not something we want to eagerly
> go for.
> 

No, probably not worth it just to save doing this test.

  Paul

Julien Grall March 6, 2020, 2:26 p.m. UTC | #12

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 <[e-mail]> On Behalf Of Jan Beulich
>>> Sent: 06 March 2020 12:20
>>> To: [e-mail]
>>> Cc: Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>;
>>> Konrad Rzeszutek Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Paul
>>> Durrant <[e-mail]>; Ian Jackson <[e-mail]>; George Dunlap
>>> <[e-mail]>; Tim Deegan <[e-mail]>; Tamas K Lengyel <[e-mail]>; xen-
>>> [e-mail]; Roger Pau Monné <[e-mail]>
>>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>>
>>> On 05.03.2020 13:45, [e-mail] 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,

Jan Beulich March 6, 2020, 2:50 p.m. UTC | #13

On 06.03.2020 15:26, Julien Grall wrote:
> On 06/03/2020 13:44, Jan Beulich wrote:
>> On 06.03.2020 13:35, Paul Durrant wrote:
>>>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>>>> Sent: 06 March 2020 12:20
>>>>
>>>> On 05.03.2020 13:45, [e-mail] 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.

Okay then, thanks for clarifying.

Jan

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ed86762fa6..add70126b9 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -394,7 +394,7 @@  long arch_do_domctl(
             page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
 
             if ( unlikely(!page) ||
-                 unlikely(is_xen_heap_page(page)) )
+                 unlikely(is_special_page(page)) )
             {
                 if ( unlikely(p2m_is_broken(t)) )
                     type = XEN_DOMCTL_PFINFO_BROKEN;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ba7563ed3c..353bde5c2c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1014,7 +1014,7 @@  get_page_from_l1e(
         unsigned long cacheattr = pte_flags_to_cacheattr(l1f);
         int err;
 
-        if ( is_xen_heap_page(page) )
+        if ( is_special_page(page) )
         {
             if ( write )
                 put_page_type(page);
@@ -2447,7 +2447,7 @@  static int cleanup_page_mappings(struct page_info *page)
     {
         page->count_info &= ~PGC_cacheattr_mask;
 
-        BUG_ON(is_xen_heap_page(page));
+        BUG_ON(is_special_page(page));
 
         rc = update_xen_mappings(mfn, 0);
     }
@@ -2477,7 +2477,7 @@  static int cleanup_page_mappings(struct page_info *page)
                 rc = rc2;
         }
 
-        if ( likely(!is_xen_heap_page(page)) )
+        if ( likely(!is_special_page(page)) )
         {
             ASSERT((page->u.inuse.type_info &
                     (PGT_type_mask | PGT_count_mask)) == PGT_writable_page);
@@ -4216,8 +4216,7 @@  int steal_page(
     if ( !(owner = page_get_owner_and_reference(page)) )
         goto fail;
 
-    if ( owner != d || is_xen_heap_page(page) ||
-         (page->count_info & PGC_extra) )
+    if ( owner != d || is_special_page(page) )
         goto fail_put;
 
     /*
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 50768f2547..c091b03ea3 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -77,7 +77,7 @@  int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn)
      * pageable() predicate for this, due to it having the same properties
      * that we want.
      */
-    if ( !p2m_is_pageable(p2mt) || is_xen_heap_page(pg) )
+    if ( !p2m_is_pageable(p2mt) || is_special_page(pg) )
     {
         rc = -EINVAL;
         goto err;
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 3835bc928f..f49f27a3ef 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -840,9 +840,8 @@  static int nominate_page(struct domain *d, gfn_t gfn,
     if ( !p2m_is_sharable(p2mt) )
         goto out;
 
-    /* Skip xen heap pages */
     page = mfn_to_page(mfn);
-    if ( !page || is_xen_heap_page(page) )
+    if ( !page || is_special_page(page) )
         goto out;
 
     /* Check if there are mem_access/remapped altp2m entries for this page */
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index cba3ab1eba..e835940d86 100644
--- 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),
                    (is_hvm_domain(d) && is_ioreq_server_page(d, page)));
     }
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index d0d095d9c7..3a57c9177f 100644
--- 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))
+
 #ifndef page_list_entry
 struct page_list_head
 {