Re: [PATCH v3 8/9] iommu/arm-smmu-v3: Add support for PCI ATS

Jean-Philippe Brucker
2019-07-02T14:59:46Z

On 01/07/2019 18:41, Robin Murphy wrote:
> Hi Jean-Philippe,
>
> I realise it's a bit late for a "review", but digging up the original
> patch seemed as good a place as any to raise this...
>
> On 17/04/2019 19:24, Jean-Philippe Brucker wrote:
> [...]
>> @@ -1740,6 +1906,9 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>>
>> master->domain = NULL;
>> arm_smmu_install_ste_for_dev(master);
>> +
>> + /* Disabling ATS invalidates all ATC entries */
>> + arm_smmu_disable_ats(master);
>> }
>
> Is that actually true? I had initially overlooked this entirely while
> diagnosing something else and thought that we were missing any ATC
> invalidation on detach at all, but even having looked again I'm not
> entirely convinced it's bulletproof.
>
> Firstly, the ATS spec only seems to say that *enabling* the ATS
> capability invalidates all ATC entries, although I think any corner
> cases that that alone opens up should be at best theoretical. More
> importantly though, pci_disable_ats() might not actually touch the
> capability - given that, it seems possible to move a VF to a new domain,
> and if it's not reset, end up preserving now-bogus ATC entries despite
> the old domain being torn down and freed. Do we need an explicit ATC
> invalidation here to be 100% safe, or is there something else I'm missing?

Good points, yes the comment is wrong and it looks like we need an
explicit invalidation given the current pci_disable_ats()
implementation. I'll send a fix shortly.

Thanks,
Jean