RE: [V5 PATCH 4/4] x86/apic: Introduce apic_extnmi boot option

[æåèå / KAWAIïHIDEHIRO]

> On Fri, Nov 20, 2015 at 06:36:50PM +0900, Hidehiro Kawai wrote:
> > This patch introduces new boot option, apic_extnmi:
> >
> >  apic_extnmi={ bsp | all | none}
> >
> > The default value is "bsp" and this is the current behavior; only
> > BSP receives external NMI.  "all" allows external NMIs to be
> > broadcast to all CPUs.  This would raise the success rate of panic
> > on NMI when BSP hangs up in NMI context or the external NMI is
> > swallowed by other NMI handlers on BSP.  If you specified "none",
> > any CPUs don't receive external NMIs.  This is useful for dump
> > capture kernel so that it wouldn't be shot down while saving a
> > crash dump.
> >
> > V5:
> > - Rename the option from "noextnmi" to "apic_extnmi"
> > - Add apic_extnmi=all feature
> > - Fix the wrong documentation about "noextnmi" (apic_extnmi=none)
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@xxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: Jonathan Corbet <corbet@xxxxxxx>
> > ---
> >  Documentation/kernel-parameters.txt |    9 +++++++++
> >  arch/x86/include/asm/apic.h         |    5 +++++
> >  arch/x86/kernel/apic/apic.c         |   31 ++++++++++++++++++++++++++++++-
> >  3 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index f8aae63..ceed3bc 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -472,6 +472,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >  			Change the amount of debugging information output
> >  			when initialising the APIC and IO-APIC components.
> >
> > +	apic_extnmi=	[APIC,X86] External NMI delivery setting
> > +			Format: { bsp (default) | all | none }
> > +			bsp:  External NMI is delivered to only CPU 0
> 
> 							only to

Thanks for tha correction.

> 
> > +			all:  External NMIs are broadcast to all CPUs as a
> > +			      backup of CPU 0
> > +			none: External NMI is masked for all CPUs. This is
> > +			      useful so that a dump capture kernel won't be
> > +			      shot down by NMI
> > +
> >  	autoconf=	[IPV6]
> >  			See Documentation/networking/ipv6.txt.
> >
> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > index 7f62ad4..c80f6b6 100644
> > --- a/arch/x86/include/asm/apic.h
> > +++ b/arch/x86/include/asm/apic.h
> > @@ -23,6 +23,11 @@
> >  #define APIC_VERBOSE 1
> >  #define APIC_DEBUG   2
> >
> > +/* Macros for apic_extnmi which controls external NMI masking */
> > +#define APIC_EXTNMI_BSP		0 /* Default */
> > +#define APIC_EXTNMI_ALL		1
> > +#define APIC_EXTNMI_NONE	2
> > +
> >  /*
> >   * Define the default level of output to be very little
> >   * This can be turned up by using apic=verbose for more
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 2f69e3b..a2a8074 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -82,6 +82,12 @@ physid_mask_t phys_cpu_present_map;
> >  static unsigned int disabled_cpu_apicid __read_mostly = BAD_APICID;
> >
> >  /*
> > + * This variable controls which CPUs receive external NMIs.  By default,
> > + * external NMIs are delivered to only BSP.
> 
> 				  only to the BSP.

...and again.

> 
> > + */
> > +static int apic_extnmi = APIC_EXTNMI_BSP;
> > +
> > +/*
> >   * Map cpu index to physical APIC ID
> >   */
> >  DEFINE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid, BAD_APICID);
> > @@ -1161,6 +1167,8 @@ void __init init_bsp_APIC(void)
> >  	value = APIC_DM_NMI;
> >  	if (!lapic_is_integrated())		/* 82489DX */
> >  		value |= APIC_LVT_LEVEL_TRIGGER;
> > +	if (apic_extnmi == APIC_EXTNMI_NONE)
> > +		value |= APIC_LVT_MASKED;
> >  	apic_write(APIC_LVT1, value);
> >  }
> >
> > @@ -1380,7 +1388,8 @@ void setup_local_APIC(void)
> >  	/*
> >  	 * only the BP should see the LINT1 NMI signal, obviously.
> >  	 */
> 
> That comment needs adjusting.

OK, I'll do that.

> 
> > -	if (!cpu)
> > +	if ((!cpu && apic_extnmi != APIC_EXTNMI_NONE) ||
> > +	    apic_extnmi == APIC_EXTNMI_ALL)
> >  		value = APIC_DM_NMI;
> >  	else
> >  		value = APIC_DM_NMI | APIC_LVT_MASKED;
> > @@ -2548,3 +2557,23 @@ static int __init apic_set_disabled_cpu_apicid(char *arg)
> >  	return 0;
> >  }
> >  early_param("disable_cpu_apicid", apic_set_disabled_cpu_apicid);
> > +
> > +static int __init apic_set_extnmi(char *arg)
> > +{
> > +	if (!arg)
> > +		return -EINVAL;
> > +
> > +	if (strcmp("all", arg) == 0)
> 
> 	if (!strncmp("all", arg, 3))
> 
> ditto for the rest

I'll fix them.

> > +		apic_extnmi = APIC_EXTNMI_ALL;
> > +	else if (strcmp("none", arg) == 0)
> > +		apic_extnmi = APIC_EXTNMI_NONE;
> > +	else if (strcmp("bsp", arg) == 0)
> > +		apic_extnmi = APIC_EXTNMI_BSP;
> > +	else {
> > +		pr_warn("Unknown external NMI delivery mode `%s' is ignored\n",
> 
> s/is //

I'll fix it, thanks.

> Also, if there's no other delivery mode which makes sense, you can do:
> 
> 	pr_warn("Unknown external NMI delivery mode `%s', defaulting to	'bsp'\n", arg);
> 	apic_extnmi = APIC_EXTNMI_BSP;

I intended to keep the previous or initial value of apic_extnmi
because boot option can be specified multiple times. If a user do
that, making the last valid value effective would be natural manner.
This is unclear part of boot option as Ingo pointed, but...

> Btw, you can let the pr_warn line be longer than 80 cols.

I like 80 cols because I'm working with multiple 80-col terminals. :-)

> And if you don't default, you need
> 
> 		return -EINVAL;
> 
> here.

You are right.  It seems that I deleted it while modifying
around the code.

Regards,
--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group