Message ID | 20200305124504.3564-4-pdurrant@amzn.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | remove one more shared xenheap page: shared_info | expand |
On 05.03.2020 13:45, pdurrant@amzn.com 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
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 06 March 2020 11:56 > To: pdurrant@amzn.com > Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné > <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com> > 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, pdurrant@amzn.com 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
On 06.03.2020 13:03, Durrant, Paul wrote: >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >> Sent: 06 March 2020 11:56 >> To: pdurrant@amzn.com >> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné >> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com> >> 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, pdurrant@amzn.com 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
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 06 March 2020 13:39 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; > pdurrant@amzn.com; Roger Pau Monné <roger.pau@citrix.com> > 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 <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > >> Sent: 06 March 2020 11:56 > >> To: pdurrant@amzn.com > >> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné > >> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com> > >> 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, pdurrant@amzn.com 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 > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 06.03.2020 14:45, Paul Durrant wrote: >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >> Sent: 06 March 2020 13:39 >> To: Durrant, Paul <pdurrant@amazon.co.uk> >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; >> pdurrant@amzn.com; Roger Pau Monné <roger.pau@citrix.com> >> 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 <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >>>> Sent: 06 March 2020 11:56 >>>> To: pdurrant@amzn.com >>>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné >>>> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com> >>>> 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, pdurrant@amzn.com 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
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));