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....@citrix.com> wrote: >>> On Tue, Feb 05, 2019 at 12:45:56AM -0700, Jan Beulich wrote: >>>>>>> On 04.02.19 at 18:18, <roger....@citrix.com> wrote: >>>>> On Mon, Feb 04, 2019 at 09:56:22AM -0700, Jan Beulich wrote: >>>>>>>>> On 30.01.19 at 11:36, <roger....@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. >>> >>> 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). -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
