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

[Tim Deegan <tim@xxxxxxx>]

At 12:02 +0000 on 14 Feb (1360843326), Ian Campbell wrote:
> 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 ;-))

Grand so. :)

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

Argh, so it is.  My apologies. 

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

Sure.  Or vaddr_t.

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

OK.

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

Also good.  My comment was really about the printk, which I think can
happily use 'TTBR' here since it's obvious in this case which one we mean.

Tim.

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