Re: [PATCH v5 12/14] xen/arm: p2m: Clean cache PT when the IOMMU doesn't support coherent walk

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

Hi Ian,

On 14/05/14 08:18, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -344,6 +344,17 @@ void iommu_crash_shutdown(void)
>>       iommu_enabled = iommu_intremap = 0;
>>   }
>>
>> +bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
>> +{
>> +    const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops;
>> +    uint32_t features = 0;
>
> Please here and further down - don't use fixed width type unless
> you really need to.
>
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -67,6 +67,14 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
>>                      unsigned int flags);
>>   int iommu_unmap_page(struct domain *d, unsigned long gfn);
>>
>> +enum iommu_feature
>> +{
>> +    IOMMU_FEAT_COHERENT_WALK = 1,
>
> Why 1? Enumerations are defined to start at zero, and starting at
> zero is what you really want here. Don't specify a value at all.

It's a mistake when I create the enum. I will drop the 1.

>> @@ -139,6 +147,7 @@ struct iommu_ops {
>>       void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
>>       void (*iotlb_flush_all)(struct domain *d);
>>       void (*dump_p2m_table)(struct domain *d);
>> +    uint32_t (*features)(struct domain *d);
>
> I think I said this on an earlier round already - for future extensibility
> this should return "const unsigned long *", and get accessed by the
> wrapper function using test_bit(). Or even better without an accessor
> function at all, just directly having a "const unsigned long *" field here.
> Unless of course the backend implementation - which isn't part of this
> patch - would have difficulty setting up a suitable bitfield during (early)
> initialization.

The SMMU drivers handle multiple SMMUs. Each SMMU can have different 
specifications (e.g coherent walk support or not).

As each domain doesn't use all SMMUs, we might be able to avoid flushing 
PT on some of them. That's why I've choose to use a callback with the 
domain in parameter.

I don't like the solution which return "unsigned long *" because we are 
assuming the driver will always a valid pointer (for instance with 2 
unsigned long), even if he doesn't need it.

Regards,

-- 
Julien Grall