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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.19 at 14:38, <roger.pau@xxxxxxxxxx> 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))));
> 
> I would write it as 'if ( p2mt == p2m_mmio_direct ) ... else ...' but
> apart from that LGTM. If you are fine with this adjustment I will
> change it in preparation for v2.

Oh, sure - what's if and what's else doesn't really matter here. You
could even use ?: inside the ASSERT() if you wanted to.

Jan


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