From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH] dump_p2m_table: For IOMMU Date: Fri, 10 Aug 2012 15:24:45 +0100 Message-ID: <502535CD0200007800094325@nat28.tlf.novell.com> References: <8deb7c7a25c4a3bc50d7.1344446255@REDBLD-XS.ad.xensource.com> <5023822E0200007800093D2A@nat28.tlf.novell.com> <5024E788.80300@amd.com> <50252031020000780009427B@nat28.tlf.novell.com> <50250F9B.9090702@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50250F9B.9090702@amd.com> 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: Wei Wang Cc: tim@xen.org, xiantao.zhang@intel.com, Santosh Jodh , xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 10.08.12 at 15:41, Wei Wang wrote: > On 08/10/2012 02:52 PM, Jan Beulich wrote: >>>>> On 10.08.12 at 12:50, Wei Wang wrote: >>> On 08/09/2012 09:26 AM, Jan Beulich wrote: >>>> (And similar >>>> to the issue Santosh has already fixed here - the original >>>> function pointlessly maps/unmaps the page when "level<= 1". >>>> Furthermore, iommu_map.c has nice helper functions >>>> iommu_next_level() and amd_iommu_is_pte_present() - why >>>> aren't they in a header instead, so they could be used here, >>>> avoiding the open coding of them?) >>> >>> Maybe those helps appears after the original function. I could sent a >>> patch to clean up these: >>> * do not map/unmap if level<= 1 >>> * move amd_iommu_is_pte_present() and iommu_next_level() to a header >>> file. and use them in deallocate_next_page_table. >>> * Using next_level instead of recalculation (if requested) >> >> Yes, please. As to using next_level - it depends, besides the above, >> on how bad it is if this is really wrong; an ASSERT() or BUG_ON() >> might be on order here. > > How about ASSERT(next_level < = (level -1) )? But that would again mean levels can be skipped. If they can, then using next_level is the way to go, and the assertion is fine. If they can't, the assertion should say ==. Jan