Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

Please CC the appropriate maintainers for all the components you modify.

On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> This flag is intended to let Xen know that the guest has devices
> which will most likely be used for passthrough.
> The primary aim of this knowledge is to help the IOMMUs that don't
> share page tables with the CPU be ready before P2M code starts
> updating IOMMU mapping.
> So, if this flag is set the unshared IOMMUs will populate their
> page tables at the domain creation time and thereby will be able
> to handle IOMMU mapping updates from *the very beginning*.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
>  tools/libxl/libxl_create.c  | 5 +++++
>  xen/arch/arm/domain.c       | 4 +++-
>  xen/arch/x86/domain.c       | 4 +++-
>  xen/common/domctl.c         | 5 ++++-
>  xen/include/public/domctl.h | 3 +++
>  xen/include/xen/sched.h     | 3 +++
>  6 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e741b9a..4393fa2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
> *d_config,
>          flags |= XEN_DOMCTL_CDF_hap;
>      }
>
> +    /* TODO Are these assumptions enough to make decision about using IOMMU? */
> +    if ((d_config->num_dtdevs && d_config->dtdevs) ||
> +        (d_config->num_pcidevs && d_config->pcidevs))
> +        flags |= XEN_DOMCTL_CDF_use_iommu;
Regardless Jan's comment about the flag, I believe we still want to keep 
the current behavior for x86 (e.g allocating the page table on-demand).
So I think this should be per architecture decision rather than a common 
change. Maybe in, libxl__arch_domain_prepare_config. This also means we 
would switch to xen_arch_domainconfig.
> +
>      /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>      libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index bab62ee..940bb98 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>                         struct xen_arch_domainconfig *config)
>  {
>      int rc, count = 0;
> +    bool_t use_iommu;
s/bool_t/bool/

>      BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
>      d->arch.relmem = RELMEM_not_started;
> @@ -550,7 +551,8 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>      ASSERT(config != NULL);
>
>      /* p2m_init relies on some value initialized by the IOMMU subsystem */
> -    if ( (rc = iommu_domain_init(d, false)) != 0 )
> +    use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
> +    if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>          goto fail;
>
>      if ( (rc = p2m_init(d)) != 0 )
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 8ef4160..7d634ff 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>  {
>      bool paging_initialised = false;
>      int rc = -ENOMEM;
> +    bool_t use_iommu;
Ditto.

>      if ( config == NULL && !is_idle_domain(d) )
>          return -EINVAL;
> @@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>          if ( (rc = init_domain_irq_mapping(d)) != 0 )
>              goto fail;
>
> -        if ( (rc = iommu_domain_init(d, false)) != 0 )
> +        use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
> +        if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>              goto fail;
>      }
>      spin_lock_init(&d->arch.e820_lock);
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 93e3029..56c4d38 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>                 | XEN_DOMCTL_CDF_hap
>                 | XEN_DOMCTL_CDF_s3_integrity
>                 | XEN_DOMCTL_CDF_oos_off
> -               | XEN_DOMCTL_CDF_xs_domain)) )
> +               | XEN_DOMCTL_CDF_xs_domain
> +               | XEN_DOMCTL_CDF_use_iommu)) )
>              break;
>
>          dom = op->domain;
> @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>              domcr_flags |= DOMCRF_oos_off;
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
>              domcr_flags |= DOMCRF_xs_domain;
> +        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu )
> +            domcr_flags |= DOMCRF_use_iommu;
>
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>                            &op->u.createdomain.config);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 85cbb7c..a37a566 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>   /* Is this a xenstore domain? */
>  #define _XEN_DOMCTL_CDF_xs_domain     5
>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
> + /* Should IOMMU page tables be populated at the domain creation time? */
> +#define _XEN_DOMCTL_CDF_use_iommu     6
> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>      uint32_t flags;
>      struct xen_arch_domainconfig config;
>  };
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 0929c0b..80e6fdc 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned int 
> domcr_flags,
>   /* DOMCRF_xs_domain: xenstore domain */
>  #define _DOMCRF_xs_domain       6
>  #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
> + /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation time */
> +#define _DOMCRF_use_iommu       7
> +#define DOMCRF_use_iommu        (1U<<_DOMCRF_use_iommu)
>
>  /*
>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel