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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.02.19 at 13:03, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, Feb 11, 2019 at 02:47:48AM -0700, Jan Beulich wrote:
>> >>> On 08.02.19 at 18:49, <roger.pau@xxxxxxxxxx> wrote:
>> > 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:
>> >> >> 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));
>> 
>> Hmm, perhaps this is good enough as an assertion, but I'd like
>> the fixed one to at least be considered:
>> 
>> 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 if ( p2m_allows_invalid_mfn(p2mt) || p2mt == p2m_invalid || p2mt == 
> p2m_mmio_dm )
>>     ASSERT(mfn_eq(mfn, INVALID_MFN));
> 
> I'm not sure this is correct, the fact that certain types (POD or
> paged memory) allow for invalid mfns doesn't mean that the mfn must
> always be invalid, like the above condition and assert assumes?

Oh, yes, I think you're right (for some of the paging types at
least; PoD should not be using other than INVALID_MFN anymore).

Jan


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