Re: [Xen-devel] [PATCH] Dump IOMMU p2m table

[Santosh Jodh <Santosh.Jodh@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, August 15, 2012 1:54 AM
> To: Santosh Jodh
> Cc: wei.wang2@xxxxxxx; xiantao.zhang@xxxxxxxxx; xen-devel; Tim
> (Xen.org)
> Subject: Re: [Xen-devel] [PATCH] Dump IOMMU p2m table
> 
> >>> On 14.08.12 at 21:55, Santosh Jodh <santosh.jodh@xxxxxxxxxx> wrote:
> 
> Sorry to be picky; after this many rounds I would have expected that no
> further comments would be needed.

I started off trying to code to existing structure and style in individual 
files I was modifying. I also created IOMMU p2m dump - similar to MMU p2m dump. 
Over the last few days, this has evolved into cleaning up existing AMD code, 
indenting for more clarity etc. All good points btw.

> 
> > +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.

Ok - will do.

> 
> > +                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 misunderstood how "%*s" works. That probably explains the output Wei saw.

> 
> 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.


Will add more history.

> 
> > +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.

Yep - got it.

> 
> > +                   (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).

I meant structure in terms of recursion logic, level checks etc. I will just 
remove the extra prints to make it more similar. My original goal was to print 
it similar to the existing MMU p2m table.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel