Re: [PATCH 1/3] [RFC][x86] move notify_die from nmi.c to traps.c

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, Feb 02, 2010 at 01:27:46PM -0500, Don Zickus wrote:
> > > > > +	        if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> > > > > +	        			                == NOTIFY_STOP)
> > > > > +	                return;
> > > > > +
> > > > >  #ifdef CONFIG_X86_LOCAL_APIC
> > > > >  		/*
> > > > >  		 * Ok, so this is none of the documented NMI sources,
> > > > > --
> > > > 
> > > > Hi Don, I suppose this notify_die should be in CONFIG_X86_LOCAL_APIC
> > > > section?
> > > 
> > > To maintain old behaviour I suppose, yes.  Personally I don't think
> > > notify_die has anything to do with CONFIG_X86_LOCAL_APIC so I put it in
> > > above the #define.
> > > 
> > 
> > I think it is. It becomes that if some (possible buggy in future) code
> > notify default_do_nmi via NOTIFY_STOP we may loose unknown_nmi_error
> 
> How is that different if the code is under the #define?
> 

It matters if code is compiled without CONFIG_X86_LOCAL_APIC.

> > for non-apic configs. And I reckon that even DIE_NMI_IPI is a bit "weird"
> > by not being under apic here, but this one should stay there in a
> > sake of kgdb I guess.
> 
> So you are saying the only way to get NMIs on x86 is through the local
> apic?  That seems odd.

No, I didn't say that. I said that semantic of DIE_NMI_IPI is somehow
weird for me. There is no IPI if no apic present (at least in official
way). But in a sake of kgdb which checks for this notification we should
save it here.

> 
> I really don't care either way, I was just trying to cleanup the code to
> make it easier to understand.  Putting it under the #define didn't seem to
> make sense (though that doesn't mean there is a valid reason).
> 

Don, please don't get me wrong. At moment I don't see problems with putting
DIE_NMI in a place you had put it at. But something worrying me, can't explain
what exactly, perhaps that is how paranoia happens :)

>
> Cheers,
> Don
> 
	-- Cyrill