Re: [PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Ingo Molnar <mingo@elte.hu>]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Ingo,
> 
> On Mon, 2012-02-27 at 08:39 +0100, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > 
> > > Ingo,
> > > 
> > > Jan posted a patch that fixes the CFI annotations. I recommend getting
> > > this into 3.3 as this is new code and it would be nice to have CFI
> > > correct. It also does a little simplification of it as well.
> > > 
> > > The second patch is comment changes only (very low impact on messing
> > > anything up). I realized that the comments had some references to
> > > previous approaches that I tried, and I fixed them to reflect what
> > > the final result was. I also added some more comments to describe
> > > the code a bit better.
> > > 
> > > Thanks,
> > > 
> > > -- Steve
> > > 
> > > Please pull the latest tip/x86/urgent tree, which can be found at:
> > > 
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> > > tip/x86/urgent
> > > 
> > > Head SHA1: 79fb4ad63e8266ffac1f69bbb45a6f86570493e7
> > > 
> > > 
> > > Jan Beulich (1):
> > >       x86-64: Fix CFI annotations for NMI nesting code
> > > 
> > > Steven Rostedt (1):
> > >       x86: Fix the NMI nesting comments
> > > 
> > > ----
> > >  arch/x86/kernel/entry_64.S |   64 ++++++++++++++++++++++++--------------------
> > >  1 files changed, 35 insertions(+), 29 deletions(-)
> > 
> > I don't think we want a 30+ lines diffstat to this rather 
> 
> Some of that was just movement of code.

That's why I said 30+ lines, not 60.

> > non-trivial NMI codepath - and it changes real instructions, 
> > not just the CFI annotations.
> 
> The changes to the real code made the CFI code easier to fix. 
> But if you are nervous about the code change (which actually 
> simplifies the code), I can ask Jan (Cc'd) to break out the 
> patch into two changes if possible.
> 
> I'm not sure how the CFI can handle the current trampoline, 
> but perhaps we can just fix the main part of the code and 
> leave the trampoline part broken? Then we can add the rest of 
> the CFI changes and the movement of the trampoline back into 
> the function for the next release.
> 
> > 
> > Also, the 'update comments' commit does not belong into 
> > x86/urgent either.
> 
> Hmm, I didn't know that fixing comments was for a merge window 
> only. [...]

We try to avoid them for later -rc's, and we are now in -rc5 
territory already.

> [...] Some of the comments are currently wrong and I didn't 
> think we would want those in a main release. While reading the 
> code again I realized that I could also add more comments to 
> make it easier to understand. I would think that comments 
> would be fine for the -rc releases because they have almost no 
> chance of introducing bugs.

By that argument we could be doing mechanic API renames in later 
-rc's as well.

In general we don't want "noise" around the really relevant 
changes to make them *really* obvious regression fixes - even if 
this noise is obviously correct code.

This is also code that *everyone* uses so if it breaks then 
everyone suffers (and seeing people suffer, even hypothetically, 
makes me sad), so the maintenance filter is quite a bit 
stricter. I wouldn't worry nearly as much about 
drivers/some/obscure/piece/of/hw.c.

> > So either you do an obviously trivial patch that only adds 
> > CFI annotations and nothing else, or I can pull these bits 
> > into tip:x86/debug, for a v3.4 merge.
> 
> I'm fine with waiting for v3.4 before these changes get in.

Jan, is that fine with you?

Thanks,

	Ingo