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 13:38:18 +0100 Message-ID: <53A036BA.9090905@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> <53A007E7.4050407@linaro.org> <53A02685020000780001AEBF@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 1Wwsec-0005Lr-37 for xen-devel@lists.xenproject.org; Tue, 17 Jun 2014 12:38:22 +0000 Received: by mail-wg0-f50.google.com with SMTP id x13so6928048wgg.21 for ; Tue, 17 Jun 2014 05:38:20 -0700 (PDT) In-Reply-To: <53A02685020000780001AEBF@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 On 06/17/2014 10:29 AM, Jan Beulich wrote: >>>> On 17.06.14 at 11:18, wrote: >> 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. > > Proper tool stack behavior is required (and not just here). I think this is important to handle toolstack failure (such as crash) just in case. Hence it doesn't add much code for this purpose. >>> 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"). > > Oops, indeed - I'm sorry, I looked at a stale branch. Looking at the > correct code I still think the current order is the correct one, and if > you need to take extra steps you ought to do so from the .teardown > hook. I though about implement it in .teardown, but it results to non-obvious code. I could call iommu_dt_domain_destroy in .teardown, that will mean to call "arch dt" code in the SMMU drivers which I think break the design. I would prefer call it the arch specific function. Do you mind if I add a new function called arch_iommu_reassign_devices? This function will reassign every devices of a given domain to the hardware domain. The iommmu_domain_destroy will look like: void iommu_domain_destroy(struct domain *d) { if ( !iommu_enabled ) return; arch_iommu_reassign_devices(d); if ( need_iommu(d) ) iommu_teardown(d); arch_iommu_domain_destroy(d); } Regards, -- Julien Grall