Re: [Xen-devel] [PATCH 02/38] arm: handy function to print a walk of the hypervisor page tables

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-07 at 09:45 +0100, Tim Deegan wrote:
> Hi,
> 
> At 15:39 +0000 on 01 Jun (1338565171), Ian Campbell wrote:
> > +void dump_pt_walk(uint32_t addr)
> > +{
> > +    paddr_t second_ma, third_ma;
> > +    lpae_t *first = NULL, *second = NULL, *third = NULL;
> > +    uint64_t httbr = READ_CP64(HTTBR);
> > +
> > +    printk("Walking Hypervisor VA %#"PRIx32" via HTTBR %#"PRIx64"\n",
> > +           addr, httbr);
> > +
> > +    BUG_ON( (lpae_t *)(unsigned long)(httbr - xen_phys_offset) != 
> > xen_pgtable );
> > +    first = xen_pgtable;
> > +    printk("1ST[%#03"PRIx32"] = %p[%#03"PRIx32"] = %#llx = %#016llx\n",
> > +           first_table_offset(addr),
> > +           first, first_table_offset(addr),
> > +           virt_to_maddr(&first[first_table_offset(addr)]),
> > +           first[first_table_offset(addr)].bits);
> > +
> > +    if ( !first[first_table_offset(addr)].pt.valid ||
> > +         !first[first_table_offset(addr)].pt.table )
> > +        goto done;
> 
> This could probably be a for loop rather than open-coding three
> almost-identical printks.

The *_table_offsets macro names are different at each stage, I suppose I
could open code the equivalent shifts, but I'd rather keep using the
access macros.

> > +
> > +    second_ma = (paddr_t)first[first_table_offset(addr)].pt.base << 
> > PAGE_SHIFT;
> > +    second = (lpae_t *)(unsigned long)(second_ma - xen_phys_offset);
> > +    printk("2ND[%#03"PRIx32"] = %p[%#03"PRIx32"] = %#llx = %#016llx\n",
> > +           second_table_offset(addr),
> > +           second, second_table_offset(addr),
> > +           virt_to_maddr(&second[second_table_offset(addr)]),
> > +           second[second_table_offset(addr)].bits);
> > +    if ( !second[second_table_offset(addr)].pt.valid ||
> > +         !second[second_table_offset(addr)].pt.table )
> > +        goto done;
> > +
> > +    third_ma = (paddr_t)second[second_table_offset(addr)].pt.base << 
> > PAGE_SHIFT;
> > +    third = (lpae_t *)(unsigned long)(third_ma - xen_phys_offset);
> > +    printk("3RD[%#03"PRIx32"] = %p[%#03"PRIx32"] = %#llx = %#016llx\n",
> > +           third_table_offset(addr),
> > +           third, third_table_offset(addr),
> > +           virt_to_maddr(&third[third_table_offset(addr)]),
> > +           third[third_table_offset(addr)].bits);
> > +    if ( !third[third_table_offset(addr)].pt.valid ||
> > +         !third[third_table_offset(addr)].pt.table )
> > +        goto done;
> > +
> > +done:
> > +    return;
> 
> Maybe use return above instead of goto?

Yes, good idea.

> 
> > +}
> > +
> >  /* Map a 4k page in a fixmap entry */
> >  void set_fixmap(unsigned map, unsigned long mfn, unsigned attributes)
> >  {
> > @@ -173,8 +222,8 @@ void __init setup_pagetables(unsigned long 
> > boot_phys_offset)
> >      flush_xen_data_tlb_va(dest_va);
> >  
> >      /* Calculate virt-to-phys offset for the new location */
> > -    phys_offset = xen_paddr - (unsigned long) _start;
> > -
> > +    xen_phys_offset = phys_offset = xen_paddr - (unsigned long) _start;
> > +    
> 
> Just make phys_offset file-scope static; no need to add a new variable
> for this.

Will do.

> 
> >      /* Copy */
> >      memcpy((void *) dest_va, _start, _end - _start);
> >  
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index b6df64e..22c56b5 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -312,6 +312,8 @@ static inline uint64_t gva_to_ipa(uint32_t va)
> >  /* Bits in the PAR returned by va_to_par */
> >  #define PAR_FAULT 0x1
> >  
> > +extern void dump_pt_walk(uint32_t addr);
> > +
> 
> Maybe a comment?

Yes, good idea.


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