Message-ID: <ZQIka+OM1+2M3CsU@swahl-linux>
Date:   Wed, 13 Sep 2023 16:06:51 -0500
From:   Steve Wahl <steve.wahl@....com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Steve Wahl <steve.wahl@....com>,
        Justin Ernst <justin.ernst@....com>,
        Kyle Meyer <kyle.meyer@....com>,
        Dimitri Sivanich <dimitri.sivanich@....com>,
        Russ Anderson <russ.anderson@....com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, Justin Stitt <justinstitt@...gle.com>
Subject: Re: [PATCH v2] x86/platform/uv: Rework NMI "action" modparam handling

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@....com>
> Cc: Justin Stitt <justinstitt@...gle.com>
> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> ---
> 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.

Reveiwed-by: Steve Wahl <steve.wahl@....com>
Tested-by: Steve Wahl <steve.wahl@....com>



>  	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

Powered by blists - more mailing lists