On 27/03/17 13:18, Robin Murphy wrote: > Hi Jean-Philippe, > > On 27/02/17 19:54, Jean-Philippe Brucker wrote: >> Reintroduce smmu_group. This structure was removed during the generic DT >> bindings rework, but will be needed when implementing PCIe ATS, to lookup >> devices attached to a given domain. >> >> When unmapping from a domain, we need to send an invalidation to all >> devices that could have stored the mapping in their ATC. It would be nice >> to use IOMMU API's iommu_group_for_each_dev, but that list is protected by >> group->mutex, which we can't use because atc_invalidate won't be allowed >> to sleep. So add a list of devices, protected by a spinlock. > > Much as I dislike that particular duplication, with patch #4 in mind I > think there's a more fundamental problem - since the primary reason for > multi-device groups is lack of ACS, is there any guarantee that ATS > support/enablement will be actually be homogeneous across any old set of > devices in that situation, and thus valid to evaluate at the iommu_group > level? I doubt that support of ATS in group would be homogeneous, so I also test ats bit in pci_device structure before sending a command, making the group check an optimization. > That said, looking at how things end up at the top commit, I think this > is fairly easily sidestepped. We have this pattern a few times: > > spin_lock_irqsave(&smmu_domain->groups_lock) > list_for_each_entry(&smmu_domain->groups) > spin_lock(&smmu_group->devices_lock) > list_for_each_entry(&smmu_group->devices) > do a thing for each device in the domain... > > which strongly suggests that we'd be better off just linking the devices > to the domain directly - which would also let us scope ats_enabled to > the per-device level which seems safer than per-group as above. And if > only devices with ATS enabled are added to a domain's list in the first > place, then ATC invalidate gets even simpler too. > > The only other uses I can see are of smmu_group->domain, which always > looks to be directly equivalent to to_smmu_domain(iommu_group->domain). > Overall it really looks like the smmu_group abstraction winds up making > the end result more complex than it needs to be. Yes in retrospect, smmu_group hardly seems necessary. I'll work on getting rid of it. Thanks, Jean-Philippe
