Re: [PATCH] dump_p2m_table: For IOMMU

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.08.12 at 21:14, Santosh Jodh <santosh.jodh@citrix.com> wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c	Tue Aug 07 18:37:31 2012 +0100
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c	Fri Aug 10 08:19:58 2012 -0700
> @@ -512,6 +513,80 @@ static int amd_iommu_group_id(u16 seg, u
>  
>  #include <asm/io_apic.h>
>  
> +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_table_maddr != 0) && (next_level != 0) )

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

> +        {
> +            int i;
> +
> +            for ( i = 0; i < indent; i++ )
> +                printk("  ");

printk("%*s...", indent, "", ...);

> +
> +            printk("gfn: %08lx  mfn: %08lx\n",
> +                   (unsigned long)PFN_DOWN(address), 
> +                   (unsigned long)PFN_DOWN(next_table_maddr));
> +        }
> +    }
> +
> +    unmap_domain_page(table_vaddr);
> +}
>...
> --- a/xen/drivers/passthrough/vtd/iommu.c	Tue Aug 07 18:37:31 2012 +0100
> +++ b/xen/drivers/passthrough/vtd/iommu.c	Fri Aug 10 08:19:58 2012 -0700
> @@ -2365,6 +2366,71 @@ static void vtd_resume(void)
>      }
>  }
>  
> +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 ( pt_maddr == 0 )
> +        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
> +        {
> +            int j;
> +
> +            for ( j = 0; j < indent; j++ )
> +                printk("  ");

See above.

Jan

> +
> +            printk("gfn: %08lx mfn: %08lx super=%d rd=%d wr=%d\n",
> +                   (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);
> +        }
> +    }
> +
> +    unmap_vtd_domain_page(pt_vaddr);
> +}