From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Wang Subject: Re: [PATCH] dump_p2m_table: For IOMMU Date: Fri, 10 Aug 2012 15:41:47 +0200 Message-ID: <50250F9B.9090702@amd.com> References: <8deb7c7a25c4a3bc50d7.1344446255@REDBLD-XS.ad.xensource.com> <5023822E0200007800093D2A@nat28.tlf.novell.com> <5024E788.80300@amd.com> <50252031020000780009427B@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50252031020000780009427B@nat28.tlf.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: tim@xen.org, xiantao.zhang@intel.com, Santosh Jodh , xen-devel List-Id: xen-devel@lists.xenproject.org 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: >> >>> Wei - here I'm particularly worried about the use of "level - 1" >>> instead of "next_level", which would similarly apply to the >>> original function. If the way this is currently done is okay, then >>> why is next_level being computed in the first place? >> >> I think that recalculation is to guarantee that this recursive function >> returns. It should run at most "paging_mode" times no matter what >> "next_level" says. But if we could assume that next level field in every >> pde is correct, then using next level is fine to me. > > Especially in the dumping function we shouldn't assume too > much. However, wasn't it that one can skip levels in your > IOMMU implementation? That can't be handled correctly if > always subtracting 1. We have no skip levels yet. But since it checks (next_level != 0) before calling itself, it should not deallocate pages unexpectedly. But it will also waste some time in the loop. if next_level == 0 but level > 1 (e.g. we have only l4, l2, l1 tables). So, yes, now using next_level with ASSERT also looks better to me. > >>> (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) )? > Jan > >