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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Ie: I think the assert should be adjusted to:

ASSERT(mfn_eq(mfn, INVALID_MFN) || mfn_valid(mfn));

Thanks, Roger.

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