Re: [Xen-devel] [PATCH 10/10] xen: arm: Use a direct mapping of RAM on arm64

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-06-26 at 19:32 +0100, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > >  void __init arch_init_memory(void)
> > > >  {
> > > > @@ -431,6 +454,7 @@ void __init setup_pagetables(unsigned long 
> > > > boot_phys_offset, paddr_t xen_paddr)
> > > >      /* Flush everything after setting WXN bit. */
> > > >      flush_xen_text_tlb();
> > > >  
> > > > +#ifdef CONFIG_ARM_32
> > > >      per_cpu(xen_pgtable, 0) = boot_pgtable;
> > > >      per_cpu(xen_dommap, 0) = xen_second +
> > > >          second_linear_offset(DOMHEAP_VIRT_START);
> > > > @@ -440,43 +464,31 @@ void __init setup_pagetables(unsigned long 
> > > > boot_phys_offset, paddr_t xen_paddr)
> > > >      memset(this_cpu(xen_dommap), 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
> > > >      flush_xen_dcache_va_range(this_cpu(xen_dommap),
> > > >                                DOMHEAP_SECOND_PAGES*PAGE_SIZE);
> > > > +#endif
> > > >  }
> > > >  
> > > >  int init_secondary_pagetables(int cpu)
> > > >  {
> > > > -    lpae_t *root, *first, *domheap, pte;
> > > > -    int i;
> > > > -
> > > > -    root = alloc_xenheap_page();
> > > >  #ifdef CONFIG_ARM_64
> > > > -    first = alloc_xenheap_page();
> > > > +    /* All CPUs share a single page table on 64 bit */
> > > > +    return 0;
> > > 
> > > I would just ifdef the entire function
> > 
> > I expect you don't mean for me to also ifdef the call?
> 
> I meant:
> 
> #ifdef CONFIG_ARM_64
> int init_secondary_pagetables(int cpu)
> {
>     bla64
> #elif CONFIG_ARM_32
> int init_secondary_pagetables(int cpu)
> {
>     bla32
> #endif
> 
> 
> 
> > I preferred to
> > write the prototype only once so the two didn't get out of sync in the
> > future, which can be an issue with the pattern of stubbing out functions
> > with a separate static inline. I can change that if you have a strong
> > preference though.
> 
> This is not a static function, if the prototype of the arm64 or the
> prototype of the arm32 function changed we would know.

My point is that it is easy for someone to change only one version (and
the caller) and unless they remember to build test the other arch the
breakage won't be spotted until later. Likely during my pre-commit build
tests since it is also the sort of thing which slips through review.

> 
> 
> > > > +#ifdef CONFIG_ARM_64
> > > > +    nr_second = frametable_size >> SECOND_SHIFT;
> > > > +    second_base = alloc_boot_pages(nr_second, 1);
> > > > +    second = mfn_to_virt(second_base);
> > > > +    for ( i = 0; i < nr_second; i++ )
> > > > +    {
> > > > +        pte = mfn_to_xen_entry(second_base + i);
> > > > +        pte.pt.table = 1;
> > > > +        
> > > > write_pte(&boot_first[first_table_offset(FRAMETABLE_VIRT_START)+i], 
> > > > pte);
> > > > +    }
> > > > +    create_32mb_mappings(second, 0, base_mfn, frametable_size >> 
> > > > PAGE_SHIFT);
> > > > +#else
> > > >      create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn, 
> > > > frametable_size >> PAGE_SHIFT);
> > > > +#endif
> > > 
> > > Is it possible to unify the arm32 and arm64 code paths here?
> > 
> > I tried to unify them as much as I could.
> > 
> > > Being able to handle a frametable that spans multiple second level
> > > pagetable pages shouldn't be a problem for arm32.
> > 
> > That's not the issue. The issue is that the frame table lives at
> > different addresses in both cases, and in particular 64-bit has its own
> > first level pages with dedicated second level pages for this region
> > while 32-bit has a small number of second level entries under the same
> > first level entry as various other bits of Xen (including .text, vmap,
> > ioremap region etc etc).
> 
> What I mean is that it seems to me that all the code you are adding
> under the #ifdef CONFIG_ARM_64 should actually work on CONFIG_ARM_32 too
> if you assume that one second level page has already been allocated on
> ARM32.  Is that right?

If one second level page has already been allocated then the code inside
the ifdef isn't needed -- since it is exactly allocating the required
second level pages for 64-bit. Hence the code in the ifdef is not
required on 32-bit but is on 64-bit.

I could combine the final create_32mb_mappings call with
        #if COFNIG_ARM64
            // existing code
            frametable_second_virt_offset = 0;
        #else 
            second = xen_second;
            frametable_second_virt_offset = FRAMETABLE_VIRT_START;
        #endif
            create_32mb_mappings(second, frametable_second_virt_offset, 
base_mfn, frametable_size >> PAGE_SHIFT);

But that doesn't seem much cleaner.

> > > > +#else
> > > > +#define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
> > > > +#define is_xen_heap_mfn(mfn) \
> > > > +    (mfn_valid(mfn) && is_xen_heap_page(__mfn_to_page(mfn)))
> > > > +#endif
> > > 
> > > Looks like the ARM64 version of these two functions should work for
> > > arm32 too.
> > 
> > arm32 doesn't set PGC_xen_heap, I don't think. It could I suppose.
> 
> I think it does. Actually it's the generic code that does it
> (xen/common/page_alloc.c:alloc_xenheap_pages).

You are looking there at the !CONFIG_SEPARATE_XENHEAP (previously X86)
variant of that function, which is the non-split domheap/xenheap case.

On arm32 we use the other variant (split heaps) which doesn't use
PGC_xen_heap, it's about a page above the function you are looking at...

Ian.



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