[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.12 2/8] amd/ntp: remove assert that prevents creating 2M MMIO entries
>>> On 04.02.19 at 18:18, <roger.pau@xxxxxxxxxx> wrote: > On Mon, Feb 04, 2019 at 09:56:22AM -0700, Jan Beulich wrote: >> >>> On 30.01.19 at 11:36, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |