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 <jean-philippe.brucker@xxxxxxx>
> >> ---
> >> 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
