diff mbox

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

Message ID b808d8e29fafb8cb80ed3563cc749b575832db53.1409847257.git.ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell Sept. 4, 2014, 4:14 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>
---
v2:
 - map_domain_page cannot fail, so don't check
 - don't map an extra page for a valid L3 entry
 - avoid unmapping levels which we didn't map.
 - root_level argument is unsigned int.
 - fold in spurious whitespace change from next patch
---
 xen/arch/arm/mm.c          |   60 ++++++++++++++++++++++++++++----------------
 xen/arch/arm/p2m.c         |    4 ++-
 xen/include/asm-arm/page.h |    2 +-
 3 files changed, 42 insertions(+), 24 deletions(-)

Comments

Julien Grall Sept. 8, 2014, 9:24 p.m. UTC | #1 
Hi Ian,

On 04/09/14 09:14, Ian Campbell wrote:
> -void dump_pt_walk(lpae_t *first, paddr_t addr)
> +void dump_pt_walk(lpae_t *root, paddr_t addr,
> +                  unsigned 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, };

Xen never maps the next level when level == 3, shoudn't mappings[3] enough?

[..]

> +    /* mappings[root_level] is provided by the caller so don't unmap that */
> +    do
> +    {
> +        unmap_domain_page(mappings[level]);
> +    }
> +    while( level-- > root_level );

NIT: It looks like we commonly use the coding style

do
{
} while ( ... )

Regards,
Julien Grall Sept. 8, 2014, 9:33 p.m. UTC | #2 
Hi Ian,

I forgot one point on my previous mail.

On 04/09/14 09:14, Ian Campbell wrote:
> -    if ( first_table_offset(addr) >= LPAE_ENTRIES )
> -        return;
> +    mappings[root_level] = root;
> +
> +    for ( level = root_level; ; level++ )
> +    {

I would add the check "level < 3" or an ASSERT in the beginning of the 
loop. So we could catch a possible overflow on offsets later if someone 
decide to modify it.

Regards,
Julien Grall Sept. 8, 2014, 10:12 p.m. UTC | #3 
On 08/09/14 14:24, Julien Grall wrote:
> Hi Ian,
>
> On 04/09/14 09:14, Ian Campbell wrote:
>> -void dump_pt_walk(lpae_t *first, paddr_t addr)
>> +void dump_pt_walk(lpae_t *root, paddr_t addr,
>> +                  unsigned 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, };
>
> Xen never maps the next level when level == 3, shoudn't mappings[3] enough?

After thinking, mappings[4] is fine here. Sorry for the noise.

Regards,
Ian Campbell Sept. 9, 2014, 3:04 p.m. UTC | #4 
On Mon, 2014-09-08 at 14:24 -0700, Julien Grall wrote:
> > +    /* mappings[root_level] is provided by the caller so don't unmap that */
> > +    do
> > +    {
> > +        unmap_domain_page(mappings[level]);
> > +    }
> > +    while( level-- > root_level );
> 
> NIT: It looks like we commonly use the coding style
> 
> do
> {
> } while ( ... )

Seems we do. Seems inconsistent with if() and while/do loops to me. Oh
well.

Ian.
Ian Campbell Sept. 9, 2014, 3:08 p.m. UTC | #5 
On Tue, 2014-09-09 at 16:04 +0100, Ian Campbell wrote:
> On Mon, 2014-09-08 at 14:24 -0700, Julien Grall wrote:
> > > +    /* mappings[root_level] is provided by the caller so don't unmap that */
> > > +    do
> > > +    {
> > > +        unmap_domain_page(mappings[level]);
> > > +    }
> > > +    while( level-- > root_level );
> > 
> > NIT: It looks like we commonly use the coding style
> > 
> > do
> > {
> > } while ( ... )
> 
> Seems we do. Seems inconsistent with if() and while/do loops to me. Oh
> well.

Actually now I go to look for the line in question it seems like we
actually have a pretty arbitrary mixture. I'm going to stick with the
extra newline (I did fix the missing space after the while though).

Ian.
Andrew Cooper Sept. 9, 2014, 4:04 p.m. UTC | #6 
On 09/09/14 16:04, Ian Campbell wrote:
> On Mon, 2014-09-08 at 14:24 -0700, Julien Grall wrote:
>>> +    /* mappings[root_level] is provided by the caller so don't unmap that */
>>> +    do
>>> +    {
>>> +        unmap_domain_page(mappings[level]);
>>> +    }
>>> +    while( level-- > root_level );
>> NIT: It looks like we commonly use the coding style
>>
>> do
>> {
>> } while ( ... )
> Seems we do. Seems inconsistent with if() and while/do loops to me. Oh
> well.
>
> Ian.

It is to visually differentiate

if ( ... )
{
    ....
}
while ( something unrelated, with a suspicious looking semicolon );

from
do
{
    ....
}
while ( something related to the loop );

~Andrew
diff mbox

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0a243b0..ab91275 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,48 @@  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,
+                  unsigned 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, };
+    unsigned int level;
+
+    BUG_ON(!root);
+#ifdef CONFIG_ARM_32
+    BUG_ON(root_level < 1);
+#endif
 
-    if ( first_table_offset(addr) >= LPAE_ENTRIES )
-        return;
+    mappings[root_level] = root;
+
+    for ( level = root_level; ; 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;
+        pte = mappings[level][offsets[level]];
 
-    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;
+        printk("%s[0x%x] = 0x%"PRIpaddr"\n",
+               level_strs[level], offsets[level], pte.bits);
 
-    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);
+        if ( level == 3 || !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 so don't unmap that */
+    do
+    {
+        unmap_domain_page(mappings[level]);
+    }
+    while( level-- > root_level );
 }
 
 void dump_hyp_walk(vaddr_t addr)
@@ -208,7 +224,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 ec3b451..6a19e37 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)
@@ -68,7 +70,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..4c21863 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, unsigned int root_level);
 
 /* Print a walk of the hypervisor's page tables for a virtual addr. */
 extern void dump_hyp_walk(vaddr_t addr);