[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 2/9] xen: arm: Implement variable levels in dump_pt_walk
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |