From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2] xen/passthrough: Support a single iommu_domain per xen domain per SMMU Date: Mon, 23 Mar 2015 21:53:00 +0000 Message-ID: <55108B3C.2030708@linaro.org> References: <1427118384-7312-1-git-send-email-robert.vanvossen@dornerworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YaAHQ-0001ER-HZ for xen-devel@lists.xenproject.org; Mon, 23 Mar 2015 21:53:04 +0000 Received: by wixw10 with SMTP id w10so76408008wix.0 for ; Mon, 23 Mar 2015 14:53:02 -0700 (PDT) In-Reply-To: <1427118384-7312-1-git-send-email-robert.vanvossen@dornerworks.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Robbie VanVossen , xen-devel@lists.xenproject.org Cc: julien.grall@citrix.com, edgar.iglesias@xilinx.com, Josh.Whitehead@dornerworks.com, Ian.Campbell@citrix.com, Stefano.Stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hello Robert, On 23/03/2015 13:46, 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 occured 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 > > --- > 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 | 98 +++++++++++++++++++++++++++--------- > 1 file changed, 74 insertions(+), 24 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index a7a7da9..1a3de00 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. > * */ > @@ -2569,7 +2570,9 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn, > { > struct iommu_domain *domain; > struct arm_smmu_xen_domain *xen_domain; > - int ret; > + struct arm_smmu_device *smmu; > + int ret = 0; > + int existing_ctxt_fnd = 0; You can use a bool_t here. > > xen_domain = domain_hvm_iommu(d)->arch.priv; > > @@ -2585,36 +2588,77 @@ 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; > + if (!list_empty(&xen_domain->contexts)) { > + smmu = find_smmu_for_device(dev); > + if (!smmu) { > + dev_err(dev, "cannot find SMMU\n"); > + ret = -ENXIO; > + goto out; > + } > > - ret = arm_smmu_domain_init(domain); > - if (ret) > - goto err_dom_init; > + /* > + * 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) > + { Coding style. This should be if (domain->priv->smmu == smmu) { > + /* > + * We have found a context already associated with the > + * same xen domain and SMMU > + */ > + ret = arm_smmu_attach_dev(domain, dev); > + if (ret) { > + dev_err(dev, > + "cannot attach device to already existing iommu_domain\n"); We don't print an error message for new domain, I don't think this one is useful. > + goto out; > + } > + atomic_inc(&domain->ref); Introducing an helper to get the iommu_domain would simply the workflow. That would give smth like: domain = arm_smmu_get_domain(d, dev); if (!domain) { allocate domain Add to list dev } arm_smmu_attach_dev(domain, dev); atomic_inc(&domain->ref); out: if (failed && atomic == 0) destroy iommu_domain. The destroy iommu_domain could be shared with the one in deassign_dev. > + > + existing_ctxt_fnd = 1; > + break; > + } > + } > + } > > - domain->priv->cfg.domain = d; > + if(!existing_ctxt_fnd){ > > - ret = arm_smmu_attach_dev(domain, dev); > - if (ret) > - goto err_attach_dev; > + domain = xzalloc(struct iommu_domain); > + if (!domain) { > + ret = -ENOMEM; > + goto out; > + } > > - spin_lock(&xen_domain->lock); > - /* Chain the new context to the domain */ > - list_add(&domain->list, &xen_domain->contexts); > - spin_unlock(&xen_domain->lock); > + ret = arm_smmu_domain_init(domain); > + if (ret) > + goto err_dom_init; > > - return 0; > + domain->priv->cfg.domain = d; > + > + ret = arm_smmu_attach_dev(domain, dev); > + if (ret) > + goto err_attach_dev; > + atomic_inc(&domain->ref); > + > + /* Chain the new context to the domain */ > + list_add(&domain->list, &xen_domain->contexts); > + > + } > + > + goto out; > > err_attach_dev: > arm_smmu_domain_destroy(domain); > err_dom_init: > xfree(domain); > +out: > + spin_unlock(&xen_domain->lock); > > return ret; > } > @@ -2631,14 +2675,20 @@ static int arm_smmu_deassign_dev(struct domain *d, struct device *dev) > return -ESRCH; > } > > + spin_lock(&xen_domain->lock); > + > arm_smmu_detach_dev(domain, dev); > + atomic_dec(&domain->ref); > > - spin_lock(&xen_domain->lock); > - list_del(&domain->list); > - spin_unlock(&xen_domain->lock); > + if (domain->ref.counter == 0) > + { Coding style: if (...) { > + list_del(&domain->list); > > - arm_smmu_domain_destroy(domain); > - xfree(domain); > + arm_smmu_domain_destroy(domain); > + xfree(domain); > + } > + > + spin_unlock(&xen_domain->lock); > > return 0; > } > Regards, -- Julien Grall