From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen/passthrough: Support a single iommu_domain(context bank) per xen domain per SMMU Date: Fri, 20 Mar 2015 15:29:40 +0000 Message-ID: <550C3CE4.2040003@linaro.org> References: <1426789514-5892-1-git-send-email-robert.vanvossen@dornerworks.com> <550C14EF.8000002@linaro.org> <550C2710.5080107@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 1YYyrp-0001G4-7P for xen-devel@lists.xenproject.org; Fri, 20 Mar 2015 15:29:45 +0000 Received: by wibg7 with SMTP id g7so147506053wib.1 for ; Fri, 20 Mar 2015 08:29:43 -0700 (PDT) In-Reply-To: <550C2710.5080107@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: Robert 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 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