From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert VanVossen Subject: Re: [PATCH] xen/passthrough: Support a single iommu_domain(context bank) per xen domain per SMMU Date: Fri, 20 Mar 2015 09:56:32 -0400 Message-ID: <550C2710.5080107@dornerworks.com> References: <1426789514-5892-1-git-send-email-robert.vanvossen@dornerworks.com> <550C14EF.8000002@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 1YYxQT-00065g-50 for xen-devel@lists.xenproject.org; Fri, 20 Mar 2015 13:57:25 +0000 In-Reply-To: <550C14EF.8000002@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , 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 On 3/20/2015 8:39 AM, Julien Grall wrote: > Hello Robert, > > Thank you for the patch. > No problem. :) >> 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. >> + 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) { >> + /* >> + * TODO: If arm_smmu_attach_dev fails, should we perform arm_smmu_domain_destroy, >> + * eventhough another smmu_master is configured correctly? If Not, what error >> + * code should we use >> + */ > > The failure may be because we don't have any stream register available. > So I don't think we should detach all the other devices within the same > context. > Agreed. >> + dev_err(dev, "cannot attach device to already existing iommu_domain\n"); >> + return -ENXIO; >> + } Is this an appropriate return error? >> + >> + existing_ctxt_fnd = 1; >> + break; >> + } >> + >> + } >> + spin_unlock(&xen_domain->lock); >> + } >> + >> + if(!existing_ctxt_fnd){ > > if (!existing_ctx_fnd) { > > [..] > >> @@ -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, Robbie VanVossen