[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 Thu, Feb 07, 2019 at 05:21:28PM +0000, George Dunlap wrote: > On 2/5/19 1:38 PM, Roger Pau Monné wrote: > > On Tue, Feb 05, 2019 at 05:44:14AM -0700, Jan Beulich wrote: > >>>>> On 05.02.19 at 11:40, <roger.pau@xxxxxxxxxx> wrote: > >>> On Tue, Feb 05, 2019 at 12:45:56AM -0700, Jan Beulich wrote: > >>>>>>> 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. > >>> > >>> Ack, so it would be: > >>> > >>> ASSERT(mfn_eq(mfn, INVALID_MFN) || > >>> !rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn), > >>> mfn_x(mfn) + PFN_DOWN(MB(2)))); > >> > >> But that's still dropping the other aspect of the original ASSERT(): > >> > >>>>>>> --- 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); > >> > >> It also made sure that "valid" MFNs can't be used for mappings with > >> p2m_mmio_direct type. Except that I realize now that this is wrong in > >> certain cases, because MMIO pages may actually have "valid" MFNs. > >> mfn_valid(), after all, only tells us whether there's a struct page_info > >> for the MFN. I wonder if it's really this brokenness that you hit, > >> rather than what is explained in the description. > >> > >> When the assertion was introduced, MMIO wasn't handled by the > >> code correctly anyway (!mfn_valid() MFNs would not have got any > >> mappings at all in the 2M and 1G paths), whereas now we have > >> p2m_allows_invalid_mfn() there. So the situation has become worse > >> with other nearby changes. As a result I think we want to correct > >> the assertion here alongside the addition of what you suggest > >> above. What about > >> > >> if ( p2mt != p2m_mmio_direct ) > >> ASSERT(mfn_valid(mfn) || (mfn_eq(mfn, INVALID_MFN) && > >> p2m_allows_invalid_mfn(p2mt))); > >> else > >> ASSERT(!mfn_eq(mfn, INVALID_MFN) && > >> !rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn), > >> mfn_x(mfn) + PFN_DOWN(MB(2)))); > > FWIW I agree with this approach (asserting !overlaps for p2m_mmio_direct > types). Seeing the report from Sandler: https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg00578.html Looks like the assert on the if branch in the above example is not correct, the code allows for invalid mfns even if p2m_allows_invalid_mfn return false by using l2e_empty. I think the correct asserts would be: if ( p2mt == p2m_mmio_direct ) ASSERT(!mfn_eq(mfn, INVALID_MFN) && !rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn), mfn_x(mfn) + PFN_DOWN(MB(2)))); else ASSERT(mfn_valid(mfn) || mfn_eq(mfn, INVALID_MFN)); Roger. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |