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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Mar 12, 2026 at 05:57:53PM +0100, Jan Beulich wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Add a new module parameter that allows adjusting the C-states table used by
> the driver.
> 
> Currently, the C-states table is hardcoded in the driver based on the CPU
> model. The goal is to have good enough defaults for most users.
> 
> However, C-state characteristics, such as exit latency and residency, can
> vary between different variants of the same CPU model and BIOS settings.
> Moreover, different platform usage models and user preferences may benefit
> from different C-state target_residency values.
> 
> Provide a way for users to adjust the C-states table via a module parameter
> "table". The general format is:
> "state1:latency1:target_residency1,state2:latency2:target_residency2,..."
> 
> In other words, represent each C-state by its name, exit latency (in
> microseconds), and target residency (in microseconds), separated by colons.
> Separate multiple C-states by commas.
> 
> For example, suppose a CPU has 3 C-states with the following
> characteristics:
>   C1:  exit_latency=1, target_residency=2
>   C1E: exit_latency=10, target_residency=10
>   C6:  exit_latency=100, target_residency=500
> 
> Users can specify a custom C-states table as follows:
> 
> 1. intel_idle.table="C1:2:2,C1E:5:20,C6:150:600"
>    Result: C1:  exit_latency=2, target_residency=2
>            C1E: exit_latency=5, target_residency=20
>            C6:  exit_latency=150, target_residency=600
> 2. intel_idle.table="C6::400"
>    Result: C1:  exit_latency=1, target_residency=2 (unchanged)
>            C1E: exit_latency=10, target_residency=10 (unchanged)
>            C6:  exit_latency=100, target_residency=400
>                 (only target_residency changed)
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Link: https://patch.msgid.link/20251216080402.156988-3-dedekind1@gmail.com
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 111f77a23348
> 
> Add __init to get_cmdline_field(). Put cmdline_table_str[] in .init.data.
> Other adjustments to fit our env.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> For the initial attempt, I've left the new option as a standalone one. It
> may be worth integrating with "mwait-idle", but I think much of the
> parsing would then want doing differently. It'll then likely be much
> harder to apply future Linux changes there.
> 
> --- 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?

> +> `= <string>`
> +
> + Define the C-states table from a user input string. Expected format is
> + 'name:latency:residency', where:
> + - name: The C-state name.
> + - latency: The C-state exit latency in us.
> + - residency: The C-state target residency in us.
> +
> + Multiple C-states can be defined by separating them with commas:
> + 'name1:latency1:residency1,name2:latency2:residency2'
> +
> + Example: intel_idle.table=C1:1:1,C1E:5:10,C6:100:600

s/intel_idle/mwait-idle/ in the line above.

> +
> + To leave latency or residency unchanged, use an empty field, for example:
> + 'C1:1:1,C1E::10' - leaves C1E latency unchanged.
> +
>  ### nmi (x86)
>  > `= ignore | dom0 | fatal`
>  
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -70,6 +70,11 @@
>  static __initdata bool opt_mwait_idle = true;
>  boolean_param("mwait-idle", opt_mwait_idle);
>  
> +/* The maximum allowed length for the 'table' module parameter  */
> +#define MAX_CMDLINE_TABLE_LEN 256
> +static char cmdline_table_str[MAX_CMDLINE_TABLE_LEN] __initdata;
> +string_param("mwait-idle.table", cmdline_table_str);
> +
>  static unsigned int mwait_substates;
>  
>  #define LAPIC_TIMER_ALWAYS_RELIABLE 0xFFFFFFFF
> @@ -122,6 +127,9 @@ struct cpuidle_state {
>   */
>  #define CPUIDLE_FLAG_IBRS		0x20000
>  
> +/* C-states data from the 'mwait-idle.table' cmdline parameter */
> +static struct cpuidle_state cmdline_states[ACPI_PROCESSOR_MAX_POWER] __initdata;
> +
>  /*
>   * MWAIT takes an 8-bit "hint" in EAX "suggesting"
>   * the C-state (top nibble) and sub-state (bottom nibble)
> @@ -1546,6 +1554,161 @@ static void __init mwait_idle_state_tabl
>  	}
>  }
>  
> + /**
> +  * get_cmdline_field - Get the current field from a cmdline string.
> +  * @args: The cmdline string to get the current field from.
> +  * @field: Pointer to the current field upon return.
> +  * @sep: The fields separator character.
> +  *
> +  * Examples:
> +  *   Input: args="C1:1:1,C1E:2:10", sep=':'
> +  *   Output: field="C1", return "1:1,C1E:2:10"
> +  *   Input: args="C1:1:1,C1E:2:10", sep=','
> +  *   Output: field="C1:1:1", return "C1E:2:10"
> +  *   Ipnut: args="::", sep=':'
> +  *   Output: field="", return ":"
> +  *
> +  * Return: The continuation of the cmdline string after the field or NULL.
> +  */
> +static char *__init get_cmdline_field(char *args, char **field, char sep)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; args[i] && !isspace(args[i]); i++) {
> +		if (args[i] == sep)
> +			break;
> +	}
> +
> +	*field = args;
> +
> +	if (args[i] != sep)
> +		return NULL;
> +
> +	args[i] = '\0';
> +	return args + i + 1;
> +}
> +
> +/**
> + * cmdline_table_adjust - Adjust the C-states table with data from cmdline.
> + *
> + * Adjust the C-states table with data from the 'mwait-idle.table' parameter
> + * (if specified).
> + */
> +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.

> +
> +		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?

Thanks, Roger.