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@lists.xen.org
http://lists.xen.org/xen-devel