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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-04-23 at 13:26 +0100, Tim Deegan wrote:
> 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.

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?

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

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

Ian.



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