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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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