From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Wang Subject: Re: [PATCH] Dump IOMMU p2m table Date: Fri, 17 Aug 2012 16:43:34 +0200 Message-ID: <502E5896.30606@amd.com> References: <575a53faf4e1f3533096.1345134973@REDBLD-XS.ad.xensource.com> <502E27B0.7030803@amd.com> <7914B38A4445B34AA16EB9F1352942F1012F0E39A007@SJCPMAILBOX01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <7914B38A4445B34AA16EB9F1352942F1012F0E39A007@SJCPMAILBOX01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Santosh Jodh Cc: "xen-devel@lists.xensource.com" , "Tim (Xen.org)" , "JBeulich@suse.com" , "xiantao.zhang@intel.com" List-Id: xen-devel@lists.xenproject.org On 08/17/2012 04:31 PM, Santosh Jodh wrote: > > >> -----Original Message----- >> From: Wei Wang [mailto:wei.wang2@amd.com] >> Sent: Friday, August 17, 2012 4:15 AM >> To: Santosh Jodh >> Cc: xen-devel@lists.xensource.com; xiantao.zhang@intel.com; Tim >> (Xen.org); JBeulich@suse.com >> Subject: Re: [PATCH] Dump IOMMU p2m table >> >> On 08/16/2012 06:36 PM, Santosh Jodh wrote: >>> New key handler 'o' to dump the IOMMU p2m table for each domain. >>> Skips dumping table for domain0. >>> Intel and AMD specific iommu_ops handler for dumping p2m table. >>> >>> Incorporated feedback from Jan Beulich and Wei Wang. >>> Fixed indent printing with %*s. >>> Removed superflous superpage and other attribute prints. >>> Make next_level use consistent for AMD IOMMU dumps. Warn if found >> inconsistent. >>> >>> Signed-off-by: Santosh Jodh >>> >>> diff -r 6d56e31fe1e1 -r 575a53faf4e1 >> xen/drivers/passthrough/amd/pci_amd_iommu.c >>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c Wed Aug 15 >> 09:41:21 2012 +0100 >>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c Thu Aug 16 >> 09:28:24 2012 -0700 >>> @@ -22,6 +22,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -512,6 +513,80 @@ static int amd_iommu_group_id(u16 seg, u >>> >>> #include >>> >>> +static void amd_dump_p2m_table_level(struct page_info* pg, int >> level, >>> + paddr_t gpa, int indent) { >>> + paddr_t address; >>> + void *table_vaddr, *pde; >>> + paddr_t next_table_maddr; >>> + int index, next_level, present; >>> + u32 *entry; >>> + >>> + if ( level< 1 ) >>> + return; >>> + >>> + table_vaddr = __map_domain_page(pg); >>> + if ( table_vaddr == NULL ) >>> + { >>> + printk("Failed to map IOMMU domain page %"PRIpaddr"\n", >>> + page_to_maddr(pg)); >>> + return; >>> + } >>> + >>> + for ( index = 0; index< PTE_PER_TABLE_SIZE; index++ ) >>> + { >>> + if ( !(index % 2) ) >>> + process_pending_softirqs(); >>> + >>> + pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE); >>> + next_table_maddr = amd_iommu_get_next_table_from_pte(pde); >>> + entry = (u32*)pde; >>> + >>> + present = get_field_from_reg_u32(entry[0], >>> + IOMMU_PDE_PRESENT_MASK, >>> + IOMMU_PDE_PRESENT_SHIFT); >>> + >>> + if ( !present ) >>> + continue; >>> + >>> + next_level = get_field_from_reg_u32(entry[0], >>> + >> IOMMU_PDE_NEXT_LEVEL_MASK, >>> + >>> + IOMMU_PDE_NEXT_LEVEL_SHIFT); >>> + >>> + if ( next_level != (level - 1) ) >>> + { >>> + printk("IOMMU p2m table error. next_level = %d, expected >> %d\n", >>> + next_level, level - 1); >>> + >>> + continue; >>> + } >> >> HI, >> >> This check is not proper for 2MB and 1GB pages. For example, if a guest >> 4 level page tables, for a 2MB entry, the next_level field will be 3 >> ->(l4)->2(l3)->0(l2), because l2 entries becomes PTEs and PTE entries >> have next_level = 0. I saw following output for those pages: >> >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> (XEN) IOMMU p2m table error. next_level = 0, expected 1 >> >> Thanks, >> Wei > > How about changing the check to: > if ( next_level&& (next_level != (level - 1)) ) That should be good, since we don't have skip levels. Thanks, Wei > > Thanks, > Santosh >