Re: [PATCH for-4.12 2/8] amd/ntp: remove assert that prevents creating 2M MMIO entries

From: "Jan Beulich" <JBeulich@suse.com>
To: Roger Pau Monne <roger.pau@citrix.com>
Cc: Juergen Gross <jgross@suse.com>, Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	george.dunlap@citrix.com,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH for-4.12 2/8] amd/ntp: remove assert that prevents creating 2M MMIO entries
Date: Tue, 05 Feb 2019 00:45:56 -0700	[thread overview]
Message-ID: <5C593F340200007800213D19@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20190204171847.q3cymyuclraa77lm@mac>

>>> On 04.02.19 at 18:18, <roger.pau@citrix.com> wrote:
> On Mon, Feb 04, 2019 at 09:56:22AM -0700, Jan Beulich wrote:
>> >>> On 30.01.19 at 11:36, <roger.pau@citrix.com> wrote:
>> > The assert was originally added to make sure that higher order
>> > regions (> PAGE_ORDER_4K) could not be used to bypass the
>> > mmio_ro_ranges check performed by p2m_type_to_flags.
>> > 
>> > This however is already checked in set_mmio_p2m_entry, which makes
>> > sure that higher order mappings don't overlap with mmio_ro_ranges,
>> > thus allowing the creation of high order MMIO mappings safely.
>> 
>> Well, the assertions were added to make sure no other code
>> path appears that violates this requirement. Arguably e.g.
>> set_identity_p2m_entry() could gain an order parameter and
>> then try to establish larger p2m_mmio_direct entries.
>> 
>> Don't get me wrong, I don't object to the removal of the
>> assertions, but the description makes it sound as if they were
>> entirely redundant. Even better would be though if they
>> could be extended to keep triggering in "bad" cases.
> 
> I could add something like:
> 
> ASSERT(!rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
>                                 mfn_x(mfn) + PFN_DOWN(MB(2))));
> 
> I think this should be safe and would trigger in case of misuse.

Looks okay, if slightly extended (or made conditional) to exclude
the addition of MB(2) to MFN_INVALID to wrap and potentially
hit a r/o range in the low 1Mb.

>> > --- a/xen/arch/x86/mm/p2m-pt.c
>> > +++ b/xen/arch/x86/mm/p2m-pt.c
>> > @@ -668,7 +668,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
>> >          }
>> >  
>> >          ASSERT(p2m_flags_to_type(flags) != p2m_ioreq_server);
>> > -        ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
>> >          l2e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
>> >              ? p2m_l2e_from_pfn(mfn_x(mfn),
>> >                                 p2m_type_to_flags(p2m, p2mt, mfn, 1))
>> 
>> There's a similar check in the 1G mapping logic several lines up. Why
>> does that not also need removing?
> 
> So far mmio_order doesn't allow creation of 1G entries for mmio
> regions, that's why I haven't removed that assert. I can however
> replace it with the assert suggested above properly adjusted for 1G
> pages.

Yes, this or explicitly say in the description why you leave alone the
other one.

Jan

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