On Fri, Apr 05, 2019 at 05:35:52PM +0100, Jean-Philippe Brucker wrote: > On 04/04/2019 15:39, Will Deacon wrote: > > On Wed, Mar 20, 2019 at 05:36:32PM +0000, Jean-Philippe Brucker wrote: > >> When removing a mapping from a domain, we need to send an invalidation to > >> all devices that might have stored it in their Address Translation Cache > >> (ATC). In addition when updating the context descriptor of a live domain, > >> we'll need to send invalidations for all devices attached to it. > >> > >> Maintain a list of devices in each domain, protected by a spinlock. It is > >> updated every time we attach or detach devices to and from domains. > >> > >> It needs to be a spinlock because we'll invalidate ATC entries from > >> within hardirq-safe contexts, but it may be possible to relax the read > >> side with RCU later. > >> > >> Signed-off-by: Jean-Philippe Brucker <[email protected]> > >> --- > >> drivers/iommu/arm-smmu-v3.c | 28 ++++++++++++++++++++++++++++ > >> 1 file changed, 28 insertions(+) > >> > >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > >> index d3880010c6cf..66a29c113dbc 100644 > >> --- a/drivers/iommu/arm-smmu-v3.c > >> +++ b/drivers/iommu/arm-smmu-v3.c > >> @@ -594,6 +594,11 @@ struct arm_smmu_device { > >> struct arm_smmu_master_data { > >> struct arm_smmu_device *smmu; > >> struct arm_smmu_strtab_ent ste; > >> + > >> + struct arm_smmu_domain *domain; > >> + struct list_head domain_head; > >> + > >> + struct device *dev; > >> }; > >> > >> /* SMMU private data for an IOMMU domain */ > >> @@ -618,6 +623,9 @@ struct arm_smmu_domain { > >> }; > >> > >> struct iommu_domain domain; > >> + > >> + struct list_head devices; > >> + spinlock_t devices_lock; > >> }; > >> > >> struct arm_smmu_option_prop { > >> @@ -1493,6 +1501,9 @@ static struct iommu_domain > >> *arm_smmu_domain_alloc(unsigned type) > >> } > >> > >> mutex_init(&smmu_domain->init_mutex); > >> + INIT_LIST_HEAD(&smmu_domain->devices); > >> + spin_lock_init(&smmu_domain->devices_lock); > > > > I'm wondering whether we can't take this a bit further and re-organise the > > data structures to make this a little simpler overall. Something along the > > lines of: > > > > struct arm_smmu_master_data { > > struct list_head list; // masters in the same > > domain > > struct arm_smmu_device *smmu; > > unsigned int num_sids; > > u32 *sids; // Points into fwspec > > struct arm_smmu_domain *domain; // NULL -> !assigned > > }; > > > > and then just add a list_head into struct arm_smmu_domain to track the > > masters that have been attached (if you're feeling brave, you could put > > this into the s1_cfg). > > I'm not sure about that last bit, shouldn't the list of masters apply to > both s1 and s2?
I was assuming that (a) we were only using ATS with stage-1 and (b) we only need the masters list for ATC invalidation. Did I go wrong somewhere? > > The ATC invalidation logic would then be: > > > > - Detaching a device: walk over the sids from the master data > > - Unmapping a range from a domain: walk over the attached masters > > > > I think this would also allow us to remove struct arm_smmu_strtab_ent > > completely. > > Makes sense, it does work and simplifies the structures. It makes the > PASID and PRI patches slightly nicer as well. I'll resend once my tests > complete. Brill, thanks for giving it a go so quickly. Will _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
