From mboxrd@z Thu Jan 1 00:00:00 1970 From: Santosh Jodh Subject: Re: [PATCH] dump_p2m_table: For IOMMU Date: Tue, 14 Aug 2012 12:34:41 -0700 Message-ID: <7914B38A4445B34AA16EB9F1352942F1012F0E1E9F41@SJCPMAILBOX01.citrite.net> References: <9c7609a4fbc117b1600f.1344626094@REDBLD-XS.ad.xensource.com> <5028DE020200007800094662@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5028DE020200007800094662@nat28.tlf.novell.com> Content-Language: en-US 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: "wei.wang2@amd.com" , "Tim (Xen.org)" , "xiantao.zhang@intel.com" , xen-devel List-Id: xen-devel@lists.xenproject.org Why do you do this differently than for VT-d here? There you don't check next_table_maddr (and I see no reason you would need to). Oh, I see, there's a similar check in a different place there. But this needs to be functionally similar here then. Specifically, ... > + { > + amd_dump_p2m_table_level( > + maddr_to_page(next_table_maddr), level - 1, > + address, indent + 1); > + } > + else ... you'd get into the else's body if next_table_maddr was zero, which is wrong afaict. So I think flow like if ( next_level ) print else if ( next_table_maddr ) recurse would be the preferable way to go if you feel that these zero checks are necessary (and if you do then, because this being the case is really a bug, this shouldn't go through silently). [Santosh Jodh] I was basing my code on existing code in the individual files. I was just being paranoid as this is debug code and I would not want to crash the system. Anyway, I am resending a patch that structures the code in the same way for both Intel and AMD. > + { > + int i; > + > + for ( i = 0; i < indent; i++ ) > + printk(" "); printk("%*s...", indent, "", ...); [Santosh Jodh] Cool - got it.