Re: [Xen-devel] [PATCH] dump_p2m_table: For IOMMU

[Santosh Jodh <Santosh.Jodh@xxxxxxxxxx>]

Thanks for the detailed feedback. Please see inline:


> +    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 %016"PRIx64"\n", 
> + page_to_maddr(pg));

We specifically have PRIpaddr for this purpose.
[Santosh Jodh] Ah - one more format specifier :-). Will change it.

> +        }
> +
> +        if ( present )
> +        {
> +            printk("gfn: %016"PRIx64"  mfn: %016"PRIx64"\n",
> +                   address >> PAGE_SHIFT, next_table_maddr >> 
> + PAGE_SHIFT);

I'd prefer you to use PFN_DOWN() here.
[Santosh Jodh] ok.

Also, depth first, as requested by Tim, to me doesn't mean recursing before 
printing. I think you really want to print first, then recurse. Otherwise how 
would be output be made sense of?
[Santosh Jodh] it gives a simple gfp -> mfn map. With nested printing, you get 
the PD and PT addresses printed - which are not super useful. Anyway, I will 
change it.

>  static void __init parse_iommu_param(char *s)  {
>      char *ss;
> @@ -119,6 +122,7 @@ void __init iommu_dom0_init(struct domai
>      if ( !iommu_enabled )
>          return;
>  
> +    setup_iommu_dump();
>      d->need_iommu = !!iommu_dom0_strict;
>      if ( need_iommu(d) )
>      {
>...
> +void __init setup_iommu_dump(void)
> +{
> +    register_keyhandler('o', &iommu_p2m_table); }

Furthermore, there's no real need for a separate function here anyway. Just 
call register_key_handler() directly. Or alternatively this ought to match 
other code doing the same - using an initcall.
[Santosh Jodh] will just call register_key_handler. I had to rearrange code in 
the file to avoid forward declarations. So much for trying to contain my 
changes to the end of the file :-)

> --- a/xen/drivers/passthrough/vtd/iommu.c     Tue Aug 07 18:37:31 2012 +0100
> +++ b/xen/drivers/passthrough/vtd/iommu.c     Wed Aug 08 09:56:50 2012 -0700
> +static void vtd_dump_p2m_table_level(u64 pt_maddr, int level, u64 
> +gpa) {
> +    u64 address;

Again, both gpa and address ought to be paddr_t, and the format specifiers 
should match.
[Santosh Jodh] will do.

> +    int i;
> +    struct dma_pte *pt_vaddr, *pte;
> +    int next_level;
> +
> +    if ( pt_maddr == 0 )
> +        return;
> +
> +    pt_vaddr = (struct dma_pte *)map_vtd_domain_page(pt_maddr);

Pointless cast.
[Santosh Jodh] yep - gone.

> +    if ( pt_vaddr == NULL )
> +    {
> +        printk("Failed to map VT-D domain page %016"PRIx64"\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);
> +
> +        if ( level == 1 )
> +            printk("gfn: %016"PRIx64" mfn: %016"PRIx64" superpage=%d\n", 
> +                    address >> PAGE_SHIFT_4K, pte->val >> 
> + PAGE_SHIFT_4K, dma_pte_superpage(*pte)? 1 : 0);

Why do you print leaf (level 1) tables here only?
[Santosh Jodh] as I said above - I was dumping gfn -> mfn. I will make it 
indent and print all levels.

And the last line certainly is above 80 chars, so needs breaking up.
[Santosh Jodh] yep - done.

(Also, just to avoid you needing to do another iteration: Don't switch to 
PFN_DOWN() here.)
[Santosh Jodh] ok

I further wonder whether "superpage" alone is enough - don't we have both 2M 
and 1G pages? Of course, that would become mute if higher levels got also 
dumped (as then this knowledge is implicit).

Which reminds me to ask that both here and in the AMD code the recursion level 
should probably be reflected by indenting the printed strings.
[Santosh Jodh] will print indented and all levels.

Jan

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