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 12:52 +0100, Ian Campbell wrote:
> On Tue, 2013-04-23 at 12:38 +0100, 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 */
> >             flush_xen_text_tlb();
> >         }
> 
> Weird, if I don't mark this function inline then it hangs at this point
> (on the model).
> 
> If I inline then all is fine.
> 
> Sounds like a missing flush or barrier which is exposed by the return
> "bx lr" or possibly the stack manipulations on the function epilogue.
> But I can't spot it...

The function is pushing the fp to the stack in the prologue, then
reading it back in the epilogue after the switch and getting some stale
value, which is pretty obvious now I think of it.

The compiler is being a bit dumb, since there are no stack local
variables so it does:
   0x00241e04 <+0>:     push    {r11}           ; (str r11, [sp, #-4]!)
   0x00241e08 <+4>:     add     r11, sp, #0
...
   0x00241e50 <+76>:    add     sp, r11, #0
   0x00241e54 <+80>:    pop     {r11}

which is all a bit pointless since the function has 0 local variables
and never touches the stack otherwise, but of well.

I'm going to stick an always_inline in there with a comment.

Ian.



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