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

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 02/03/15 13:10, Ian Campbell wrote:
>> 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)

I will use the former.

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

I think it's useful to have a comment explaining why we are retrieving
the SMMU.

>> +	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;
>      }

I though about this solution but it doesn't say if for some reason we
miss to find the SMMU.

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

I would like to keep the BUG(). Because this would be a mistake to not
find the last SMMU added.

I will move to an helper function.

Regards,

-- 
Julien Grall