Re: [Xen-devel] [PATCH for-4.12 v2 2/7] amd/npt/shadow: replace assert that prevents creating 2M/1G MMIO entries

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

On Wed, Feb 13, 2019 at 08:53:14AM -0700, Jan Beulich wrote:
> >>> On 11.02.19 at 18:46, <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.
> > 
> > Replace the assert to allow 2M/1G entries to be created for MMIO
> > regions and add some extra asserts as a replacement to make sure
> > there's no overlapping with MMIO read-only ranges.
> > 
> > Note that 1G MMIO entries will not be created unless mmio_order is
> > changed to allow it.
> > 
> > Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> 
> Is this still the case? Iirc the original suggestion was to remove
> the assertion altogether?

Right, should I instead add your suggested-by tag?

> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -576,7 +576,15 @@ 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);
> > +        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_valid(mfn) || mfn_eq(mfn, INVALID_MFN));
> > +        else
> > +            ASSERT(mfn_valid(mfn));
> >          l3e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
> >              ? p2m_l3e_from_pfn(mfn_x(mfn),
> >                                 p2m_type_to_flags(p2m, p2mt, mfn, 2))
> > @@ -668,7 +676,15 @@ 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);
> > +        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_valid(mfn) || mfn_eq(mfn, INVALID_MFN));
> > +        else
> > +            ASSERT(mfn_valid(mfn));
> 
> Seeing this supposedly almost the same (but actually entirely the same,
> due to the wrong MB(2) in the first hunk) code I wonder whether this
> wouldn't better be put in a helper (macro or function), together with
> adjacent assertion in context, immediately ahead of the line you alter.

Thanks, I've placed this into a helper function named check_entry,
which is the best name I could come up with.

Roger.

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