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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 15/05/2023 11:06, Julien Grall wrote:
> 
> 
> On 15/05/2023 10:03, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>>
>> 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?
> 
> I can do it on commit. With that:
> 
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks,
Bare in mind that Rahul responded in HTML so there will be a <mailto when using 
b4.

~Michal