Re: [patch] i386: annotate the rest of entry.s::nmi

Re: [patch] i386: annotate the rest of entry.s::nmi

[Chuck Ebbert]

In-Reply-To: <44DB0927.76E4.0078.0@novell.com>

On Thu, 10 Aug 2006 10:23:35 +0200, Jan Beulich wrote:

> >Part of the NMI handler is missing annotations.  Just moving
> >the RING0_INT_FRAME macro fixes it.  And additional comments
> >should warn anyone changing this to recheck the annotations.
> 
> I have to admit that I can't see the value of this movement; the
> code sequence in question was left un-annotated intentionally.
> The point is that the push-es in FIX_STACK() aren't annotated, so
> things won't be correct at those points anyway.

I have a patch here that adds that, but it won't compile
because that part of the NMI handler is un-annotated:


i386: annotate the FIX_STACK macro.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>
---

 arch/i386/kernel/entry.S  |    8 +++++++-
 include/asm-i386/dwarf2.h |    2 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

--- 2.6.18-rc4-nb.orig/arch/i386/kernel/entry.S
+++ 2.6.18-rc4-nb/arch/i386/kernel/entry.S
@@ -698,9 +698,15 @@ device_not_available_emulate:
 	jne ok;					\
 label:						\
 	movl TSS_sysenter_esp0+offset(%esp),%esp;	\
+	CFI_DEF_CFA esp, 0;			\
+	CFI_UNDEFINED eip;			\
 	pushfl;					\
+	CFI_ADJUST_CFA_OFFSET 4;		\
 	pushl $__KERNEL_CS;			\
-	pushl $sysenter_past_esp
+	CFI_ADJUST_CFA_OFFSET 4;		\
+	pushl $sysenter_past_esp;		\
+	CFI_ADJUST_CFA_OFFSET 4;		\
+	CFI_REL_OFFSET eip, 0
 
 KPROBE_ENTRY(debug)
 	RING0_INT_FRAME
--- 2.6.18-rc4-nb.orig/include/asm-i386/dwarf2.h
+++ 2.6.18-rc4-nb/include/asm-i386/dwarf2.h
@@ -28,6 +28,7 @@
 #define CFI_RESTORE .cfi_restore
 #define CFI_REMEMBER_STATE .cfi_remember_state
 #define CFI_RESTORE_STATE .cfi_restore_state
+#define CFI_UNDEFINED .cfi_undefined
 
 #else
 
@@ -48,6 +49,7 @@
 #define CFI_RESTORE	ignore
 #define CFI_REMEMBER_STATE ignore
 #define CFI_RESTORE_STATE ignore
+#define CFI_UNDEFINED ignore
 
 #endif
 
-- 
Chuck

Re: [patch] i386: annotate the rest of entry.s::nmi

[Andi Kleen]

On Thursday 10 August 2006 14:48, Chuck Ebbert wrote:
> In-Reply-To: <44DB0927.76E4.0078.0@novell.com>
> 
> On Thu, 10 Aug 2006 10:23:35 +0200, Jan Beulich wrote:
> 
> > >Part of the NMI handler is missing annotations.  Just moving
> > >the RING0_INT_FRAME macro fixes it.  And additional comments
> > >should warn anyone changing this to recheck the annotations.
> > 
> > I have to admit that I can't see the value of this movement; the
> > code sequence in question was left un-annotated intentionally.
> > The point is that the push-es in FIX_STACK() aren't annotated, so
> > things won't be correct at those points anyway.
> 
> I have a patch here that adds that, but it won't compile
> because that part of the NMI handler is un-annotated:

Ok i dropped the original patch for now and you guys can work
out a correct fix.

-Andi

Re: [patch] i386: annotate the rest of entry.s::nmi

[Jan Beulich]

>> >Part of the NMI handler is missing annotations.  Just moving
>> >the RING0_INT_FRAME macro fixes it.  And additional comments
>> >should warn anyone changing this to recheck the annotations.
>> 
>> I have to admit that I can't see the value of this movement; the
>> code sequence in question was left un-annotated intentionally.
>> The point is that the push-es in FIX_STACK() aren't annotated, so
>> things won't be correct at those points anyway.
>
>I have a patch here that adds that, but it won't compile
>because that part of the NMI handler is un-annotated:

But you didn't clarify why you need this piece of code annotated...

Jan

Re: [patch] i386: annotate the rest of entry.s::nmi

[Chuck Ebbert]

In-Reply-To: <44DC496D.76E4.0078.0@novell.com>

On Fri, 11 Aug 2006 09:10:05 +0200, Jan Beulich wrote:

> I understand now, but am still uncertain
> about the need to annotate FIX_STACK() - especially since you use
> .cfi_undefined, meaning the return point cannot be established anyway.
> If at all I'd annotate the initial pushes with either just the normal
> CFI_ADJUST_CFA_OFFSET, and the final one with one setting back the
> CFA base to the now adjusted frame. That way, until the pushes are
> complete the old frame will be used for determining the call origin,
> and once complete the (full) new state will be used.

But that's the whole point of the new annotations -- we have just
overwritten %esp with a new value and the old assumptions are
completely broken:

        movl TSS_sysenter_esp0+offset(%esp),%esp;       \

After this the old frame cannot be located by using %esp as a base
and the new frame is incomplete.  So the only choice is to make eip
undefined until the new value is available -- if not then the
unwinder will try to use whatever random values are on the new frame.
Either that or I'm still unclear on how unwind works...

-- 
Chuck

Re: [patch] i386: annotate the rest of entry.s::nmi

[Jan Beulich]

>> I understand now, but am still uncertain
>> about the need to annotate FIX_STACK() - especially since you use
>> .cfi_undefined, meaning the return point cannot be established
anyway.
>> If at all I'd annotate the initial pushes with either just the
normal
>> CFI_ADJUST_CFA_OFFSET, and the final one with one setting back the
>> CFA base to the now adjusted frame. That way, until the pushes are
>> complete the old frame will be used for determining the call
origin,
>> and once complete the (full) new state will be used.
>
>But that's the whole point of the new annotations -- we have just
>overwritten %esp with a new value and the old assumptions are
>completely broken:
>
>        movl TSS_sysenter_esp0+offset(%esp),%esp;       \
>
>After this the old frame cannot be located by using %esp as a base
>and the new frame is incomplete.  So the only choice is to make eip
>undefined until the new value is available -- if not then the
>unwinder will try to use whatever random values are on the new frame.
>Either that or I'm still unclear on how unwind works...

Hmm, yes, on a second look I agree.

Jan

Re: [patch] i386: annotate the rest of entry.s::nmi

[Chuck Ebbert]

In-Reply-To: <44DB532F.76E4.0078.0@novell.com>

On Thu, 10 Aug 2006 15:39:27 +0200, Jan Beulich wrote:
>
> >> The point is that the push-es in FIX_STACK() aren't annotated, so
> >> things won't be correct at those points anyway.
> >
> >I have a patch here that adds that, but it won't compile
> >because that part of the NMI handler is un-annotated:
> 
> But you didn't clarify why you need this piece of code annotated...

Uh, which one didn't I clarify?

FIX_STACK() is already invoked from debug(), which is annotated, but
FIX_STACK() isn't.  And that messes with the stack, so for a few
instructions the annotations are all wrong.

When I annotated FIX_STACK(), I found entry.S wouldn't compile because
nmi() included FIX_STACK() but was completely missing annotations
in that piece. So I added them so FIX_STACK()'s annotations would
compile...

Should I send a combined patch, leave the two patches separate, or just
drop it?

-- 
Chuck

Re: [patch] i386: annotate the rest of entry.s::nmi

[Jan Beulich]

>> >> The point is that the push-es in FIX_STACK() aren't annotated, so
>> >> things won't be correct at those points anyway.
>> >
>> >I have a patch here that adds that, but it won't compile
>> >because that part of the NMI handler is un-annotated:
>> 
>> But you didn't clarify why you need this piece of code annotated...
>
>Uh, which one didn't I clarify?
>
>FIX_STACK() is already invoked from debug(), which is annotated, but
>FIX_STACK() isn't.  And that messes with the stack, so for a few
>instructions the annotations are all wrong.
>
>When I annotated FIX_STACK(), I found entry.S wouldn't compile
because
>nmi() included FIX_STACK() but was completely missing annotations
>in that piece. So I added them so FIX_STACK()'s annotations would
>compile...

Ah, okay, this means the original sequence of additions was the reverse
of
how I got to see these patches. I understand now, but am still
uncertain
about the need to annotate FIX_STACK() - especially since you use
.cfi_undefined, meaning the return point cannot be established anyway.
If at all I'd annotate the initial pushes with either just the normal
CFI_ADJUST_CFA_OFFSET, and the final one with one setting back the
CFA base to the now adjusted frame. That way, until the pushes are
complete the old frame will be used for determining the call origin,
and
once complete the (full) new state will be used.
Or annotate them so that the new values take effect immediately with
each push, but clearly without any CFI_UNDEFINED. That way, the
frame will be slightly inconsistent in between, which could be of
concern
once we also properly annotate the segment register spills/restores.

>Should I send a combined patch, leave the two patches separate, or
just
>drop it?

Either way, but if you leave them separate you should always send them
as pair, to make the intentions clear.

Jan

[patch] i386: annotate the rest of entry.s::nmi

[Chuck Ebbert]

Part of the NMI handler is missing annotations.  Just moving
the RING0_INT_FRAME macro fixes it.  And additional comments
should warn anyone changing this to recheck the annotations.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

--- 2.6.18-rc4-nb.orig/arch/i386/kernel/entry.S
+++ 2.6.18-rc4-nb/arch/i386/kernel/entry.S
@@ -750,6 +750,7 @@ ENTRY(nmi)
 	cmpl $sysenter_entry,12(%esp)
 	je nmi_debug_stack_check
 nmi_stack_correct:
+	/* We have a RING0_INT_FRAME here */
 	pushl %eax
 	CFI_ADJUST_CFA_OFFSET 4
 	SAVE_ALL
@@ -760,9 +761,12 @@ nmi_stack_correct:
 	CFI_ENDPROC
 
 nmi_stack_fixup:
+	RING0_INT_FRAME
 	FIX_STACK(12,nmi_stack_correct, 1)
 	jmp nmi_stack_correct
+
 nmi_debug_stack_check:
+	/* We have a RING0_INT_FRAME here */
 	cmpw $__KERNEL_CS,16(%esp)
 	jne nmi_stack_correct
 	cmpl $debug,(%esp)
@@ -773,8 +777,10 @@ nmi_debug_stack_check:
 	jmp nmi_stack_correct
 
 nmi_16bit_stack:
-	RING0_INT_FRAME
-	/* create the pointer to lss back */
+	/* We have a RING0_INT_FRAME here.
+	 *
+	 * create the pointer to lss back
+	 */
 	pushl %ss
 	CFI_ADJUST_CFA_OFFSET 4
 	pushl %esp
-- 
Chuck

Re: [patch] i386: annotate the rest of entry.s::nmi

[Andi Kleen]

On Thursday 10 August 2006 06:59, Chuck Ebbert wrote:
> Part of the NMI handler is missing annotations.  Just moving
> the RING0_INT_FRAME macro fixes it.  And additional comments
> should warn anyone changing this to recheck the annotations.

Added thanks.
-Andi

Re: [patch] i386: annotate the rest of entry.s::nmi

[Jan Beulich]

>>> Chuck Ebbert <76306.1226@compuserve.com> 10.08.06 06:59 >>>
>Part of the NMI handler is missing annotations.  Just moving
>the RING0_INT_FRAME macro fixes it.  And additional comments
>should warn anyone changing this to recheck the annotations.

I have to admit that I can't see the value of this movement; the
code sequence in question was left un-annotated intentionally.
The point is that the push-es in FIX_STACK() aren't annotated, so
things won't be correct at those points anyway. Furthermore,
getting interrupted in any way while in this code path is going to
kill the system (and is just impossible AFAICT), so annotations
there also seem worthless. (I am actually surprised that I
annotated the code after nmi_16bit_stack, as that's similarly
constrained; even more, as I'm now looking at it, this code seems
outright wrong in using iret since that unmasks NMIs - Stas, is
your pending adjustment to the 16-bit stack handling going to
overcome this?)

The added comments certainly might be helpful, though there are
more places where frame state gets "inherited" across labels.

Jan

Re: [patch] i386: annotate the rest of entry.s::nmi

[Andi Kleen]

On Thursday 10 August 2006 10:23, Jan Beulich wrote:
> >>> Chuck Ebbert <76306.1226@compuserve.com> 10.08.06 06:59 >>>
> >Part of the NMI handler is missing annotations.  Just moving
> >the RING0_INT_FRAME macro fixes it.  And additional comments
> >should warn anyone changing this to recheck the annotations.
> 
> I have to admit that I can't see the value of this movement; 

Should I drop it again? 

-Andi 

Re: [patch] i386: annotate the rest of entry.s::nmi

[Jan Beulich]

>>> Andi Kleen <ak@suse.de> 10.08.06 10:26 >>>
>On Thursday 10 August 2006 10:23, Jan Beulich wrote:
>> >>> Chuck Ebbert <76306.1226@compuserve.com> 10.08.06 06:59 >>>
>> >Part of the NMI handler is missing annotations.  Just moving
>> >the RING0_INT_FRAME macro fixes it.  And additional comments
>> >should warn anyone changing this to recheck the annotations.
>> 
>> I have to admit that I can't see the value of this movement; 
>
>Should I drop it again? 

I would suggest so, but would also want to wait for Chuck to perhaps
provide some background on why he felt these annotations were
useful.

Jan

Re: [patch] i386: annotate the rest of entry.s::nmi

[Stas Sergeev]

Hello.

Jan Beulich wrote:
> even more, as I'm now looking at it, this code seems
> outright wrong in using iret since that unmasks NMIs - Stas, is
> your pending adjustment to the 16-bit stack handling going to
> overcome this?)
No, it leaves the NMI path almost untouched.
But what exactly problem do you see with this iret?
If it unmasks NMI, then it does so for reason, which
is a return from an NMI handler. What exactly is wrong
with it?

Re: [patch] i386: annotate the rest of entry.s::nmi

[Jan Beulich]

>> even more, as I'm now looking at it, this code seems
>> outright wrong in using iret since that unmasks NMIs - Stas, is
>> your pending adjustment to the 16-bit stack handling going to
>> overcome this?)
>No, it leaves the NMI path almost untouched.
>But what exactly problem do you see with this iret?
>If it unmasks NMI, then it does so for reason, which
>is a return from an NMI handler. What exactly is wrong
>with it?

Oh, yes, you're right, I didn't pay attention to the second do_nmi
call in that path.

Sorry, Jan