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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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));
else
    ASSERT(mfn_valid(mfn));

Jan


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