Re: [PATCH 10/13] iommu/io-pgtable: Replace ->tlb_add_flush() with ->tlb_add_page()

[Robin Murphy <robin.murphy@arm.com>]

On 21/08/2019 13:05, Will Deacon wrote:
> Hi Robin,
> 
> Thanks for looking at this.
> 
> On Wed, Aug 21, 2019 at 12:42:11PM +0100, Robin Murphy wrote:
>> On 14/08/2019 18:56, Will Deacon wrote:
>>> The ->tlb_add_flush() callback in the io-pgtable API now looks a bit
>>> silly:
>>>
>>>     - It takes a size and a granule, which are always the same
>>>     - It takes a 'bool leaf', which is always true
>>>     - It only ever flushes a single page
>>>
>>> With that in mind, replace it with an optional ->tlb_add_page() callback
>>> that drops the useless parameters.
>>>
>>> Signed-off-by: Will Deacon <will@kernel.org>
> 
> [...]
> 
>>> -static const struct iommu_flush_ops arm_smmu_s2_tlb_ops_v2 = {
>>> -	.tlb_flush_all	= arm_smmu_tlb_inv_context_s2,
>>> -	.tlb_flush_walk	= arm_smmu_tlb_inv_walk,
>>> -	.tlb_flush_leaf	= arm_smmu_tlb_inv_leaf,
>>> -	.tlb_add_flush	= arm_smmu_tlb_inv_range_nosync,
>>> -	.tlb_sync	= arm_smmu_tlb_sync_context,
>>> +static const struct arm_smmu_flush_ops arm_smmu_s2_tlb_ops_v2 = {
>>> +	.tlb = {
>>> +		.tlb_flush_all	= arm_smmu_tlb_inv_context_s2,
>>> +		.tlb_flush_walk	= arm_smmu_tlb_inv_walk,
>>> +		.tlb_flush_leaf	= arm_smmu_tlb_inv_leaf,
>>> +		.tlb_add_page	= arm_smmu_tlb_add_page,
>>> +		.tlb_sync	= arm_smmu_tlb_sync_context,
>>> +	},
>>> +	.tlb_inv_range		= arm_smmu_tlb_inv_range_nosync,
>>>    };
>>> -static const struct iommu_flush_ops arm_smmu_s2_tlb_ops_v1 = {
>>> -	.tlb_flush_all	= arm_smmu_tlb_inv_context_s2,
>>> -	.tlb_flush_walk	= arm_smmu_tlb_inv_walk,
>>> -	.tlb_flush_leaf	= arm_smmu_tlb_inv_leaf,
>>> -	.tlb_add_flush	= arm_smmu_tlb_inv_vmid_nosync,
>>> -	.tlb_sync	= arm_smmu_tlb_sync_vmid,
>>> +static const struct arm_smmu_flush_ops arm_smmu_s2_tlb_ops_v1 = {
>>> +	.tlb = {
>>> +		.tlb_flush_all	= arm_smmu_tlb_inv_context_s2,
>>> +		.tlb_flush_walk	= arm_smmu_tlb_inv_walk,
>>> +		.tlb_flush_leaf	= arm_smmu_tlb_inv_leaf,
>>
>> Urgh, that ain't right... :(
>>
>> Sorry I've only spotted it now while trying to rebase onto Joerg's queue,
>> but we can't use either of those callbacks for v1 stage 2 since the
>> registers they access don't exist. I'll spin a fixup patch first, then come
>> back to the question of whether it's more practical to attempt merging my v2
>> or concede to rebasing a v3.
> 
> Although the code is quite difficult to follow, I think it's alright because
> the tlb_flush_{walk,leaf} functions just indirect back through the
> tlb_inv_range callback. This patch is supposed to be a big NOP moving
> drivers over to the new API, but not actually exploiting its benefits.

Ah, sorry, I did indeed fail the reading comprehension test - too many 
levels of indirection...

On second reading I agree that this probably should work out OK (other 
than perhaps a performance hit from chaining more indirect branches). 
I've noted on my to-do list to come back and clean up arm_smmu_flush_ops 
for next cycle, but for now I'll get back to the more pressing matters.

Thanks,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu