Re: [Xen-devel] [PATCH] xen/passthrough: Support a single iommu_domain(context bank) per xen domain per SMMU

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 20/03/2015 13:56, Robert VanVossen wrote:

> > >    xen/drivers/passthrough/arm/smmu.c |  113 
> > > ++++++++++++++++++++++++++++--------
> > >    1 file changed, 88 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > index a7a7da9..9b46054 100644
> > [..]
> >
> > > @@ -1596,7 +1617,7 @@ static void arm_smmu_detach_dev(struct iommu_domain 
> > > *domain, struct device *dev)
> > >         if (!cfg)
> > >                 return;
> > >
> > > -       dev_iommu_domain(dev) = NULL;
> > > +       iommu_domain_remove_device(domain, dev);
> > >         arm_smmu_domain_remove_master(smmu_domain, cfg);
> > >    }
> > I'd like to avoid modifying arm_smmu_attach_dev and arm_smmu_detach_dev
> > as it's part of the Linux code.
> >
> > I think you can increment the reference counter in arm_smmu_assign_dev
> > and arm_smmu_deassign_dev. That would avoid some possible race condition
> > (see below).
> I can definitely increment the reference counter in arm_smmu_assign_dev, but
> arm_smmu_detach_dev doesn't return any results, so there isn't a guarantee that
> the iommu_domain has actually been dereferenced for the device.
AFAICT arm_smmu_detach_dev will only fail it's not able to find an 
iommu_group (i.e the list of Stream IDs) for a device.
On Xen case, the check in arm_smmu_deassign_dev guaranty us that the 
device is used by the SMMU and therefore an iommu_group exits for it.
So I think we can safely move the call in arm_smmu_detach_dev.

[..]

> > > +                                       dev_err(dev, "cannot attach device to 
> > > already existing iommu_domain\n");
> > > +                                       return -ENXIO;
> > > +                               }
> Is this an appropriate return error?
I would return the result of arm_smmu_attach_dev (i.e ret).

[..]

> > > @@ -2633,12 +2692,16 @@ static int arm_smmu_deassign_dev(struct domain *d, 
> > > struct device *dev)
> > >
> > >         arm_smmu_detach_dev(domain, dev);
> > >
> > > -       spin_lock(&xen_domain->lock);
> > > -       list_del(&domain->list);
> > > -       spin_unlock(&xen_domain->lock);
> > > +       if (domain->ref.counter == 0)
> > > +       {
> > There is a possible race with the previous function. arm_smmu_assign_dev
> > still have time to increment domain->ref and we will free a domain with
> > 1 device assigned.
> >
> > Overall, I think the 2 functions should be completely protected by the
> > xen_domain->lock.
> Agreed. I will move the locks around.
Thanks.

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel