From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests Date: Tue, 10 Mar 2015 13:35:26 +0000 Message-ID: <54FF012E02000078000681AF@mail.emea.novell.com> References: <1425036812-23536-1-git-send-email-roger.pau@citrix.com> <54FEE8BA.8060505@linaro.org> <54FEFA5102000078000680F7@mail.emea.novell.com> <54FEEF33.7010201@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YVKJt-0002AJ-Ml for xen-devel@lists.xenproject.org; Tue, 10 Mar 2015 13:35:37 +0000 In-Reply-To: <54FEEF33.7010201@linaro.org> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Roger Pau Monne , Julien Grall Cc: Yang Zhang , xen-devel@lists.xenproject.org, Kevin Tian , Aravind Gopalakrishnan , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org >>> On 10.03.15 at 14:18, wrote: > On 10/03/15 13:06, Jan Beulich wrote: >>>>> On 10.03.15 at 13:51, wrote: >>> Hi, >>> >>> On 27/02/15 11:33, Roger Pau Monne wrote: >>>> diff --git a/xen/drivers/passthrough/iommu.c >>> b/xen/drivers/passthrough/iommu.c >>>> index cc12735..7fcbbb1 100644 >>>> --- a/xen/drivers/passthrough/iommu.c >>>> +++ b/xen/drivers/passthrough/iommu.c >>>> @@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d) >>>> { >>>> const struct iommu_ops *ops = iommu_get_ops(); >>>> >>>> - if ( iommu_enabled && is_hvm_domain(d) ) >>>> + ASSERT( hap_enabled(d) ); >>> >>> This line is breaking compilation on ARM. >>> >>> Shouldn't it be replaced by iommu_use_hap_pt? >> >> No, that's a different thing. But shouldn't ARM have a stub >> hap_enabled() evaluating to constant true? > > I'm not sure if we should introduce hap_enabled. It's not something that > we should used in general. > > What are we trying to catch with this ASSERT? I guess wrong caller? > > If so, every share_p2m callbacks have a check "if iommu_use_hap_pt()" > which contains a check to hap_enabled on x86. Ah, right, I mixed this up with iommu_hap_pt_share. Roger - looks like this could indeed be replaced by if ( iommu_enabled && iommu_use_hap_pt(d) ) ops->share_p2m(d); and the corresponding check in VT-d and AMD Vi code could then also be dropped. Jan