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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-02-14 at 11:48 +0000, Tim Deegan wrote:
> 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)

They talk about level 0..3 in their books. (In theory their scheme
allows for five levels, not sure what happens then ;-))

> >  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.

The cast is associated with the %lx, not the %PRIpaddr. I think PRIpaddr
is right where it is used.

Casting in the _offset() might be better -- unsigned long is good?

> >             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.

For talking about actual registers I want to use ARM64 names in any
where that isn't 100% 32-bit code. Partly because it makes the accessor
macros easier (because the 64-bit gas speaks the names natively, without
cpp magic) and also just because I think consistence is worthwhile,
nothing would be worse than mixing and matching in common code!

For variable names the arm64 names are a bit ugly though (e.g.
ttbr0_el2), I think ttbr is a good compromise if the context is such
that its not confusing.

Ian.



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