[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |