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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.05.14 at 15:11, <julien.grall@xxxxxxxxxx> wrote:
> On 05/14/2014 02:08 PM, Jan Beulich wrote:
>>>>> On 14.05.14 at 14:45, <julien.grall@xxxxxxxxxx> wrote:
>>> On 05/14/2014 10:25 AM, Jan Beulich wrote:
>>>>>>> On 14.05.14 at 11:09, <julien.grall@xxxxxxxxxx> wrote:
>>>>>>> @@ -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.
>>>>
>>>> In that case simply make this a bitmap embedded in struct iommu_ops,
>>>> which the driver has to populate suitably early. That way the individual
>>>> drivers don't need to care about the eventually growing size.
>>>
>>> That doesn't really help me. Because a same platform can have a mix of
>>> SMMU support coherent walk or not.
>>>
>>> For optimization it would be better to have a per-domain feature.
>> 
>> Oh, right, I didn't pay attention to you passing a domain into the
>> query.
>> 
>>> What about creating a callback:
>>> iommu_has_feature(struct domain *d, enum iommu_feature)
>>>
>>> and let the driver handling the feature?
>> 
>> That sounds fine as long as you don't expect such to sit on any hot
>> path (indirect calls are expensive not only on x86 I think).
> 
> Actually, it's called during an hot patch (every time a page is mapped
> in the p2m).

In that case, why don't you put the feature flag(s) somewhere in
struct domain or struct arch_domain (or some sub-structure thereof)?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel