Re: [Xen-devel] [PATCH 21/45] xen: arm64: changes to setup_pagetables and mm.c

[Tim Deegan <tim@xxxxxxx>]

At 15:56 +0000 on 23 Jan (1358956587), Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> index eb5213e..e870ef6 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -40,13 +40,17 @@
>  struct domain *dom_xen, *dom_io, *dom_cow;
>  
>  /* Static start-of-day pagetables that we use before the allocators
> are up */
> +/* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 
> 32-bit) */

Is 0th ARM's numbering scheme or ours?  I don't object to it, but if
we're diverging from their numbering we should say so somewhere
(e.g. in the comment in asm-arm/page.h:75)

>  lpae_t xen_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
> +#ifdef CONFIG_ARM_64
> +lpae_t xen_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
> +#endif
>  lpae_t xen_second[LPAE_ENTRIES*4] __attribute__((__aligned__(4096*4)));
>  lpae_t xen_fixmap[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
>  static lpae_t xen_xenmap[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
>  
>  /* Non-boot CPUs use this to find the correct pagetables. */
> -uint64_t boot_httbr;
> +uint64_t boot_ttbr;
>  
>  static paddr_t phys_offset;
>  
> @@ -69,24 +73,24 @@ void dump_pt_walk(lpae_t *first, paddr_t addr)
>      if ( first_table_offset(addr) >= LPAE_ENTRIES )
>          return;
>  
> -    printk("1ST[0x%llx] = 0x%"PRIpaddr"\n",
> -           first_table_offset(addr),
> +    printk("1ST[0x%lx] = 0x%"PRIpaddr"\n",
> +           (unsigned long)first_table_offset(addr),

Eep!  Please either cast to paddr_t or stop using PRIpaddr (likewise
below).  I suppose it might be useful to have the nth_*_offset() macros
explicitly cast to some suitable small integer type, instead.

>             first[first_table_offset(addr)].bits);
>      if ( !first[first_table_offset(addr)].walk.valid ||
>           !first[first_table_offset(addr)].walk.table )
>          goto done;
>  
>      second = map_domain_page(first[first_table_offset(addr)].walk.base);
> -    printk("2ND[0x%llx] = 0x%"PRIpaddr"\n",
> -           second_table_offset(addr),
> +    printk("2ND[0x%lx] = 0x%"PRIpaddr"\n",
> +           (unsigned long)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;
>  
>      third = map_domain_page(second[second_table_offset(addr)].walk.base);
> -    printk("3RD[0x%llx] = 0x%"PRIpaddr"\n",
> -           third_table_offset(addr),
> +    printk("3RD[0x%lx] = 0x%"PRIpaddr"\n",
> +           (unsigned long)third_table_offset(addr),
>             third[third_table_offset(addr)].bits);
>  
>  done:
> @@ -95,14 +99,14 @@ done:
>  
>  }
>  
> -void dump_hyp_walk(uint32_t addr)
> +void dump_hyp_walk(vaddr_t addr)
>  {
> -    uint64_t httbr = READ_CP64(HTTBR);
> +    uint64_t ttbr = READ_SYSREG64(TTBR0_EL2);
>  
> -    printk("Walking Hypervisor VA 0x%08"PRIx32" via HTTBR 0x%016"PRIx64"\n",
> -           addr, httbr);
> +    printk("Walking Hypervisor VA 0x%"PRIvaddr" via TTBR0_EL2 
> 0x%016"PRIx64"\n",
> +           addr, ttbr);

Are we going with ARM64 names for HTTBR &c even on 32-bit?  Maybe 'TTBR'
would do in this case.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel