Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.02.19 at 12:35, <julien.grall@xxxxxxx> wrote:
> --- 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));

I think this change wants to be accompanied by a warning attached
to the field declaration in the public header.

But I'd also like to have the tool stack maintainers' view on making
this field effectively unusable for Arm.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -510,6 +510,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> *memflags)
>      return true;
>  }
>  
> +#ifdef CONFIG_M2P
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) 
> arg)
>  {
>      struct xen_memory_exchange exch;
> @@ -802,6 +803,7 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>          rc = -EFAULT;
>      return rc;
>  }
> +#endif

Please can you move the #ifdef inside the function body, add #else
to return -EOPNOTSUPP, and ...

> @@ -1233,12 +1235,14 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          break;
>  
> +#ifdef CONFIG_M2P
>      case XENMEM_exchange:
>          if ( unlikely(start_extent) )
>              return -EINVAL;
>  
>          rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
>          break;
> +#endif

... avoid the extra #ifdef-ary here altogether?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -186,9 +186,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 )
>          {
> @@ -215,6 +216,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 = -ENOSYS;

-EOPNOTSUPP please.

> --- 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/xen/domain.h b/xen/include/xen/domain.h
> index d1bfc82f57..00d8b09794 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -118,4 +118,13 @@ struct vnuma_info {
>  
>  void vnuma_destroy(struct vnuma_info *vnuma);
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d_) ({                           \
> +    struct domain *d = (d_);                                    \

const

Also the naming needs to be the other way around: The macro
parameter should be named d (all instances will get substituted
anyway, and hence name collisions are impossible) while the
local variable should be named d_, to avoid collisions with names
in the outer scopes.

> +                                                                \

I won't insist, but especially small macros are often an exception
to the blank-line-between-declarations-and-statements rule. IOW
I'd prefer if you dropped this line, unless others correct my view
here.

Jan



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