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

[Tim Deegan <tim@xxxxxxx>]

At 16:21 +0100 on 23 Apr (1366734090), Ian Campbell wrote:
> > > 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.
> 
> flush_xen_text_tlb is:
>         "isb;"                        /* Ensure synchronization with previous 
> changes to text */
>         STORE_CP32(0, TLBIALLH)       /* Flush hypervisor TLB */
>         STORE_CP32(0, ICIALLU)        /* Flush I-cache */
>         STORE_CP32(0, BPIALL)         /* Flush branch predictor */
>         "dsb;"                        /* Ensure completion of TLB+BP flush */
>         "isb;"
> 
> So I think we still need the dsb?

That depends what the DSB is for. :)  If it's to stop any later data
accesses being hoisted past the TTBR write, I think the one in
flush_xen_text_tlb() will do -- none of the instructions before it are
memory operations.  If it's to stop any earlier writes being sunk past
the TTBR write, surely it needs to go before the WRITE_SYSREG64() itself, 
not before the ISB.

Or maybe I'm (once again) confused about exactly what these barriers are
doing.  Clearly something's going on here that I don't quite understand
yet, if the return from write_ttbr is crashing.

> > 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.
> 
> Isn't it the case that because there are no instructions between the
> isb/dsb and neither of them access memory themselves it doesn't really
> matter which way around they are?

I think that since the purpose of the ISB is to synchronize against the
MMU finishing the TTBR update, then allowing reads to be hoisted before
it must be a Bad Thing[tm].

> > 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.
> 
> Stefano seems to have added it in a8c8110333 but it replaced an open
> coded asm inline with mostly the same affect (the function has an extra
> BPIALL). The asm inline came from the initial checkin...

Oh. :|  In that case I'm not sure what use it is; it seems like a
siimple 'isb; dsb;' should do to make sure we're ready for the TTBR
switch.

Tim.

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