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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 15/05/2023 10:56, Julien Grall wrote:
> 
> 
> Hi,
> 
> 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;
> 
> AFAICT, this variable is not meant to change after boot. So please add
> the attribute __ro_after_init.
Yes, that makes total sense. After probing this variable is not meant to be 
modified.
Is it something that can be done on commit or would you want me to respin this 
patch?

~Michal