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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.19 at 11:40, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Feb 05, 2019 at 12:45:56AM -0700, Jan Beulich wrote:
>> >>> On 04.02.19 at 18:18, <roger.pau@xxxxxxxxxx> wrote:
>> > 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.
>> 
>> Looks okay, if slightly extended (or made conditional) to exclude
>> the addition of MB(2) to MFN_INVALID to wrap and potentially
>> hit a r/o range in the low 1Mb.
> 
> Ack, so it would be:
> 
> ASSERT(mfn_eq(mfn, INVALID_MFN) ||
>        !rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
>                                 mfn_x(mfn) + PFN_DOWN(MB(2))));

But that's still dropping the other aspect of the original ASSERT():

>> >> > --- 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);

It also made sure that "valid" MFNs can't be used for mappings with
p2m_mmio_direct type. Except that I realize now that this is wrong in
certain cases, because MMIO pages may actually have "valid" MFNs.
mfn_valid(), after all, only tells us whether there's a struct page_info
for the MFN. I wonder if it's really this brokenness that you hit,
rather than what is explained in the description.

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))));

? This could be further improved by checking the page owner to
(not) be dom_io, but doing so perhaps goes too far at this point.

>> >> >          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.
>> 
>> Yes, this or explicitly say in the description why you leave alone the
>> other one.
> 
> I think changing the assert is likely more future proof.

I agree, but I didn't want to omit the alternative.

Jan


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