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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.05.19 at 17:14, <julien.grall@xxxxxxx> wrote:
> 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.

Was it you or someone else to suggest it be restricted to non-translated
guests in the first place? I'd prefer this over the #ifdef-ary you add.

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

There's no use of mfn_to_gmfn() in either of the wrappers, so why
mention this to-be-removed one?

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

What is the intended behavior on 32-bit Arm here? Do you really
mean to return a value with 32 bits of ones (instead of 64 bits of
them) in this 64-bit field?

> --- 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",

Would you mind extending the scope of the #ifdef beyond this printk()?
It seems pretty pointless to me to unconditionally emit a log message
here.

> --- 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.
> +     */

Along the lines of what I've said above, I think you want to spell out
here what the value is going to be in this case.

> --- 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)));       \
> +})
> +#endif

And an inline function doesn't work here?

Jan



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