Re: [PATCH v4 7/8] xen/iommu: smmu: Advertise when the SMMU support coherent table walk

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-02-25 at 18:53 +0000, 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

"mainstream".

> supporting coherent table walk. If not, we may need to enable this feature
> per-domain.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     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 v4:
>         - Browse the list to retrieve the last SMMU added.
>         - Take the spinlock
> 
>     Changes in v3:
>         - Patch added
> ---
>  xen/drivers/passthrough/arm/smmu.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index d01a26a..b478463 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2531,6 +2531,13 @@ MODULE_LICENSE("GPL v2");
>  /* Xen only supports stage-2 translation, so force the value to 2. */
>  static int force_stage = 2;
>  
> +/*
> + * Platform features. It indicates the list of features supported by all the
> + * SMMUs.

s/the//

> + * Actually we only care about coherent table walk.
> + */
> +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
> +
>  static void arm_smmu_iotlb_flush_all(struct domain *d)
>  {
>  	struct arm_smmu_xen_domain *smmu_domain = domain_hvm_iommu(d)->arch.priv;
> @@ -2668,6 +2675,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.

s/The feature c/C/

or "The coherent walk feature..." (I prefer the former since it is
briefer)

> +	 */
> +	if (platform_features & ARM_SMMU_FEAT_COHERENT_WALK)
> +		iommu_set_feature(d, IOMMU_FEAT_COHERENT_WALK);
> +
>  	return 0;
>  }
>  
> @@ -2742,6 +2756,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
> @@ -2755,6 +2770,20 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev,
>  
>  	iommu_set_ops(&arm_smmu_iommu_ops);
>  
> +	/* Find the last SMMU added and retrieve its features. */

This comment no longer applies, I think?

> +	spin_lock(&arm_smmu_devices_lock);
> +	list_for_each_entry(smmu, &arm_smmu_devices, list) {
> +		if (smmu->dev == &dev->dev)
> +			goto found;

Please try and avoid goto. In this case I think 
     {
         platform_features &= smmu->features;
         break;
     }

within the if would be fine, combined with dropping the following BUG().

Alternatively you might prefer to provide a helper function to lookup an
smmu by &dev->dev.


> +	}
> +
> +	BUG();
> +
> +found:
> +	platform_features &= smmu->features;
> +
> +	spin_unlock(&arm_smmu_devices_lock);
> +
>  	return 0;
>  }
>