Re: [PATCH RFC 3/9] xen: arm: handle concatenated root tables in dump_pt_walk

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

Hi Ian,

On 07/30/2014 02:47 PM, Ian Campbell wrote:
> ARM allows for the concatenation of pages at the root of a p2m (but not a
> regular page table) in order to support a larger IPA space than the number of
> levels in the P2M would normally support. We use this to support 40-bit guest
> addresses.
> 
> Previously we were unable to dump IPAs which were outside the first page of the
> root. To fix this we adjust dump_pt_walk to take the machine address of the
> page table root instead of expecting the caller to have mapper it. This allows
> the walker code to select the correct page to map.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/mm.c          |   45 +++++++++++++++++++++++++++++++-------------
>  xen/arch/arm/p2m.c         |   13 +++----------
>  xen/include/asm-arm/page.h |   15 +++++++++++++--
>  3 files changed, 48 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fa6a729..4ff783a 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -167,50 +167,69 @@ static inline void check_memory_layout_alignment_constraints(void) {
>  #endif
>  }
>  
> -void dump_pt_walk(lpae_t *root, paddr_t addr, int root_level)
> +void dump_pt_walk(paddr_t ttbr, paddr_t addr, int root_level, int nr_root_tables)

I should have said that yon the previous patch...

Both root_level and nr_root_tables can't be negative. I would use
unsigned int here.

>  {
>      static const char *level_strs[4] = { "0TH", "1ST", "2ND", "3RD" };
> +    const unsigned long root_pfn = paddr_to_pfn(ttbr);
>      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;
> +    lpae_t pte, *mapping;
> +    int level, root_table;

unsigned int here too.

>  #ifdef CONFIG_ARM_32
>      BUG_ON(root_level < 1);
>  #endif
>  
> -    mappings[root_level] = root;
> +    if ( nr_root_tables > 1 )
> +    {
> +        /*
> +         * Concatenated root-level tables. The table number will be
> +         * the offset at the previous level. It is not possible to
> +         * concetenate a level-0 root.

concatenate

> +         */
> +        BUG_ON(root_level == 0);
> +        root_table = offsets[root_level - 1];
> +        printk("Using concatenated root table %d\n", root_table);
> +        if ( root_table >= nr_root_tables )
> +        {
> +            printk("Invalid root table offset\n");
> +            return;
> +        }
> +    }
> +    else
> +        root_table = 0;
> +
> +    mapping = map_domain_page(root_pfn + root_table);
>  
>      for ( level = root_level; level < 4; level++ )
>      {
>          if ( offsets[level] > LPAE_ENTRIES )
>              break;
>  
> -        if ( !mappings[level] )
> +        if ( !mapping )
>          {
>              printk("%s: Failed to map PT page\n", level_strs[level]);
>              break;
>          }
>  
> -        pte = mappings[level][offsets[level]];
> +        pte = mapping[offsets[level]];
>  
>          printk("%s[0x%x] = 0x%"PRIpaddr"\n",
>                 level_strs[level], offsets[level], pte.bits);
> +

Spurious change, maybe it belong to the previous patch?

[..]


>  /* Map a 4k page in a fixmap entry */
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 64efdce..6839acf 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -16,6 +16,7 @@
>  /* First level P2M is 2 consecutive pages */
>  #define P2M_ROOT_ORDER 1
>  #define P2M_ROOT_ENTRIES (LPAE_ENTRIES<<P2M_ROOT_ORDER)
> +#define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>  
>  static bool_t p2m_valid(lpae_t pte)
>  {
> @@ -52,22 +53,14 @@ void p2m_dump_info(struct domain *d)
>  void dump_p2m_lookup(struct domain *d, paddr_t addr)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> -    lpae_t *first;
>  
>      printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
>  
> -    if ( first_linear_offset(addr) > LPAE_ENTRIES )
> -    {
> -        printk("Cannot dump addresses in second of first level pages...\n");
> -        return;
> -    }
> -
>      printk("P2M @ %p mfn:0x%lx\n",
>             p2m->root, page_to_mfn(p2m->root));
>  
> -    first = __map_domain_page(p2m->root);
> -    dump_pt_walk(first, addr, P2M_ROOT_LEVEL);
> -    unmap_domain_page(first);
> +    dump_pt_walk(page_to_maddr(p2m->root), addr,

You can directly use p2m->vttbr and avoid to call page_to_maddr.

Regards,

-- 
Julien Grall