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

[Tim Deegan <tim@xxxxxxx>]

At 12:38 +0100 on 23 Apr (1366720695), Ian Campbell wrote:
> On Tue, 2013-04-23 at 12:33 +0100, Tim Deegan wrote:
> > Hi, 
> > 
> > At 12:21 +0100 on 23 Apr (1366719671), Ian Campbell wrote:
> > > > > +    /* Change to this CPUs pagetables */
> > > > > +    flush_xen_text_tlb();
> > > > > +
> > > > > +    ttbr = (uintptr_t) virt_to_maddr(this_cpu(xen_pgtable));
> > > > > +    WRITE_SYSREG64(ttbr, TTBR0_EL2);
> > > > 
> > > > isb() here, to make sure the CP write completes? 
> > > 
> > > Hrm, we don't have one in the primary CPU bring up either. 
> > > 
> > > flush_xen_text_tlb starts with an isb, which I suspect is sufficient? (I
> > > vaguely remember having a conversation along those lines when this code
> > > was changed to look like this).
> > 
> > Hrmn.  I guess that does turn out OK, but given that this happens
> > exactly once per CPU, the extra ISB is a cheap enough price to pay for
> > making the code obviously correct.
> 
> True. I refactored this code into:
>         static void write_ttbr(uint64_t ttbr)
>         {
>             flush_xen_text_tlb();
>         
>             WRITE_SYSREG64(boot_ttbr, TTBR0_EL2);
>             dsb(); /* ensure memory accesses do not cross over the TTBR0 
> write */
>             isb(); /* ensure that the TTBT write has completed */

Actually I'm going to u-turn entirely here: looking at
flush_xen_text_tlb(), maybe we can drop the explicit ISB/DSB here
entirely, with a comment saying that they're taken care of.

If not, I think these need to be the other way around: isb to complete
the CP write, _then_ dsb to stop anything getting hoisted.

Also, I don't remember why we needed the extra flush_xen_text_tlb before
the TTBR write.  ISTR it was needed when we moved on to real hardware,
but can't recall exactly why.

>             flush_xen_text_tlb();
>         }
>         
> I wasn't 100% sure about carrying that first flush_xen_text_tlb to here
> instead of leaving it in the caller but my rationale was that it ensures
> things are in a consistent state before we make the switch and both
> callers include it.

Yep, sounds like a good idea.

Tim.

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