From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 16/24] xen/passthrough: Introduce iommu_construct Date: Wed, 21 Jan 2015 14:22:44 +0000 Message-ID: <54BFB634.6030407@linaro.org> References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org> <1421159133-31526-17-git-send-email-julien.grall@linaro.org> <54BD46C60200007800056A6B@mail.emea.novell.com> <54BE6611.1070701@linaro.org> <54BE932502000078000572A8@mail.emea.novell.com> <54BE8C4A.7030801@linaro.org> <54BF8C470200007800057611@mail.emea.novell.com> <54BF815F.5090106@linaro.org> <54BF92110200007800057667@mail.emea.novell.com> <54BF9803.6090404@linaro.org> <54BFC22D0200007800057B41@mail.emea.novell.com> 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 1YDwBd-0000kA-RA for xen-devel@lists.xenproject.org; Wed, 21 Jan 2015 14:23:13 +0000 Received: by mail-we0-f181.google.com with SMTP id k48so2956130wev.12 for ; Wed, 21 Jan 2015 06:23:12 -0800 (PST) In-Reply-To: <54BFC22D0200007800057B41@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 21/01/15 14:13, Jan Beulich wrote: >>>> On 21.01.15 at 13:13, wrote: >> Hi Jan, >> >> On 21/01/15 10:48, Jan Beulich wrote: >>>>>> On 21.01.15 at 11:37, wrote: >>>> On 21/01/2015 10:23, Jan Beulich wrote: >>>>>>>> On 20.01.15 at 18:11, wrote: >>>>>> While this function is currently only used for DOM0, this will be used >>>>>> in a later patch for guest non-PCI passthrough. >>>>> >>>>> Okay, but you shouldn't break (or alter in [seemingly] benign ways) the >>>>> Dom0 case imo. >>>> >>>> As iommu_hwdom_init is initialized correctly the IOMMU for DOM0, >>>> iommu_construct is a no-op. >>>> >>>> Would an if ( need_iommu(d) ) will be more clear? Maybe we an assert >>>> (!is_hardware_domain(d)). >>> >>> Just think this through properly: iommu_hwdom_init() may leave >>> Dom0's ->need_iommu at 0 or 1 (depending on iommu_dom0_strict). >>> And iommu_construct() specifically is a nop only when ->need_iommu >>> is positive (x86's arch_iommu_populate_page_table() sets it to a >>> negative value to indicate "being set up", and I wonder how ARM >>> gets away without doing so). >> >> iommu_dom0_strict is always set to 1 when IOMMU is used on ARM (see >> check_hwdom_reqs). >> >> Futhermore, we always share the page table with the processor, so we >> never need to populate the page table. > > That's all far from obvious looking at the patch at hand. And you > can then only hope that these two "always" will always remain to > be that way. Hence the suggested ASSERT on a previous mail. I could also add a comment in the code explaining the ASSERT(). Regards, -- Julien Grall