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 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.

> 
> > Remove the assert to allow 2M entries to be created for MMIO regions
> > that don't overlap with mmio_ro_ranges.
> > 
> > Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > Without this patch trying to create a PVH dom0 will trigger an assert
> > on certain hardware depending on the memory map.
> 
> Giving a simple example in the description would be helpful.
> 
> > --- 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);
> >          l2e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
> >              ? p2m_l2e_from_pfn(mfn_x(mfn),
> >                                 p2m_type_to_flags(p2m, p2mt, mfn, 1))
> 
> There's a similar check in the 1G mapping logic several lines up. Why
> does that not also need removing?

So far mmio_order doesn't allow creation of 1G entries for mmio
regions, that's why I haven't removed that assert. I can however
replace it with the assert suggested above properly adjusted for 1G
pages.

Thanks, Roger.

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