diff mbox

[Xen-devel,RFC,2/9] xen: arm: Implement variable levels in dump_pt_walk

Message ID 265a2af15667d6fbf30a88ec7e5adebb818687fc.1406728037.git.ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell July 30, 2014, 1:47 p.m. UTC 
This allows us to correctly dump 64-bit hypervisor addresses, which use a 4
level table.

It also paves the way for boot-time selection of the number of levels to use in
the p2m, which is required to support both 40-bit and 48-bit systems.

To support multiple levels it is convenient to recast the page table walk as a
loop over the levels instead of the current open coding.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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(-)

Comments

Julien Grall July 30, 2014, 4:40 p.m. UTC | #1 
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,
diff mbox

Patch

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;
+        }
 
-    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);
+    }
 
+    /* mappings[root_level] is provided by the caller */
+    for ( level = root_level + 1 ; level < 4; level++ )
+    {
+        unmap_domain_page(mappings[level]);
+    }
 }
 
 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);
     unmap_domain_page(first);
 }
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 739038a..d1f8d52 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -352,7 +352,7 @@  static inline void flush_xen_data_tlb_range_va(unsigned long va,
 void flush_page_to_ram(unsigned long mfn);
 
 /* Print a walk of an arbitrary page table */
-void dump_pt_walk(lpae_t *table, paddr_t addr);
+void dump_pt_walk(lpae_t *table, paddr_t addr, int root_level);
 
 /* Print a walk of the hypervisor's page tables for a virtual addr. */
 extern void dump_hyp_walk(vaddr_t addr);