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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Jan 06, 2011 at 03:18:19PM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 15:51, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > From the x86_64 low level interrupt handlers, the frame pointer is
> > saved in pt_regs in the wrong place, in the offset of rbx.
> > 
> > rbx is not part of the irq partial saved registers, so it's not
> > critical, but later code that uses get_irq_regs() to get the
> > interrupted frame pointer instead get a stale rbx value, causing
> > unwinding code to fail miserably.
> 
> Code using get_irq_regs() can't rely on the not explicitly
> saved registers' fields of pt_regs anyway; you now fix this
> for %rbp, but someone else might look at a different
> register and want that to be saved too. You just shouldn't,
> and the fact that %rbp happens to be saved at all shouldn't
> be taken to mean you can access it via the provided
> pt_regs pointer. This saving of %rbp could go away or be
> done in a different way at any point.

Right. That was something I was wondering about: is rbp saved here
as the beginning of the proc stack, or was is supposed to be
part of the pt_regs although mistakenly recorded in rbp?

So given what you are explaining me, it rather seem it wasn't supposed
to be of part pt_regs and it's there because it begins the frame of the irq
handler.

I agree that in the case of common irqs, rbp is not supposed to be part
of saved regs. I think may be we can change that semantic though as it
requires very few changes. In fact the only added overhead is that addq
below.

I'm waiting for opinions though.

 
> > @@ -808,6 +813,8 @@ ret_from_intr:
> >  	TRACE_IRQS_OFF
> >  	decl PER_CPU_VAR(irq_count)
> >  	leaveq
> > +	/* we did not save rbx, restore only from ARGOFFSET */
> > +	addq $8, %rsp
> >  	CFI_RESTORE		rbp
> >  	CFI_DEF_CFA_REGISTER	rsp
> >  	CFI_ADJUST_CFA_OFFSET	-8
> 
> *If* the patch was to be taken anyway, I would strongly suggest
> getting this mis-insertion fixed first: CFI annotations belong to the
> immediately preceding instruction, and hence you must not insert
> new instructions between an existing one and its annotation.

Good to know, will fix.

> Furthermore, an adjustment to %rsp (when it serves as the frame
> pointer as is the case here, though would be better visible if the
> added instruction was at the right place) needs to be annotated
> itself.


Hmm, I'm not familiar with those CFI adjustments.
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) ?

And what follows means that rsp-8 points to the return address?
But it doesn't seem to be the case, the return address here beeing
rip stored in pt_regs...

I'm confused.