Re: [PATCH 8/9] x86/mwait-idle: Add cmdline option to adjust C-states table

[Jan Beulich <jbeulich@suse.com>]

On 24.04.2026 21:10, Roger Pau Monné wrote:
> On Thu, Mar 12, 2026 at 05:57:53PM +0100, Jan Beulich wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1928,6 +1928,23 @@ Print boot time MTRR state.
>>  Use the MWAIT idle driver (with model specific C-state knowledge) instead
>>  of the ACPI based one.
>>  
>> +### mwait-idle.table (x86)
> 
> The .table suffix is kind of weird, as we don't use it elsewhere.  It
> might feel more natural given the naming of the existing command line
> options to use mwait-idle-table?

I thought it might be better to follow the Linux way of naming per-module
sub-options, when we "inherit" them. See also "mtrr.show".

>> +static void __init cmdline_table_adjust(void)
>> +{
>> +	char *args = cmdline_table_str;
>> +	struct cpuidle_state *state;
>> +	unsigned int i, state_count;
>> +
>> +	if (args[0] == '\0')
>> +		/* The 'mwait-idle.table' module parameter was not specified */
>> +		return;
>> +
>> +	/* Create a copy of the C-states table */
>> +	for (i = 0;
>> +	     i < ARRAY_SIZE(cmdline_states) && icpu.state_table[i].name[0];
>> +	     i++)
>> +		cmdline_states[i] = icpu.state_table[i];
>> +
>> +	state_count = i;
>> +
>> +	/*
>> +	 * Adjust the C-states table copy with data from the 'mwait-idle.table'
>> +	 * module parameter.
>> +	 */
>> +	while (args) {
>> +		char *fields, *name, *val;
>> +
>> +		/*
>> +		 * Get the next C-state definition, which is expected to be
>> +		 * '<name>:<latency_us>:<target_residency_us>'. Treat "empty"
>> +		 * fields as unchanged. For example,
>> +		 * '<name>::<target_residency_us>' leaves the latency unchanged.
>> +		 */
>> +		args = get_cmdline_field(args, &fields, ',');
>> +
>> +		/* name */
>> +		fields = get_cmdline_field(fields, &name, ':');
>> +		if (!fields)
>> +			goto error;
>> +
>> +		/* Find the C-state by its name */
>> +		state = NULL;
>> +		for (i = 0; i < state_count; i++) {
>> +			if (!strcmp(name, cmdline_states[i].name)) {
>> +				state = &cmdline_states[i];
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!state) {
>> +			printk(XENLOG_ERR PREFIX "C-state '%s' was not found\n",
>> +			       name);
>> +			continue;
>> +		}
>> +
>> +		/* Latency */
>> +		fields = get_cmdline_field(fields, &val, ':');
>> +		if (!fields)
>> +			goto error;
>> +
>> +		if (*val) {
>> +			const char *end;
>> +			unsigned long n = simple_strtoul(val, &end, 0);
>> +
>> +			state->exit_latency = n;
>> +			if (*end || state->exit_latency != n)
>> +				goto error;
>> +		}
>> +
>> +		/* Target residency */
>> +		fields = get_cmdline_field(fields, &val, ':');
>> +
>> +		if (*val) {
>> +			const char *end;
>> +			unsigned long n = simple_strtoul(val, &end, 0);
>> +
>> +			state->target_residency = n;
>> +			if (*end || state->target_residency != n)
>> +				goto error;
>> +		}
>> +
>> +		/*
>> +		 * Allow for 3 more fields, but ignore them. Helps to make
>> +		 * possible future extensions of the cmdline format backward
>> +		 * compatible.
>> +		 */
>> +		for (i = 0; fields && i < 3; i++) {
>> +			fields = get_cmdline_field(fields, &val, ':');
>> +			if (!fields)
>> +				break;
>> +		}
> 
> This seems a bit arbitrary for my taste.  I would rather ignore the
> extra fields (and print a message about it), and proceed with the next
> state.

I think we want to stick to the original Linux behavior here. If you
think that wants adjustment, imo doing so would call for going through
Linux first.

>> +
>> +		if (fields) {
>> +			printk(XENLOG_ERR PREFIX
>> +			       "Too many fields for C-state '%s'\n",
>> +			       state->name);
>> +			goto error;
>> +		}
>> +
>> +		printk(XENLOG_INFO PREFIX
>> +		       "C-state from cmdline: name=%s, latency=%u, residency=%u\n",
>> +		       state->name, state->exit_latency, state->target_residency);
>> +	}
>> +
>> +	/* Copy the adjusted C-states table back */
>> +	for (i = 0; i < state_count; i++)
>> +		icpu.state_table[i] = cmdline_states[i];
>> +
>> +	printk(XENLOG_INFO PREFIX
>> +	       "Adjusted C-states with data from 'mwait-idle.table'\n");
>> +	return;
>> +
>> + error:
>> +	printk(PREFIX
> 
> XENLOG_ERR maybe?

It's pr_info() in the Linux original, so ERR would feel like going too
far. And WARNING is the default anyway. I can switch to XENLOG_WARNING
if you think we want to make this explicit.

Jan