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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Wed, Apr 19, 2017 at 9:26 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
Hi, Julien

>
> Please CC the appropriate maintainers for all the components you modify.
Sorry, sure.

>
>
> 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.
Agree. Will do. So, the use_iommu flag will be included to xen_arch_domainconfig
and passed to iommu_domain_init() on ARM. On x86 we will always pass
the "false" value to
iommu_domain_init(). Right?

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

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

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



-- 
Regards,

Oleksandr Tyshchenko

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