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

[Jan Beulich <jbeulich@xxxxxxxx>]

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