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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.


> > > +#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?


> > > +        /*
> > > +         * Locate the xenheap using these constraints:
> > > +         *
> > > +         *  - must be 32 MiB aligned
> > > +         *  - must not include Xen itself or the boot modules
> > > +         *  - must be at most 1 GiB
> > > +         *  - must be at least 128M
> > > +         *
> > > +         * We try to allocate the largest xenheap possible within these
> > > +         * constraints.
> > > +         */
> > 
> > this comment is not correct on ARM64, is it?
> 
> No, I'll fix it.
> 
> > > +#ifdef CONFIG_ARM_32
> > >  #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
> > >  #define is_xen_heap_mfn(mfn) ({                                 \
> > >      unsigned long _mfn = (mfn);                                 \
> > >      (_mfn >= xenheap_mfn_start && _mfn < xenheap_mfn_end);      \
> > >  })
> > 
> > I take that this check wouldn't work for arm64 because it would always
> > return true?
> 
> Correct.
> 
> > > +#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).

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