Re: [PATCH 2/2] xen/arm: smmuv3: Advertise coherent table walk if supported

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 12/05/2023 18:59, Ayan Kumar Halder wrote:
> Hi Michal,
> 
> On 12/05/2023 15:35, Michal Orzel wrote:
>> At the moment, even in case of a SMMU being I/O coherent, we clean the
>> updated PT as a result of not advertising the coherency feature. SMMUv3
>> coherency feature means that page table walks, accesses to memory
>> structures and queues are I/O coherent (refer ARM IHI 0070 E.A, 3.15).
>>
>> Follow the same steps that were done for SMMU v1,v2 driver by the commit:
>> 080dcb781e1bc3bb22f55a9dfdecb830ccbabe88
>>
>> The same restrictions apply, meaning that in order to advertise coherent
>> table walk platform feature, all the SMMU devices need to report coherency
>> feature. This is because the page tables (we are sharing them with CPU)
>> are populated before any device assignment and in case of a device being
>> behind non-coherent SMMU, we would have to scan the tables and clean
>> the cache.
>>
>> It is to be noted that the SBSA/BSA (refer ARM DEN0094C 1.0C, section D)
>> requires that all SMMUv3 devices support I/O coherency.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> There are very few platforms out there with SMMUv3 but I have never seen
>> a SMMUv3 that is not I/O coherent.
>> ---
>>   xen/drivers/passthrough/arm/smmu-v3.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index bf053cdb6d5c..2adaad0fa038 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -2526,6 +2526,15 @@ static const struct dt_device_match 
>> arm_smmu_of_match[] = {
>>   };
>>   
>>   /* Start of Xen specific code. */
>> +
>> +/*
>> + * Platform features. It indicates the list of features supported by all
>> + * SMMUs. Actually we only care about coherent table walk, which in case of
>> + * SMMUv3 is implied by the overall coherency feature (refer ARM IHI 0070 
>> E.A,
>> + * section 3.15 and SMMU_IDR0.COHACC bit description).
>> + */
>> +static uint32_t platform_features = ARM_SMMU_FEAT_COHERENCY;
>> +
>>   static int __must_check arm_smmu_iotlb_flush_all(struct domain *d)
>>   {
>>      struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
>> @@ -2708,8 +2717,12 @@ static int arm_smmu_iommu_xen_domain_init(struct 
>> domain *d)
>>      INIT_LIST_HEAD(&xen_domain->contexts);
>>   
>>      dom_iommu(d)->arch.priv = xen_domain;
>> -    return 0;
>>   
>> +    /* Coherent walk can be enabled only when all SMMUs support it. */
>> +    if (platform_features & ARM_SMMU_FEAT_COHERENCY)
>> +            iommu_set_feature(d, IOMMU_FEAT_COHERENT_WALK);
>> +
>> +    return 0;
>>   }
>>   
> All good till here.
>>   static void arm_smmu_iommu_xen_domain_teardown(struct domain *d)
>> @@ -2738,6 +2751,7 @@ static __init int arm_smmu_dt_init(struct 
>> dt_device_node *dev,
>>                              const void *data)
>>   {
>>      int rc;
>> +    const struct arm_smmu_device *smmu;
>>   
>>      /*
>>       * Even if the device can't be initialized, we don't want to
>> @@ -2751,6 +2765,14 @@ static __init int arm_smmu_dt_init(struct 
>> dt_device_node *dev,
>>   
>>      iommu_set_ops(&arm_smmu_iommu_ops);
>>   
>> +    /* Find the just added SMMU and retrieve its features. */
>> +    smmu = arm_smmu_get_by_dev(dt_to_dev(dev));
>> +
>> +    /* It would be a bug not to find the SMMU we just added. */
>> +    BUG_ON(!smmu);
>> +
>> +    platform_features &= smmu->features;
>> +
> 
> Can you explain this change in the commit message ?
I think it is already explained by saying that in order to advertise the 
*platform* feature, all
SMMUs need to report it. If at least one doesn't, the feature is disabled. This 
is exactly
what this line is doing. It is comparing platform features with a just probed 
SMMU features (arm_smmu_dt_init()
is called for each SMMU).

~Michal