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
