Re: [PATCH 2/4] x86/mwait_idle: Allow limiting the sub C-state

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/06/14 16:04, Ross Lagerwall wrote:
> Allow limiting the sub C-state using a new command-line parameter, max_substate.
> The limit only applies to the highest legal C-state. For example:
>  max_cstate = 1, max_substate = 0 ==> C0, C1 okay, but not C1E
>  max_cstate = 1, max_substate = 1 ==> C0, C1 and C1E okay, but not C2
>  max_cstate = 2, max_substate = 0 ==> C0, C1, C1E, C2 okay, but not C3
>  max_cstate = 2, max_substate = 1 ==> C0, C1, C1E, C2 okay, but not C3
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

At the very least, please patch docs/misc/xen-command-line.markdown

However, the name "max_substate" is far too generic on its own.

"max_csubstate" would be better, but still not as easily identifiable on
its own as "max_cstate".

> ---
>  xen/arch/x86/acpi/cpu_idle.c  | 3 +++
>  xen/arch/x86/cpu/mwait-idle.c | 4 +++-
>  xen/include/xen/acpi.h        | 2 ++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index b05fb39..96909b3 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -106,6 +106,8 @@ static uint64_t (*__read_mostly tick_to_ns)(uint64_t) = acpi_pm_tick_to_ns;
>  void (*__read_mostly pm_idle_save)(void);
>  unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
>  integer_param("max_cstate", max_cstate);
> +unsigned int max_substate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
> +integer_param("max_substate", max_substate);
>  static bool_t __read_mostly local_apic_timer_c2_ok;
>  boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
>  
> @@ -240,6 +242,7 @@ static void print_acpi_power(uint32_t cpu, struct acpi_processor_power *power)
>      last_state_idx = power->last_state ? power->last_state->idx : -1;
>      printk("active state:\t\tC%d\n", last_state_idx);
>      printk("max_cstate:\t\tC%d\n", max_cstate);
> +    printk("max_substate:\t\t%d\n", max_substate);

%u

The preceding line is also wrong with its formatting parameter.

>      printk("states:\n");
>      
>      for ( i = 1; i < power->count; i++ )
> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
> index 38172e5..90c8aea 100644
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -330,7 +330,9 @@ static void mwait_idle(void)
>  	    (next_state = cpuidle_current_governor->select(power)) > 0) {
>  		do {
>  			cx = &power->states[next_state];
> -		} while (cx->type > max_cstate && --next_state);
> +		} while ((cx->type > max_cstate || (cx->type == max_cstate &&
> +			  MWAIT_HINT2SUBSTATE(cx->address) > max_substate)) &&
> +			 --next_state);
>  		if (!next_state)
>  			cx = NULL;
>  		menu_get_trace_data(&exp, &pred);
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index aedec65..b49c4fc 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -140,6 +140,8 @@ static inline void acpi_set_cstate_limit(unsigned int new_limit)
>  	max_cstate = new_limit;
>  	return;
>  }
> +
> +extern unsigned int max_substate;

This should live next to the extern for max_cstate.

~Andrew

>  #else
>  static inline unsigned int acpi_get_cstate_limit(void) { return 0; }
>  static inline void acpi_set_cstate_limit(unsigned int new_limit) { return; }