Re: [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig

[Julien Grall <julien.grall@arm.com>]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> This flag is intended to let Xen know that the guest has devices
> which will most likely be used for passthrough and as the result
> the use of IOMMU is expected for this domain.
> The primary aim of this knowledge is to help the IOMMUs that don't
> share page tables with the CPU on ARM be ready before P2M code starts
> updating IOMMU mapping.
> So, if this flag is set the non-shared 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*.
>
> In order to retain the current behavior for x86 still call
> iommu_domain_init() with use_iommu flag being forced to false.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
>
> ---
>    Changes in V1:
>       - Treat use_iommu flag as the ARM decision only. Don't use
>         common domain creation flag for it, use ARM config instead.
>       - Clarify patch subject/description.
> ---
>  tools/libxl/libxl_arm.c       | 10 ++++++++++
>  xen/arch/arm/domain.c         |  2 +-
>  xen/include/public/arch-arm.h |  5 +++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..9c4705e 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>          return ERROR_FAIL;
>      }
>
> +    /* 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))

Checking num_dtdevs and num_pcidevs is enough. It would be a bug if 
dtdevs and pcidevs are not null.

> +        xc_config->use_iommu = 1;
> +    else
> +        xc_config->use_iommu = 0;
> +
> +    LOG(DEBUG, "The use of IOMMU %s expected for this domain",
> +        xc_config->use_iommu ? "is" : "isn't");
> +
>      return 0;
>  }
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index ec19310..81c4b90 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -569,7 +569,7 @@ 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 )
> +    if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 )

!!config->use_iommu is enough.

>          goto fail;
>
>      if ( (rc = p2m_init(d)) != 0 )
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..cb33f75 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -322,6 +322,11 @@ struct xen_arch_domainconfig {
>       *
>       */
>      uint32_t clock_frequency;
> +    /*
> +     * IN
> +     * Inform the hypervisor that the use of IOMMU is expected for this domain.

I would simplify to : "IOMMU is expected to be used for this domain".

> +     */
> +    uint8_t use_iommu;
>  };
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>
>

Cheers,

-- 
Julien Grall

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