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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 7 May 2019, Julien Grall 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.
>     - getdomaininfo: A new helper is introduced to wrap the call to
>     mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
>     will fail.
> 
> [1] https://patchwork.kernel.org/patch/9719913/
> 
> Signed-off-by Julien Grall <julien.grall@xxxxxxx>
> 
> ---
> 
> Cc: oleksandr_tyshchenko@xxxxxxxx
> Cc: andrii_anisov@xxxxxxxx
> 
>     Changes in v2:
>         - Add a warning in public headers
>         - Constify local variable in domain_shared_info_gfn
>         - Invert the naming (_d / d) in domain_shared_info_gfn
>         - Use -EOPNOTSUPP rather than -ENOSYS
>         - Rework how the memory_exchange hypercall is removed from Arm
> ---
>  xen/arch/x86/Kconfig            | 1 +
>  xen/common/Kconfig              | 3 +++
>  xen/common/domctl.c             | 2 +-
>  xen/common/memory.c             | 4 ++++
>  xen/drivers/passthrough/iommu.c | 6 +++++-
>  xen/include/asm-arm/domain.h    | 5 +++++
>  xen/include/public/domctl.h     | 4 ++++
>  xen/include/xen/domain.h        | 8 ++++++++
>  8 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 4b8b07b549..52922a87e7 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -16,6 +16,7 @@ config X86
>       select HAS_IOPORTS
>       select HAS_KEXEC
>       select MEM_ACCESS_ALWAYS_ON
> +     select HAS_M2P
>       select HAS_MEM_PAGING
>       select HAS_MEM_SHARING
>       select HAS_NS16550
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index c838506241..df871adc8f 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -63,6 +63,9 @@ config HAS_GDBSX
>  config HAS_IOPORTS
>       bool
>  
> +config HAS_M2P
> +     bool
> +
>  config NEEDS_LIBELF
>       bool
>  
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index bade9a63b1..29940fdea5 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>      info->outstanding_pages = d->outstanding_pages;
>      info->shr_pages         = atomic_read(&d->shr_pages);
>      info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>      BUG_ON(SHARED_M2P(info->shared_info_frame));
>  
>      info->cpupool = d->cpupool ? d->cpupool->cpupool_id : CPUPOOLID_NONE;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 86567e6117..d6a580da31 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -512,6 +512,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) 
> arg)
>  {
> +#ifdef CONFIG_M2P
>      struct xen_memory_exchange exch;
>      PAGE_LIST_HEAD(in_chunk_list);
>      PAGE_LIST_HEAD(out_chunk_list);
> @@ -806,6 +807,9 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>          rc = -EFAULT;
>      return rc;
> +#else /* !CONFIG_M2P */
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index a6697d58fb..dbb64b13bc 100644
> --- 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 )
>          {
> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          /* Use while-break to avoid compiler warning */
>          while ( iommu_iotlb_flush_all(d, flush_flags) )
>              break;
> +#else
> +        rc = -EOPNOTSUPP;
> +#endif
>  
>          if ( rc )
>              printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 312fec8932..d61b0188da 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct 
> vcpu_guest_context *vgc)
>  
>  static inline void arch_vcpu_block(struct vcpu *v) {}
>  
> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
> +{
> +    return INVALID_GFN;
> +}
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 19486d5e32..cac8ffffe9 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo {
>      uint64_aligned_t outstanding_pages;
>      uint64_aligned_t shr_pages;
>      uint64_aligned_t paged_pages;
> +    /*
> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> +     * provide a valid GFN.
> +     */
>      uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
>      uint64_aligned_t cpu_time;
>      uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index d1bfc82f57..f1761fe183 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -118,4 +118,12 @@ struct vnuma_info {
>  
>  void vnuma_destroy(struct vnuma_info *vnuma);
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d) ({                            \
> +    const struct domain *d_ = (d);                              \
> +                                                                \
> +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \

Aren't you missing a _gfn here?

  _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));


> +})
> +#endif
> +
>  #endif /* __XEN_DOMAIN_H__ */


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