Re: [PATCH v2] x86/platform/uv: Rework NMI "action" modparam handling

[Steve Wahl]

On Wed, Sep 13, 2023 at 04:06:51PM -0500, Steve Wahl wrote:
> On Wed, Sep 13, 2023 at 08:01:11PM +0200, Hans de Goede wrote:
> > Rework NMI "action" modparam handling:
> > 
> > 1. Replace the uv_nmi_action string with an enum; and
> > 2. Use sysfs_match_string() for string parsing in param_set_action()
> > 
> > Suggested-by: Steve Wahl <steve.wahl@xxxxxxx>
> > Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> > Changes in v2:
> > - Also change uv_nmi_action to an enum to replace a bunch of
> >   strcmp() calls
> > ---
> >  arch/x86/platform/uv/uv_nmi.c | 104 +++++++++++++++++++---------------
> >  1 file changed, 57 insertions(+), 47 deletions(-)
> > 
> > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> > index 45d0c17ce77c..b92f1b4adeb0 100644
> > --- a/arch/x86/platform/uv/uv_nmi.c
> > +++ b/arch/x86/platform/uv/uv_nmi.c
> > @@ -178,49 +178,57 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
> >  	} while (0)
> >  
> >  /* Valid NMI Actions */
> > -#define	ACTION_LEN	16
> > -static struct nmi_action {
> > -	char	*action;
> > -	char	*desc;
> > -} valid_acts[] = {
> > -	{	"kdump",	"do kernel crash dump"			},
> > -	{	"dump",		"dump process stack for each cpu"	},
> > -	{	"ips",		"dump Inst Ptr info for each cpu"	},
> > -	{	"kdb",		"enter KDB (needs kgdboc= assignment)"	},
> > -	{	"kgdb",		"enter KGDB (needs gdb target remote)"	},
> > -	{	"health",	"check if CPUs respond to NMI"		},
> > +enum action_t {
> > +	nmi_act_kdump,
> > +	nmi_act_dump,
> > +	nmi_act_ips,
> > +	nmi_act_kdb,
> > +	nmi_act_kgdb,
> > +	nmi_act_health,
> >  };
> > -typedef char action_t[ACTION_LEN];
> > -static action_t uv_nmi_action = { "dump" };
> > +
> > +static const char * const actions[] = {
> > +	[nmi_act_kdump] = "kdump",
> > +	[nmi_act_dump] = "dump",
> > +	[nmi_act_ips] = "ips",
> > +	[nmi_act_kdb] = "kdb",
> > +	[nmi_act_kgdb] = "kgdb",
> > +	[nmi_act_health] = "health",
> > +};
> > +
> > +static const char * const actions_desc[] = {
> > +	[nmi_act_kdump] = "do kernel crash dump",
> > +	[nmi_act_dump] = "dump process stack for each cpu",
> > +	[nmi_act_ips] = "dump Inst Ptr info for each cpu",
> > +	[nmi_act_kdb] = "enter KDB (needs kgdboc= assignment)",
> > +	[nmi_act_kgdb] = "enter KGDB (needs gdb target remote)",
> > +	[nmi_act_health] = "check if CPUs respond to NMI",
> > +};
> > +
> > +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
> > +
> > +static enum action_t uv_nmi_action = nmi_act_dump;
> >  
> >  static int param_get_action(char *buffer, const struct kernel_param *kp)
> >  {
> > -	return sprintf(buffer, "%s\n", uv_nmi_action);
> > +	return sprintf(buffer, "%s\n", actions[uv_nmi_action]);
> >  }
> >  
> >  static int param_set_action(const char *val, const struct kernel_param *kp)
> >  {
> > -	int i;
> > -	int n = ARRAY_SIZE(valid_acts);
> > -	char arg[ACTION_LEN];
> > +	int i, n = ARRAY_SIZE(actions);
> >  
> > -	/* (remove possible '\n') */
> > -	strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
> > -
> > -	for (i = 0; i < n; i++)
> > -		if (!strcmp(arg, valid_acts[i].action))
> > -			break;
> > -
> > -	if (i < n) {
> > -		strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action));
> > -		pr_info("UV: New NMI action:%s\n", uv_nmi_action);
> > +	i = sysfs_match_string(actions, val);
> > +	if (i >= 0) {
> > +		uv_nmi_action = i;
> > +		pr_info("UV: New NMI action:%s\n", actions[i]);
> >  		return 0;
> >  	}
> >  
> > -	pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> > +	pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
> 
> This is a very minor nit in an otherwise fine patch:
> 
> Testing by echoing to /sys/module/uv_nmi/parameters/action shows an
> invalid action in val has a trailing newline that appears just before
> the comma:
> 
> # echo "invalid" >/sys/module/uv_nmi/parameters/action
> [ 1070.079303] UV: Invalid NMI action:invalid
> [ 1070.079303] , valid actions are:
> [ 1070.087485] UV: kdump    - do kernel crash dump
> [ 1070.092558] UV: dump     - dump process stack for each cpu
> [ 1070.098694] UV: ips      - dump Inst Ptr info for each cpu
> [ 1070.098697] UV: kdb      - enter KDB (needs kgdboc= assignment)
> [ 1070.098699] UV: kgdb     - enter KGDB (needs gdb target remote)
> [ 1070.098702] UV: health   - check if CPUs respond to NMI
> -bash: echo: write error: Invalid argument
> #
> 
> There's no newline in val if it comes from the kernel command line, so
> you can't just assume it's there.
> 
> It would be bad style to just overwrite the newline in place.
> Allocating space for and copying a string seems a waste.  Maybe rework
> the message so a possible newline doesn't look so awkward, by removing
> the comma?
> 
> > +	pr_err("UV: Invalid NMI action:%s Valid actions are:\n", val);
> 
> Frankly, I approve of this patch going in, regardless of what, if
> anything, is done about this.
> 
> Thanks.

I forgot to mention that testing included setting all option values from
kernel command line or /sys file, and sending NMIs to see that they
triggered the action that was set.

--> Steve

> Reveiwed-by: Steve Wahl <steve.wahl@xxxxxxx>
> Tested-by: Steve Wahl <steve.wahl@xxxxxxx>
> 
> 
> 
> >  	for (i = 0; i < n; i++)
> > -		pr_err("UV: %-8s - %s\n",
> > -			valid_acts[i].action, valid_acts[i].desc);
> > +		pr_err("UV: %-8s - %s\n", actions[i], actions_desc[i]);
> > +
> >  	return -EINVAL;
> >  }
> >  
> > @@ -228,15 +236,10 @@ static const struct kernel_param_ops param_ops_action = {
> >  	.get = param_get_action,
> >  	.set = param_set_action,
> >  };
> > -#define param_check_action(name, p) __param_check(name, p, action_t)
> > +#define param_check_action(name, p) __param_check(name, p, enum action_t)
> >  
> >  module_param_named(action, uv_nmi_action, action, 0644);
> >  
> > -static inline bool uv_nmi_action_is(const char *action)
> > -{
> > -	return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
> > -}
> > -
> >  /* Setup which NMI support is present in system */
> >  static void uv_nmi_setup_mmrs(void)
> >  {
> > @@ -727,10 +730,10 @@ static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> >  	if (cpu == 0)
> >  		uv_nmi_dump_cpu_ip_hdr();
> >  
> > -	if (current->pid != 0 || !uv_nmi_action_is("ips"))
> > +	if (current->pid != 0 || uv_nmi_action != nmi_act_ips)
> >  		uv_nmi_dump_cpu_ip(cpu, regs);
> >  
> > -	if (uv_nmi_action_is("dump")) {
> > +	if (uv_nmi_action == nmi_act_dump) {
> >  		pr_info("UV:%sNMI process trace for CPU %d\n", dots, cpu);
> >  		show_regs(regs);
> >  	}
> > @@ -798,7 +801,7 @@ static void uv_nmi_dump_state(int cpu, struct pt_regs *regs, int master)
> >  		int saved_console_loglevel = console_loglevel;
> >  
> >  		pr_alert("UV: tracing %s for %d CPUs from CPU %d\n",
> > -			uv_nmi_action_is("ips") ? "IPs" : "processes",
> > +			uv_nmi_action == nmi_act_ips ? "IPs" : "processes",
> >  			atomic_read(&uv_nmi_cpus_in_nmi), cpu);
> >  
> >  		console_loglevel = uv_nmi_loglevel;
> > @@ -874,7 +877,7 @@ static inline int uv_nmi_kdb_reason(void)
> >  static inline int uv_nmi_kdb_reason(void)
> >  {
> >  	/* Ensure user is expecting to attach gdb remote */
> > -	if (uv_nmi_action_is("kgdb"))
> > +	if (uv_nmi_action == nmi_act_kgdb)
> >  		return 0;
> >  
> >  	pr_err("UV: NMI error: KDB is not enabled in this kernel\n");
> > @@ -950,28 +953,35 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
> >  	master = (atomic_read(&uv_nmi_cpu) == cpu);
> >  
> >  	/* If NMI action is "kdump", then attempt to do it */
> > -	if (uv_nmi_action_is("kdump")) {
> > +	if (uv_nmi_action == nmi_act_kdump) {
> >  		uv_nmi_kdump(cpu, master, regs);
> >  
> >  		/* Unexpected return, revert action to "dump" */
> >  		if (master)
> > -			strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action));
> > +			uv_nmi_action = nmi_act_dump;
> >  	}
> >  
> >  	/* Pause as all CPU's enter the NMI handler */
> >  	uv_nmi_wait(master);
> >  
> >  	/* Process actions other than "kdump": */
> > -	if (uv_nmi_action_is("health")) {
> > +	switch (uv_nmi_action) {
> > +	case nmi_act_health:
> >  		uv_nmi_action_health(cpu, regs, master);
> > -	} else if (uv_nmi_action_is("ips") || uv_nmi_action_is("dump")) {
> > +		break;
> > +	case nmi_act_ips:
> > +	case nmi_act_dump:
> >  		uv_nmi_dump_state(cpu, regs, master);
> > -	} else if (uv_nmi_action_is("kdb") || uv_nmi_action_is("kgdb")) {
> > +		break;
> > +	case nmi_act_kdb:
> > +	case nmi_act_kgdb:
> >  		uv_call_kgdb_kdb(cpu, regs, master);
> > -	} else {
> > +		break;
> > +	default:
> >  		if (master)
> > -			pr_alert("UV: unknown NMI action: %s\n", uv_nmi_action);
> > +			pr_alert("UV: unknown NMI action: %d\n", uv_nmi_action);
> >  		uv_nmi_sync_exit(master);
> > +		break;
> >  	}
> >  
> >  	/* Clear per_cpu "in_nmi" flag */
> > -- 
> > 2.41.0
> > 
> 
> -- 
> Steve Wahl, Hewlett Packard Enterprise

-- 
Steve Wahl, Hewlett Packard Enterprise