Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker <fweisbec@gmail.com>]

On Fri, Jan 07, 2011 at 07:45:53AM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 18:12, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Thu, Jan 06, 2011 at 04:58:54PM +0000, Jan Beulich wrote:
> >> >>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> > On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
> >> >> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> >> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> >> >> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> >> >> > Before we had:
> >> >> >> > 
> >> >> >> > 
> >> >> >> > 	leaveq
> >> >> >> > 
> >> >> >> > 	CFI_RESTORE             rbp
> >> >> >> > 	CFI_DEF_CFA_REGISTER    rsp
> >> >> >> > 	CFI_ADJUST_CFA_OFFSET   -8
> >> >> >> > 
> >> >> >> > So CFI_RESTORE means rbp has now the value of the base frame of
> >> >> >> > the calling frame (the base frame pointer of the interrupted proc) ?
> >> >> >> 
> >> >> >> No - all it means is that %rbp now has its original (caller or
> >> >> >> interrupted procedure) value again (i.e. an unwinder should not
> >> >> >> try to read it from the stack [or other previously recorded
> >> >> >> location] anymore).
> >> >> >> 
> >> >> >> > And what follows means that rsp-8 points to the return address?
> >> >> >> 
> >> >> >> No - .cfi_def_cfa_register says which register serves as the frame
> >> >> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> >> >> >> the frame pointer to the top [or bottom] of frame. At any time
> >> >> >> 
> >> >> >> 	CFA = cfa_register + cfa_offset
> >> >> >> 
> >> >> >> with CFA being what all locations on the stack are expressed
> >> >> >> relative to.
> >> >> > 
> >> >> > Ok.
> >> >> > 
> >> >> > So here rsp points to pt_regs::r11
> >> >> > 
> >> >> > I don't understand why locations relative to the stack must be
> >> >> > expressed here by taking rsp - 8 as a base.
> >> >> 
> >> >> Nothing says rsp-8. The annotations merely say to set the base
> >> >> register to rsp and to *adjust* the offset by -8 (after all, that's
> >> >> what the leaveq instruction does).
> >> > 
> >> > Ah! So CFA acts like a virtual frame base pointer right?
> >> 
> >> Correct.
> > 
> > Ah great. I was starting to prepare for the case you come to stab me :)
> > 
> > So what do you think about that:
> > 
> >         leaveq
> > 
> >         CFI_RESTORE             rbp
> >         CFI_DEF_CFA_REGISTER    rsp
> >         CFI_ADJUST_CFA_OFFSET   -8
> > 
> >        /* we did not save rbx, restore only from ARGOFFSET */
> >        addq $8, %rsp
> >        CFI_ADJUST_CFA_OFFSET   -16
> > 
> > 
> > Does that look correct to you? We increased rsp to start recovering
> > the regs from the right place, but the frame pointer of the current
> > proc must stay what it was.
> 
> As you hinted in your subsequent reply - it's -8 here (that's
> why the directive is named *adjust*; there are other
> directives allowing to *set* an offset).

Ok, I'll respin with a proper patch then.
 
> > Now I don't understand how this is all useful as this is not a normal
> > proc but an interruption. We can't get back the return address from
> > the CFA. Or am I missing something?
> 
> Unwind annotations, when written correctly, allow unwinding
> through all kinds of execution flows, including interrupts or
> exceptions as well as including stack switches.

Hmm I see, I guess this is handled through the movq_cfi things we have,
so that the unwinder can ignore the whole part with the saved registers
after which we can find the instruction pointer (considered as the return
address) saved by the hardware.

Fine, thanks for your explanations!