Re: [PATCH] x86/cpuidle: split the max_cstate variable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2026 18:14, Roger Pau Monné wrote:
> On Wed, Apr 08, 2026 at 01:34:43PM +0200, Jan Beulich wrote:
>> @@ -690,18 +694,18 @@ static void cf_check acpi_processor_idle
>>      u32 exp = 0, pred = 0;
>>      u32 irq_traced[4] = { 0 };
>>  
>> -    if ( max_cstate > 0 && power &&
>> +    if ( max_cstate() > 0 && power &&
>>           (next_state = cpuidle_current_governor->select(power)) > 0 )
>>      {
>>          unsigned int max_state = sched_has_urgent_vcpu() ? ACPI_STATE_C1
>> -                                                         : max_cstate;
>> +                                                         : max_cstate();
>>  
>>          do {
>>              cx = &power->states[next_state];
>>          } while ( (cx->type > max_state ||
>>                     cx->entry_method == ACPI_CSTATE_EM_NONE ||
>>                     (cx->entry_method == ACPI_CSTATE_EM_FFH &&
>> -                    cx->type == max_cstate &&
>> +                    cx->type == max_allowed_cstate &&
> 
> I'm afraid I'm missing why this uses max_allowed_cstate instead of
> max_state.

max_allowed_cstate is what needs using along with ...

>>                      (cx->address & MWAIT_SUBSTATE_MASK) > max_csubstate)) &&

... max_csubstate, as both are driven by the "max_cstate=" command line
option. Renaming max_csubstate to max_allowed_csubstate would be an
option, but would incure yet more churn.

>> --- a/xen/arch/x86/cpu/mwait-idle.c
>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>> @@ -1045,15 +1045,16 @@ static void cf_check mwait_idle(void)
>>      u64 before, after;
>>      u32 exp = 0, pred = 0, irq_traced[4] = { 0 };
>>  
>> -    if (max_cstate > 0 && power &&
>> +    if (max_cstate() > 0 && power &&
>>          (next_state = cpuidle_current_governor->select(power)) > 0) {
>>              unsigned int max_state = sched_has_urgent_vcpu() ? ACPI_STATE_C1
>> -                                                             : max_cstate;
>> +                                                             : max_cstate();
>>  
>>              do {
>>                      cx = &power->states[next_state];
>> -            } while ((cx->type > max_state || (cx->type == max_cstate &&
>> -                      MWAIT_HINT2SUBSTATE(cx->address) > max_csubstate)) &&
>> +            } while ((cx->type > max_state ||
>> +                          (cx->type == max_allowed_cstate &&
> 
> Indentation is weird for the above line IMO, you should use hard 3
> tabs plus spaces afterwards, like the surrounding indentation?

Ouch, indeed.

>> +                       MWAIT_HINT2SUBSTATE(cx->address) > max_csubstate)) &&
>>                       --next_state);
>>              if (!next_state)
>>                      cx = NULL;
> 
> Seeing max_cstate() is used in multiple places here, you might want to
> introduce a local max_cstate variable?

Except that Misra doesn't like such naming, and any other name would feel
odd to use.

>> --- a/xen/include/xen/acpi.h
>> +++ b/xen/include/xen/acpi.h
>> @@ -142,30 +142,33 @@ int acpi_gsi_to_irq (u32 gsi, unsigned i
>>  
>>  #ifdef      CONFIG_ACPI_CSTATE
>>  /*
>> - * max_cstate sets the highest legal C-state.
>> - * max_cstate = 0: C0 okay, but not C1
>> - * max_cstate = 1: C1 okay, but not C2
>> - * max_cstate = 2: C2 okay, but not C3 etc.
>> -
>> - * max_csubstate sets the highest legal C-state sub-state. Only applies to 
>> the
>> - * highest legal C-state.
>> - * max_cstate = 1, max_csubstate = 0 ==> C0, C1 okay, but not C1E
>> - * max_cstate = 1, max_csubstate = 1 ==> C0, C1 and C1E okay, but not C2
>> - * max_cstate = 2, max_csubstate = 0 ==> C0, C1, C1E, C2 okay, but not C3
>> - * max_cstate = 2, max_csubstate = 1 ==> C0, C1, C1E, C2 okay, but not C3
>> + * max_{allowed,usable}_cstate sets the highest allowed / usable C-state, 
>> where
>> + * "allowed" is command line / sysctl based.
> 
> Hm, this is a bit misleading, because max_usable_cstate is also
> command line based (plus system errata).  What about:
> 
> "max_{allowed,usable}_cstate sets the highest allowed / usable C-state.
> max_usable_cstate can only be set from the command line, while
> max_allowed_cstate can be set from both command line and systcl."

Well. While I think I get your point, what I'm trying to get across is that
max_usable_cstate is internally controlled (bounded by command line setting
of max_allowed_cstate, but possibly forced lower than that internally). So
maybe

"max_{allowed,usable}_cstate sets the highest allowed / usable C-state.
 max_usable_cstate, while affected by the command line, is internally driven,
 whereas max_allowed_cstate can be set from both command line and systcl."

?

>> + * max_*_cstate = 0: C0 okay, but not C1
>> + * max_*_cstate = 1: C1 okay, but not C2
>> + * max_*_cstate = 2: C2 okay, but not C3 etc.
>> + *
>> + * max_csubstate sets the highest allowed C-state sub-state. Only applies to
>> + * the highest allowed C-state.
>> + * max_allowed_cstate = 1, max_csubstate = 0 ==> C0, C1 okay, but not C1E
>> + * max_allowed_cstate = 1, max_csubstate = 1 ==> C0, C1 and C1E okay, but 
>> not C2
>> + * max_allowed_cstate = 2, max_csubstate = 0 ==> C0, C1, C1E, C2 okay, but 
>> not C3
>> + * max_allowed_cstate = 2, max_csubstate = 1 ==> C0, C1, C1E, C2 okay, but 
>> not C3
>>   */
>>  
>> -extern unsigned int max_cstate;
>> +extern unsigned int max_usable_cstate;
>> +extern unsigned int max_allowed_cstate;
>>  extern unsigned int max_csubstate;
>>  
>> +#define max_cstate() min(max_usable_cstate, max_allowed_cstate)
> 
> I would be tempted to drop the ending parenthesis so that you don't
> need to adjust callers, but that's likely misleading, as then it would
> need to be uppercase MAX_CSTATE.

I deliberately want to have the parentheses, to make sure all uses of
max_cstate (without the parentheses) have been covered (by converting in
whatever appropriate way). Which extends to possible backports. In a
subsequent, not to be backported commit we could drop them again if so
desired.

>>  static inline unsigned int acpi_get_cstate_limit(void)
>>  {
>> -    return max_cstate;
>> +    return max_allowed_cstate;
>>  }
>>  static inline void acpi_set_cstate_limit(unsigned int new_limit)
>>  {
>> -    max_cstate = new_limit;
>> -    return;
>> +    max_allowed_cstate = new_limit;
> 
> Do we want to check the new limit doesn't exceed max_usable_cstate and
> return -ERANGE or similar on failure?
> 
> After this change it's a bit weird to silently ignore invalid values
> IMO.

I disagree. Those values may be valid, just not usable (i.e. they are
still a valid upper bound, but we'd never go as high up). If people wanted
to use the same settings across their fleet, undue (and confusing) errors
might result on some of their systems if we did as you suggest. Plus we
have always accepted arbitrarily large (and hence entirely meaningless)
values anyway.

Jan