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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 23 Apr 2013, Ian Campbell wrote:
> > > +    {
> > > +        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..

No, but fewer assumptions we make, less error prone is the code


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

Ah, that's right


> > > +    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 ;-)
 
Yes, I was agreeing :)

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