Re: [Xen-devel] [PATCH v3 07/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.06.19 at 18:03, <julien.grall@xxxxxxx> wrote:
> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
> bogus as we directly return the MFN passed in parameter.
> 
> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
> are only 3 callers:
>     - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU
>     page-tables when the P2M is not shared with the IOMMU. No issues so
>     far as Arm does not yet support non-shared P2M case.
>     - memory_exchange: Arm cannot not use it because steal_page is not
>     implemented.
>     - getdomaininfo: Toolstack may map the shared page. It looks like
>     this is mostly used for mapping the P2M of PV guest. Therefore the
>     issue might be minor.
> 
> Implementing the M2P on Arm is not planned. The M2P would require 
> significant
> amount of VA address (very tough on 32-bit) that can hardly be justified with
> the current use of mfn_to_gmfn.
>     - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the
>     IOMMU page-tables is delayed until the first device is assigned.
>     In the embedded case, we will known in most of the times what
>     devices are assigned during the domain creation. So it is possible
>     to take to enable the IOMMU from start. See [1] for the patch.
>     - memory_exchange: This does not work and I haven't seen any
>     request for it so far.
>     - getdomaininfo: The structure on Arm does not seem to contain a lot
>     of useful information on Arm. It is unclear whether we want to
>     allow the toolstack mapping it on Arm.
> 
> This patch introduces a config option HAS_M2P to tell whether an
> architecture implements the M2P.
>     - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
>     is not sharing the P2M.
>     - memory_exchange: The hypercall is marked as not supported when the
>     M2P does not exist.

But where's the connection between there being M2P and the
availability of this operation? I think I've suggested so before:
Why don't we simply disable this operation for translated
guests (in an independent patch)?

>     - getdomaininfo: A new helper is introduced to wrap the call to
>     mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
>     fail on mapping if used.

This reads slightly stale now.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>      if ( need_iommu_pt_sync(d) )
>      {
> +        int rc = 0;
> +#ifdef CONFIG_HAS_M2P
>          struct page_info *page;
>          unsigned int i = 0, flush_flags = 0;
> -        int rc = 0;
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> @@ -221,6 +222,11 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          if ( rc )
>              printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
>                     d->domain_id, rc);
> +#else
> +        ASSERT_UNREACHABLE();
> +        rc = -EOPNOTSUPP;
> +#endif
> +
>      }

Please don't add a blank line ahead of a closing brace.

Jan



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