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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

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