On 08/15/2012 10:54 AM, Jan Beulich wrote:
>>>> On 14.08.12 at 21:55, Santosh Jodh<santosh.jodh@citrix.com>  wrote:
>
> Sorry to be picky; after this many rounds I would have
> expected that no further comments would be needed.
>
>> +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);
>> +
>> +        address = gpa + amd_offset_level_address(index, level);
>> +        if ( next_level>= 1 )
>> +            amd_dump_p2m_table_level(
>> +                maddr_to_page(next_table_maddr), level - 1,
>
> Did you see Wei's cleanup patches to the code you cloned from?
> You should follow that route (replacing the ASSERT() with
> printing of the inconsistency and _not_ recursing or doing the
> normal printing), and using either "level" or "next_level"
> consistently here.

Hi, I tested the patch and the output looks much better now，please see 
attachment. One thing I notice: there is a 1GB mapping in the guest, but 
the format of it looks like other 2MB mappings:

(XEN)  gfn: 0003fa00  mfn: 0023b000
(XEN)  gfn: 0003fc00  mfn: 00136200
(XEN)  gfn: 0003fe00  mfn: 0023ae00
(XEN)  gfn: 00040000  mfn: 00040000 << 1GB here
(XEN)  gfn: 00080000  mfn: 0023ac00
(XEN)  gfn: 00080200  mfn: 00136000
(XEN)  gfn: 00080400  mfn: 0023aa00
(XEN)  gfn: 00080600  mfn: 00135e00
(XEN)  gfn: 00080800  mfn: 0023a800
(XEN)  gfn: 00080a00  mfn: 00135c00
(XEN)  gfn: 00080c00  mfn: 0023a600
(XEN)  gfn: 00080e00  mfn: 00135a00
(XEN)  gfn: 00081000  mfn: 0023a400
(XEN)  gfn: 00081200  mfn: 00135800
(XEN)  gfn: 00081400  mfn: 0023a200
(XEN)  gfn: 00081600  mfn: 00135600

Thanks,
Wei

>> +                address, indent + 1);
>> +        else
>> +            printk("%*s" "gfn: %08lx  mfn: %08lx\n",
>> +                   indent, " ",
>
>              printk("%*sgfn: %08lx  mfn: %08lx\n",
>                     indent, "",
>
> I can vaguely see the point in splitting the two strings in the
> first argument, but the extra space in the third argument is
> definitely wrong - it'll make level 1 and level 2 indistinguishable.
>
> I also don't see how you addressed Wei's reporting of this still
> not printing correctly. I may be overlooking something, but
> without you making clear in the description what you changed
> over the previous version that's also relatively easy to happen.
>
>> +static void vtd_dump_p2m_table_level(paddr_t pt_maddr, int level, paddr_t gpa,
>> +                                     int indent)
>> +{
>> +    paddr_t address;
>> +    int i;
>> +    struct dma_pte *pt_vaddr, *pte;
>> +    int next_level;
>> +
>> +    if ( level<  1 )
>> +        return;
>> +
>> +    pt_vaddr = map_vtd_domain_page(pt_maddr);
>> +    if ( pt_vaddr == NULL )
>> +    {
>> +        printk("Failed to map VT-D domain page %"PRIpaddr"\n", pt_maddr);
>> +        return;
>> +    }
>> +
>> +    next_level = level - 1;
>> +    for ( i = 0; i<  PTE_NUM; i++ )
>> +    {
>> +        if ( !(i % 2) )
>> +            process_pending_softirqs();
>> +
>> +        pte =&pt_vaddr[i];
>> +        if ( !dma_pte_present(*pte) )
>> +            continue;
>> +
>> +        address = gpa + offset_level_address(i, level);
>> +        if ( next_level>= 1 )
>> +            vtd_dump_p2m_table_level(dma_pte_addr(*pte), next_level,
>> +                                     address, indent + 1);
>> +        else
>> +            printk("%*s" "gfn: %08lx mfn: %08lx super=%d rd=%d wr=%d\n",
>> +                   indent, " ",
>
> Same comment as above.
>
>> +                   (unsigned long)(address>>  PAGE_SHIFT_4K),
>> +                   (unsigned long)(pte->val>>  PAGE_SHIFT_4K),
>> +                   dma_pte_superpage(*pte)? 1 : 0,
>> +                   dma_pte_read(*pte)? 1 : 0,
>> +                   dma_pte_write(*pte)? 1 : 0);
>
> Missing spaces. Even worse - given your definitions of these
> macros there's no point in using the conditional operators here
> at all.
>
> And, despite your claim in another response, this still isn't similar
> to AMD's variant (which still doesn't print any of these three
> attributes).
>
> The printing of the superpage status is pretty pointless anyway,
> given that there's no single use of dma_set_pte_superpage()
> throughout the tree - validly so since superpages can be in use
> currently only when the tables are shared with EPT, in which
> case you don't print anything. Plus you'd need to detect the flag
> _above_ level 1 (at leaf level the bit is ignored and hence just
> confusing if printed) and print the entry instead of recursing. And
> if you decide to indeed properly implement this (rather than just
> dropping superpage support here), _I_ would expect you to
> properly implement level skipping in the corresponding AMD code
> too (which similarly isn't being used currently).
>
> Jan
>