Re: [Xen-devel] [PATCH v3 13/13] xen/iommu: smmu: Advertise when the SMMU support coherent table walk

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 30 Jan 2015, Julien Grall wrote:
> When SMMU doesn't support coherent table walk, Xen may need to clean
> updated PT (see commit 4c5f4cb "xen/arm: p2m: Clean cache PT when the
> IOMMU doesn't support coherent walk").
> 
> If one SMMU of the platform doesn't support coherent table walk, the
> feature is disabled for the whole platform. This is because device is
> assigned to a domain after the page table are populated.
> 
> This could impact performance on domain which doesn't use device
> passthrough. But, as the spec strongly recommends the support of this
> feature for maintstream platform, I expect server will always have SMMUs
> supporting coherent table walk. If not, we may need to enable this feature
> per-domain.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     I've just noticed that the support on the previous driver (i.e in
>     Xen 4.5) may incorrectly expose this feature when all the SMMUs is
>     not supporting coherent table walk.
> 
>     I'm not sure, if I should send a patch for it.
> 
>     Also I didn't squash this patch into "xen/iommu: smmu: Add Xen specific
>     code to be able to use the driver" to help for review and to catch
>     possible error in this patch.
> 
>     Changes in v3:
>         - Patch added
> ---
>  xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 373eee8..f4c7e49 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -577,6 +577,13 @@ struct arm_smmu_domain {
>       spinlock_t                      lock;
>  };
>  
> +/*
> + * Xen: Platform features. It indicates the list of features support by all 
> the
> + * SMMUs.
> + * Actually we only care about coherent table walk.
> + */
> +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
> +
>  /* Xen: Dummy iommu_domain */
>  struct iommu_domain
>  {
> @@ -2810,6 +2817,13 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
>  
>       domain_hvm_iommu(d)->arch.priv = xen_domain;
>  
> +     /*
> +      * The feature coherent walk can be enabled only when all SMMUs
> +      * support it.
> +      */
> +     if (platform_features & ARM_SMMU_FEAT_COHERENT_WALK)
> +             iommu_set_feature(d, IOMMU_FEAT_COHERENT_WALK);
> +
>       return 0;
>  }

As long as you are sure that arm_smmu_iommu_domain_init is called after
all the iommus have been initialized by arm_smmu_dt_init, then this
patch is correct.


> @@ -2885,6 +2899,7 @@ static __init int arm_smmu_dt_init(struct 
> dt_device_node *dev,
>                                  const void *data)
>  {
>       int rc;
> +     struct arm_smmu_device *smmu;
>  
>       /*
>        * Even if the device can't be initialized, we don't want to
> @@ -2896,6 +2911,16 @@ static __init int arm_smmu_dt_init(struct 
> dt_device_node *dev,
>       if ( !rc )
>               iommu_set_ops(&arm_smmu_iommu_ops);
>  
> +     /*
> +      * The last added SMMU is the first element of arm_smmu_devices.
> +      * It's not necessary to take the lock because only the boot CPU is
> +      * initialized the SMMU devices.
> +      */
> +     smmu = list_entry(arm_smmu_devices.next, typeof(*smmu), list);
> +     ASSERT(smmu != NULL);
> +
> +     platform_features &= smmu->features;
> +
>       return rc;
>  }
>  
> -- 
> 2.1.4
> 

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