[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.