Re: [Xen-devel] [PATCH v4] xen/passthrough: Support a single iommu_domain per xen domain per SMMU

2015-03-31 Thread Ian Campbell
On Tue, 2015-03-24 at 16:48 -0400, Robbie VanVossen wrote:
> If multiple devices are being passed through to the same domain and they
> share a single SMMU, then they only require a single iommu_domain.
> 
> In arm_smmu_assign_dev, before a new iommu_domain is created, the
> xen_domain->contexts is checked for any iommu_domains that are already
> assigned to device that uses the same SMMU as the current device. If one
> is found, attach the device to that iommu_domain. If a new one isn't
> found, create a new iommu_domain just like before.
> 
> The arm_smmu_deassign_dev function assumes that there is a single
> device per iommu_domain. This meant that when the first device was
> deassigned, the iommu_domain was freed and when another device was
> deassigned a crash occurred in xen.
> 
> To fix this, a reference counter was added to the iommu_domain struct.
> When an arm_smmu_xen_device references an iommu_domain, the
> iommu_domains ref is incremented. When that reference is removed, the
> iommu_domains ref is decremented. The iommu_domain will only be freed
> when the ref is 0.
> 
> Signed-off-by: Robbie VanVossen 
> Reviewed-by: Julien Grall 

I was expecting this to come back in a new version of  Julien's
passthrough series, but Julien prodded me to say that this patch didn't
actually have any dependency/interaction with his passthrough series
like I thought, so it can go in now. 

So Acked + applied, sorry for the delay.



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


[Xen-devel] [PATCH v4] xen/passthrough: Support a single iommu_domain per xen domain per SMMU

2015-03-24 Thread Robbie VanVossen
If multiple devices are being passed through to the same domain and they
share a single SMMU, then they only require a single iommu_domain.

In arm_smmu_assign_dev, before a new iommu_domain is created, the
xen_domain->contexts is checked for any iommu_domains that are already
assigned to device that uses the same SMMU as the current device. If one
is found, attach the device to that iommu_domain. If a new one isn't
found, create a new iommu_domain just like before.

The arm_smmu_deassign_dev function assumes that there is a single
device per iommu_domain. This meant that when the first device was
deassigned, the iommu_domain was freed and when another device was
deassigned a crash occurred in xen.

To fix this, a reference counter was added to the iommu_domain struct.
When an arm_smmu_xen_device references an iommu_domain, the
iommu_domains ref is incremented. When that reference is removed, the
iommu_domains ref is decremented. The iommu_domain will only be freed
when the ref is 0.

Signed-off-by: Robbie VanVossen 
Reviewed-by: Julien Grall 
---
Changed since v3:
  * Fixed formatting
Changed since v2:
  * Fixed coding style
  * Removed an unnecessary error message
  * Added some helper functions to clean up the workflow in arm_smmu_assign_dev 
a bit
Changed since v1:
  * Fixed coding style for comments
  * Move increment/decrement outside of attach/detach functions
  * Expanded xen_domain->lock to protect more of the assign/deassign
functions
  * Removed iommu_domain add/remove_device functions
---
 xen/drivers/passthrough/arm/smmu.c |   99 ++--
 1 file changed, 71 insertions(+), 28 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index a7a7da9..8a9b58b 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -223,6 +223,7 @@ struct iommu_domain
/* Runtime SMMU configuration for this iommu_domain */
struct arm_smmu_domain  *priv;
 
+   atomic_t ref;
/* Used to link iommu_domain contexts for a same domain.
 * There is at least one per-SMMU to used by the domain.
 * */
@@ -2564,12 +2565,45 @@ static void arm_smmu_iotlb_flush(struct domain *d, 
unsigned long gfn,
 arm_smmu_iotlb_flush_all(d);
 }
 
+static struct iommu_domain *arm_smmu_get_domain(struct domain *d,
+   struct device *dev)
+{
+   struct iommu_domain *domain;
+   struct arm_smmu_xen_domain *xen_domain;
+   struct arm_smmu_device *smmu;
+
+   xen_domain = domain_hvm_iommu(d)->arch.priv;
+
+   smmu = find_smmu_for_device(dev);
+   if (!smmu)
+   return NULL;
+
+   /*
+* Loop through the &xen_domain->contexts to locate a context
+* assigned to this SMMU
+*/
+   list_for_each_entry(domain, &xen_domain->contexts, list) {
+   if (domain->priv->smmu == smmu)
+   return domain;
+   }
+
+   return NULL;
+
+}
+
+static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
+{
+   list_del(&domain->list);
+   arm_smmu_domain_destroy(domain);
+   xfree(domain);
+}
+
 static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
   struct device *dev)
 {
struct iommu_domain *domain;
struct arm_smmu_xen_domain *xen_domain;
-   int ret;
+   int ret = 0;
 
xen_domain = domain_hvm_iommu(d)->arch.priv;
 
@@ -2585,36 +2619,44 @@ static int arm_smmu_assign_dev(struct domain *d, u8 
devfn,
return ret;
}
 
+   spin_lock(&xen_domain->lock);
+
/*
-* TODO: Share the context bank (i.e iommu_domain) when the device is
-* under the same SMMU as another device assigned to this domain.
-* Would it useful for PCI
+* Check to see if a context bank (iommu_domain) already exists for
+* this xen domain under the same SMMU
 */
-   domain = xzalloc(struct iommu_domain);
-   if (!domain)
-   return -ENOMEM;
+   domain = arm_smmu_get_domain(d, dev);
+   if (!domain) {
 
-   ret = arm_smmu_domain_init(domain);
-   if (ret)
-   goto err_dom_init;
+   domain = xzalloc(struct iommu_domain);
+   if (!domain) {
+   ret = -ENOMEM;
+   goto out;
+   }
 
-   domain->priv->cfg.domain = d;
+   ret = arm_smmu_domain_init(domain);
+   if (ret) {
+   xfree(domain);
+   goto out;
+   }
 
-   ret = arm_smmu_attach_dev(domain, dev);
-   if (ret)
-   goto err_attach_dev;
+   domain->priv->cfg.domain = d;
 
-   spin_lock(&xen_domain->lock);
-   /* Chain the new context to the domain */
-   list_add(&domain->list, &xen_domain->contexts);
-   spin_unlock(&xen_domain