Re: [Xen-devel] [PATCH for-4.12 3/8] iommu/pvh: add reserved regions below 1MB to the iommu page tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.19 at 11:36, <roger.pau@xxxxxxxxxx> wrote:
> Reserved memory ranges below 1MB on a PVH dom0 are added to the HAP
> page tables, but due to this being done before setting up the IOMMU
> the non RAM regions in those areas are not added to the IOMMU page
> tables. Fix this by making sure any reserved regions below 1MB are
> added to the IOMMU page tables.

So what was the reason again that we call iommu_hwdom_init() after
pvh_setup_p2m()? Am I mis-remembering there having been a patch
to flip their order?

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -151,12 +151,7 @@ static bool __hwdom_init hwdom_iommu_map(const struct 
> domain *d,
>       * inclusive mapping additionally maps in every pfn up to 4GB except 
> those
>       * that fall in unusable ranges for PV Dom0.
>       */
> -    if ( (pfn > max_pfn && !mfn_valid(mfn)) || xen_in_range(pfn) ||
> -         /*
> -          * Ignore any address below 1MB, that's already identity mapped by 
> the
> -          * Dom0 builder for HVM.
> -          */
> -         (!d->domain_id && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) )

There was a domain ID check here, and the comment explicitly said
Dom0.

> +    if ( (pfn > max_pfn && !mfn_valid(mfn)) || xen_in_range(pfn) )
>          return false;
>  
>      switch ( type = page_get_ram_type(mfn) )
> @@ -245,7 +240,12 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>          if ( !hwdom_iommu_map(d, pfn, max_pfn) )
>              continue;
>  
> -        if ( paging_mode_translate(d) )
> +        /*
> +         * Don't add any address below 1MB to the HAP page tables, that's
> +         * already done by the domain builder. Add addresses below 1MB to the
> +         * IOMMU page tables only.
> +         */
> +        if ( paging_mode_translate(d) && pfn >= PFN_DOWN(MB(1)) )

Nothing like this here. Did you determine that in the late hwdom
case things work without that extra precaution (i.e. the removed
check was really pointless)? If so, mentioning this would be helpful
(at the very least to be sure this was intentional).

Jan

>              rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
>          else
>              rc = iommu_map(d, _dfn(pfn), _mfn(pfn), PAGE_ORDER_4K,
> -- 
> 2.20.1





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