From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates Date: Thu, 1 Oct 2015 15:42:22 +0100 Message-ID: <560D464E.2010908@citrix.com> References: <560D261D02000078000A760A@prv-mh.provo.novell.com> <560D365A.90804@citrix.com> <560D611502000078000A7751@prv-mh.provo.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 1Zhf45-00058u-Us for xen-devel@lists.xenproject.org; Thu, 01 Oct 2015 14:42:34 +0000 In-Reply-To: <560D611502000078000A7751@prv-mh.provo.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: George Dunlap , xen-devel , Wei Liu List-Id: xen-devel@lists.xenproject.org On 01/10/15 15:36, Jan Beulich wrote: >>>> On 01.10.15 at 15:34, wrote: >> On 01/10/15 11:25, Jan Beulich wrote: >>> @@ -645,11 +665,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m, >>> && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) ) >>> p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1; >>> >>> - if ( iommu_enabled && need_iommu(p2m->domain) ) >>> + if ( iommu_enabled && need_iommu(p2m->domain) && >>> + (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) ) >>> { >>> if ( iommu_use_hap_pt(p2m->domain) ) >>> { >>> - if ( old_mfn && (old_mfn != mfn_x(mfn)) ) >>> + if ( iommu_old_flags ) >>> amd_iommu_flush_pages(p2m->domain, gfn, page_order); >> iommu_hap_pt_share is hardwired to 0 on AMD, making this if() clause >> effectively dead. Is this what you mean by your second TBD? I would >> suggest dropping it. > Yes, that's what I mean. > >>> } >>> else >>> >>> >> In this else clause there is a now-shadowed "flags" which might better >> be renamed to iommu_flags to avoid confusion. >> >> There is also an extra shadowed 'i' which could do with removing, as it >> introduces a 64bit->32bit truncation (which is not currently a problem). > See the (not re-submitted because it didn't really change) follow-up > patch > (http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg02585.html) > which deletes that pointless variable altogether. Ah - I appear to have missed that in my review queue - apologies. Consider it Reviewed-by: Andrew Cooper As for the logic presented in this patch, I believe it is correct, so also Reviewed-by: Andrew Cooper