From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC 11/19] xen/passthrough: Call arch_iommu_domain_destroy before calling iommu_teardown Date: Tue, 17 Jun 2014 10:18:31 +0100 Message-ID: <53A007E7.4050407@linaro.org> References: <1402935486-29136-1-git-send-email-julien.grall@linaro.org> <1402935486-29136-12-git-send-email-julien.grall@linaro.org> <53A01376020000780001AE02@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WwpXI-0002pU-9j for xen-devel@lists.xenproject.org; Tue, 17 Jun 2014 09:18:36 +0000 Received: by mail-wi0-f170.google.com with SMTP id cc10so6538365wib.1 for ; Tue, 17 Jun 2014 02:18:33 -0700 (PDT) In-Reply-To: <53A01376020000780001AE02@mail.emea.novell.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: Jan Beulich Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org Hi Ian, On 17/06/14 09:07, Jan Beulich wrote: >>>> On 16.06.14 at 18:17, wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >> if ( !iommu_enabled || !hd->platform_ops ) >> return; >> >> + arch_iommu_domain_destroy(d); >> + >> if ( need_iommu(d) ) >> iommu_teardown(d); >> - >> - arch_iommu_domain_destroy(d); > > At the first glance this doesn't look right, including the explanation > you gave (why would devices still be assigned to a guest at this > point). Because the toolstack may forget to deassign a device. How do you handle this case in x86? In the SMMU case, this will mean a memory leak and misconfiguration of the registers. I think it's safer to let Xen deassign the remaining devices. > And it's rather hard to properly decide with the series here > depending on two other series, i.e. there not being a > arch_iommu_domain_destroy() at all in current staging. Are you sure? The other series doesn't deal with the IOMMU stuff. This change has been pushed upstream a month ago (see commit 4905b35c " iommu: introduce arch specific code"). Regards, -- Julien Grall