Re: [PATCH RFC 2/9] xen: arm: Implement variable levels in dump_pt_walk

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 07/30/2014 02:47 PM, Ian Campbell wrote:
> ---
>  xen/arch/arm/mm.c          |   62 ++++++++++++++++++++++++++++----------------
>  xen/arch/arm/p2m.c         |    4 ++-
>  xen/include/asm-arm/page.h |    2 +-
>  3 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0a243b0..fa6a729 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -84,10 +84,12 @@ lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
>   */
>  
>  #ifdef CONFIG_ARM_64
> +#define HYP_PT_ROOT_LEVEL 0
>  lpae_t xen_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
>  lpae_t xen_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
>  #define THIS_CPU_PGTABLE xen_pgtable
>  #else
> +#define HYP_PT_ROOT_LEVEL 1
>  /* Per-CPU pagetable pages */
>  /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */
>  static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
> @@ -165,34 +167,50 @@ static inline void check_memory_layout_alignment_constraints(void) {
>  #endif
>  }
>  
> -void dump_pt_walk(lpae_t *first, paddr_t addr)
> +void dump_pt_walk(lpae_t *root, paddr_t addr, int root_level)
>  {
> -    lpae_t *second = NULL, *third = NULL;
> +    static const char *level_strs[4] = { "0TH", "1ST", "2ND", "3RD" };
> +    const unsigned int offsets[4] = {
> +        zeroeth_table_offset(addr),
> +        first_table_offset(addr),
> +        second_table_offset(addr),
> +        third_table_offset(addr)
> +    };
> +    lpae_t pte, *mappings[4] = { 0, };
> +    int level;
>  
> -    if ( first_table_offset(addr) >= LPAE_ENTRIES )
> -        return;
> +#ifdef CONFIG_ARM_32
> +    BUG_ON(root_level < 1);
> +#endif
> +
> +    mappings[root_level] = root;
> +
> +    for ( level = root_level; level < 4; level++ )
> +    {
> +        if ( offsets[level] > LPAE_ENTRIES )
> +            break;
>  
> -    printk("1ST[0x%x] = 0x%"PRIpaddr"\n", first_table_offset(addr),
> -           first[first_table_offset(addr)].bits);
> -    if ( !first[first_table_offset(addr)].walk.valid ||
> -         !first[first_table_offset(addr)].walk.table )
> -        goto done;
> +        if ( !mappings[level] )
> +        {
> +            printk("%s: Failed to map PT page\n", level_strs[level]);
> +            break;
> +        }

map_domain_page can't fail. So this test is not necessary.

>  
> -    second = map_domain_page(first[first_table_offset(addr)].walk.base);
> -    printk("2ND[0x%x] = 0x%"PRIpaddr"\n", second_table_offset(addr),
> -           second[second_table_offset(addr)].bits);
> -    if ( !second[second_table_offset(addr)].walk.valid ||
> -         !second[second_table_offset(addr)].walk.table )
> -        goto done;
> +        pte = mappings[level][offsets[level]];
>  
> -    third = map_domain_page(second[second_table_offset(addr)].walk.base);
> -    printk("3RD[0x%x] = 0x%"PRIpaddr"\n", third_table_offset(addr),
> -           third[third_table_offset(addr)].bits);
> +        printk("%s[0x%x] = 0x%"PRIpaddr"\n",
> +               level_strs[level], offsets[level], pte.bits);
> +        if ( !pte.walk.valid || !pte.walk.table )
> +            break;
>  
> -done:
> -    if (third) unmap_domain_page(third);
> -    if (second) unmap_domain_page(second);
> +        mappings[level+1] = map_domain_page(pte.walk.base);
> +    }

On 3rd level, pte.walk.table is always equal to 1. So for valid mapping,
you are mapping one more page than necessary.

> +    /* mappings[root_level] is provided by the caller */
> +    for ( level = root_level + 1 ; level < 4; level++ )
> +    {
> +        unmap_domain_page(mappings[level]);


unmap_domain_page doesn't handle NULL pointer. So if you loop has exited
before the last mapping, Xen may hangs.

> +    }
>  }
>  
>  void dump_hyp_walk(vaddr_t addr)
> @@ -208,7 +226,7 @@ void dump_hyp_walk(vaddr_t addr)
>          BUG_ON( (lpae_t *)(unsigned long)(ttbr - phys_offset) != pgtable );
>      else
>          BUG_ON( virt_to_maddr(pgtable) != ttbr );
> -    dump_pt_walk(pgtable, addr);
> +    dump_pt_walk(pgtable, addr, HYP_PT_ROOT_LEVEL);
>  }
>  
>  /* Map a 4k page in a fixmap entry */
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 61958ba..64efdce 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -11,6 +11,8 @@
>  #include <asm/hardirq.h>
>  #include <asm/page.h>
>  
> +#define P2M_ROOT_LEVEL       1
> +
>  /* First level P2M is 2 consecutive pages */
>  #define P2M_ROOT_ORDER 1
>  #define P2M_ROOT_ENTRIES (LPAE_ENTRIES<<P2M_ROOT_ORDER)
> @@ -64,7 +66,7 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
>             p2m->root, page_to_mfn(p2m->root));
>  
>      first = __map_domain_page(p2m->root);
> -    dump_pt_walk(first, addr);
> +    dump_pt_walk(first, addr, P2M_ROOT_LEVEL);

I've just noticed that we forgot to take the p2m lock here. So the P2M
can be modified under our feet. I'm not sure what are the implications
with the dump_pt_walk.

Regards,

-- 
Julien Grall