[v3,3/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

Commit Message

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

From: Paul Durrant <[e-mail]>

The walk of page_list in dom0_construct_pv() should ignore PGC_extra pages
as they are only created for special purposes and, if mapped, will be mapped
explicitly for whatever purpose is relevant.

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

v2:
 - New in v2
---
 xen/arch/x86/pv/dom0_build.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Jan Beulich March 6, 2020, 11:56 a.m. UTC | #1

On 05.03.2020 13:45, [e-mail] wrote:
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
>      {
>          mfn = mfn_x(page_to_mfn(page));
>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
> +
> +        if ( page->count_info & PGC_extra )
> +            continue;

This surely is a pattern, i.e. there are more similar changes to
make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
and hence with the goal of converting the shared info page would
also want adjustment. For dump_numa() it may be less important,
but it would still look more correct if it too got changed.
audit_p2m() might apparently complain about such pages (and
hence might be a problem with the one PGC_extra page VMX domains
now have). And this is only from me looking at
page_list_for_each(..., &d->page_list) constructs; who knows
what else there is.

Jan

Durrant, Paul March 6, 2020, 12:03 p.m. UTC | #2

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> Sent: 06 March 2020 11:56
> To: [e-mail]
> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Roger Pau Monné
> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
> constructing dom0
> 
> On 05.03.2020 13:45, [e-mail] wrote:
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
> >      {
> >          mfn = mfn_x(page_to_mfn(page));
> >          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
> > +
> > +        if ( page->count_info & PGC_extra )
> > +            continue;
> 
> This surely is a pattern, i.e. there are more similar changes to
> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
> and hence with the goal of converting the shared info page would
> also want adjustment. For dump_numa() it may be less important,
> but it would still look more correct if it too got changed.
> audit_p2m() might apparently complain about such pages (and
> hence might be a problem with the one PGC_extra page VMX domains
> now have). And this is only from me looking at
> page_list_for_each(..., &d->page_list) constructs; who knows
> what else there is.
> 

Those are dealt with by the is_special_page() patch later on I think. It didn't seem appropriate to use that macro here though since we know pages on the page list cannot be xenheap pages.

  Paul

Jan Beulich March 6, 2020, 1:39 p.m. UTC | #3

On 06.03.2020 13:03, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>> Sent: 06 March 2020 11:56
>> To: [e-mail]
>> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Roger Pau Monné
>> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>
>> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
>> constructing dom0
>>
>> On 05.03.2020 13:45, [e-mail] wrote:
>>> --- a/xen/arch/x86/pv/dom0_build.c
>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
>>>      {
>>>          mfn = mfn_x(page_to_mfn(page));
>>>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
>>> +
>>> +        if ( page->count_info & PGC_extra )
>>> +            continue;
>>
>> This surely is a pattern, i.e. there are more similar changes to
>> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
>> and hence with the goal of converting the shared info page would
>> also want adjustment. For dump_numa() it may be less important,
>> but it would still look more correct if it too got changed.
>> audit_p2m() might apparently complain about such pages (and
>> hence might be a problem with the one PGC_extra page VMX domains
>> now have). And this is only from me looking at
>> page_list_for_each(..., &d->page_list) constructs; who knows
>> what else there is.
>>
> 
> Those are dealt with by the is_special_page() patch later on I think.

Having already looked at that patch as well - I don't think so, no.
That one only replaces uses of is_xen_heap_page(), but doesn't add
any checks where such uses simply aren't needed because code is
looking at ->page_list only.

Jan

Paul Durrant March 6, 2020, 1:45 p.m. UTC | #4

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> Sent: 06 March 2020 13:39
> To: Durrant, Paul <[e-mail]>
> Cc: [e-mail]; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>;
> [e-mail]; Roger Pau Monné <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
> constructing dom0
> 
> On 06.03.2020 13:03, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> >> Sent: 06 March 2020 11:56
> >> To: [e-mail]
> >> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Roger Pau Monné
> >> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>
> >> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
> >> constructing dom0
> >>
> >> On 05.03.2020 13:45, [e-mail] wrote:
> >>> --- a/xen/arch/x86/pv/dom0_build.c
> >>> +++ b/xen/arch/x86/pv/dom0_build.c
> >>> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
> >>>      {
> >>>          mfn = mfn_x(page_to_mfn(page));
> >>>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
> >>> +
> >>> +        if ( page->count_info & PGC_extra )
> >>> +            continue;
> >>
> >> This surely is a pattern, i.e. there are more similar changes to
> >> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
> >> and hence with the goal of converting the shared info page would
> >> also want adjustment. For dump_numa() it may be less important,
> >> but it would still look more correct if it too got changed.
> >> audit_p2m() might apparently complain about such pages (and
> >> hence might be a problem with the one PGC_extra page VMX domains
> >> now have). And this is only from me looking at
> >> page_list_for_each(..., &d->page_list) constructs; who knows
> >> what else there is.
> >>
> >
> > Those are dealt with by the is_special_page() patch later on I think.
> 
> Having already looked at that patch as well - I don't think so, no.
> That one only replaces uses of is_xen_heap_page(), but doesn't add
> any checks where such uses simply aren't needed because code is
> looking at ->page_list only.

Well, I did say:

"It didn't seem appropriate to use that macro here though since we know pages on the page list cannot be xenheap pages."

i.e. an open coded check here seems like the right thing to do. If I've missed other places where I need to account for pages which are specifically PGC_extra pages then I'll need to fix them similarly.

  Paul



> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Jan Beulich March 6, 2020, 1:47 p.m. UTC | #5

On 06.03.2020 14:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>> Sent: 06 March 2020 13:39
>> To: Durrant, Paul <[e-mail]>
>> Cc: [e-mail]; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>;
>> [e-mail]; Roger Pau Monné <[e-mail]>
>> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
>> constructing dom0
>>
>> On 06.03.2020 13:03, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>>>> Sent: 06 March 2020 11:56
>>>> To: [e-mail]
>>>> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Roger Pau Monné
>>>> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>
>>>> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
>>>> constructing dom0
>>>>
>>>> On 05.03.2020 13:45, [e-mail] wrote:
>>>>> --- a/xen/arch/x86/pv/dom0_build.c
>>>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>>>> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
>>>>>      {
>>>>>          mfn = mfn_x(page_to_mfn(page));
>>>>>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
>>>>> +
>>>>> +        if ( page->count_info & PGC_extra )
>>>>> +            continue;
>>>>
>>>> This surely is a pattern, i.e. there are more similar changes to
>>>> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
>>>> and hence with the goal of converting the shared info page would
>>>> also want adjustment. For dump_numa() it may be less important,
>>>> but it would still look more correct if it too got changed.
>>>> audit_p2m() might apparently complain about such pages (and
>>>> hence might be a problem with the one PGC_extra page VMX domains
>>>> now have). And this is only from me looking at
>>>> page_list_for_each(..., &d->page_list) constructs; who knows
>>>> what else there is.
>>>>
>>>
>>> Those are dealt with by the is_special_page() patch later on I think.
>>
>> Having already looked at that patch as well - I don't think so, no.
>> That one only replaces uses of is_xen_heap_page(), but doesn't add
>> any checks where such uses simply aren't needed because code is
>> looking at ->page_list only.
> 
> Well, I did say:
> 
> "It didn't seem appropriate to use that macro here though since we
> know pages on the page list cannot be xenheap pages."

And I agree and understand.

> i.e. an open coded check here seems like the right thing to do.

Indeed.

> If I've missed other places where I need to account for pages which
> are specifically PGC_extra pages then I'll need to fix them similarly.

Yes please. Thanks.

Jan

Patch

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index dc16ef2e79..f8f1bbe2f4 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -792,6 +792,10 @@  int __init dom0_construct_pv(struct domain *d,
     {
         mfn = mfn_x(page_to_mfn(page));
         BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
+
+        if ( page->count_info & PGC_extra )
+            continue;
+
         if ( get_gpfn_from_mfn(mfn) >= count )
         {
             BUG_ON(is_pv_32bit_domain(d));