Re: [Xen-devel] [PATCH 4/4] arm: allocate per-PCPU domheap pagetable pages

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-04-22 at 19:16 +0100, Stefano Stabellini wrote:
> On Mon, 22 Apr 2013, Ian Campbell wrote:
> > +    if ( root == NULL || domheap == NULL || first == NULL
> > +        )
> 
> code style

Remnant of an ifdef around first, fixed.

> > +    /* Update the first level mapping to reference the local CPUs
> > +     * domheap mapping pages. */
> > +    for ( i = 0; i < 2; i++ )
> 
> instead of being hardcoded to "2", shouldn't the limit be based on
> DOMHEAP_SECOND_PAGES?

Yes, thought I'd caught all these, thanks!

> > +    {
> > +        pte = mfn_to_xen_entry(virt_to_mfn(domheap+i*LPAE_ENTRIES));
> > +        pte.pt.table = 1;
> > +        
> > write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
> 
> Also shouldn't we add an ASSERT to check that DOMHEAP_VIRT_START is
> properly aligned?

It would be a BUILD_BUG_ON I think and this would be far from the first
place where that would be a problem, are we terribly worried about
people changing config.h in ways which would break this? They'd notice
pretty quickly..

> > +    }
> > +
> > +    per_cpu(xen_pgtable, cpu) = root;
> > +    per_cpu(xen_dommap, cpu) = domheap;
> > +
> > +    return 0;
> >  }
> >  
> >  /* MMU setup for secondary CPUS (which already have paging enabled) */
> >  void __cpuinit mmu_init_secondary_cpu(void)
> >  {
> > +    uint64_t ttbr;
> > +
> > +    /* Change to this CPUs pagetables */
> > +    ttbr = (uintptr_t) virt_to_maddr(this_cpu(xen_pgtable));
> 
> we should be flushing this ttbr write

ttbr is a local variable, it's probably only in a register, it's used
for the following SYSREG_WRITE to TTBR0_EL2.

> > +    flush_xen_dcache_va_range(this_cpu(xen_pgtable), PAGE_SIZE);
> > +    flush_xen_dcache_va_range(this_cpu(xen_dommap),
> > +                              DOMHEAP_SECOND_PAGES*PAGE_SIZE);
> 
> Given that these pagetable pages are written by cpu0, I wonder whether we
> actually need to execute any of these flushes on secondary cpus. I think
> they should be moved to init_secondary_pagetables.

I wondered about that too, TBH I'm not sure but I think you are probably
right, I will move them.

> > +    flush_xen_text_tlb();
> >
> > +    WRITE_SYSREG64(ttbr, TTBR0_EL2);
> > +    dsb();                         /* Ensure visibility of HTTBR update */
> > +    flush_xen_text_tlb();
> 
> The two flush_xen_text_tlb are probably necessary at least for the
> I-cache and the BP.

You are agreeing with the code, rather than suggesting a change, right?

If you are suggesting a change I've not understood what it is ;-)

Ian.


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