Re: [PATCH v3 13/13] xen/iommu: smmu: Advertise when the SMMU support coherent table walk

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

On 10/02/2015 09:36, Julien Grall wrote:
> Hi Stefano,
>
> On 06/02/2015 22:06, Stefano Stabellini wrote:
>> On Fri, 30 Jan 2015, Julien Grall wrote:
>>> When SMMU doesn't support coherent table walk, Xen may need to clean
>>> updated PT (see commit 4c5f4cb "xen/arm: p2m: Clean cache PT when the
>>> IOMMU doesn't support coherent walk").
>>>
>>> If one SMMU of the platform doesn't support coherent table walk, the
>>> feature is disabled for the whole platform. This is because device is
>>> assigned to a domain after the page table are populated.
>>>
>>> This could impact performance on domain which doesn't use device
>>> passthrough. But, as the spec strongly recommends the support of this
>>> feature for maintstream platform, I expect server will always have SMMUs
>>> supporting coherent table walk. If not, we may need to enable this
>>> feature
>>> per-domain.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> ---
>>>      I've just noticed that the support on the previous driver (i.e in
>>>      Xen 4.5) may incorrectly expose this feature when all the SMMUs is
>>>      not supporting coherent table walk.
>>>
>>>      I'm not sure, if I should send a patch for it.
>>>
>>>      Also I didn't squash this patch into "xen/iommu: smmu: Add Xen
>>> specific
>>>      code to be able to use the driver" to help for review and to catch
>>>      possible error in this patch.
>>>
>>>      Changes in v3:
>>>          - Patch added
>>> ---
>>>   xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/xen/drivers/passthrough/arm/smmu.c
>>> b/xen/drivers/passthrough/arm/smmu.c
>>> index 373eee8..f4c7e49 100644
>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>> @@ -577,6 +577,13 @@ struct arm_smmu_domain {
>>>       spinlock_t            lock;
>>>   };
>>>
>>> +/*
>>> + * Xen: Platform features. It indicates the list of features support
>>> by all the
>>> + * SMMUs.
>>> + * Actually we only care about coherent table walk.
>>> + */
>>> +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
>>> +
>>>   /* Xen: Dummy iommu_domain */
>>>   struct iommu_domain
>>>   {
>>> @@ -2810,6 +2817,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.
>>> +     */
>>> +    if (platform_features & ARM_SMMU_FEAT_COHERENT_WALK)
>>> +        iommu_set_feature(d, IOMMU_FEAT_COHERENT_WALK);
>>> +
>>>       return 0;
>>>   }
>>
>> As long as you are sure that arm_smmu_iommu_domain_init is called after
>> all the iommus have been initialized by arm_smmu_dt_init, then this
>> patch is correct.
>
> We don't support SMMU hotplug on Xen :). All the SMMUs are initialize
> via iommu_setup before DOM0 is created.

Though, I could update the commit message to explain it.

-- 
Julien Grall