diff mbox series

[4/7] intel_idle: improve C-state flags handling robustness

Message ID 20230419143947.1342730-5-dedekind1@gmail.com (mailing list archive)
State Mainlined, archived
Headers show
Series misc intel_idle cleanups | expand

Commit Message

Artem Bityutskiy April 19, 2023, 2:39 p.m. UTC 
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

The following C-state flags are currently mutually-exclusive and should not
be combined:
  * IRQ_ENABLE
  * IBRS
  * XSTATE

There is a warning for the situation when the IRQ_ENABLE flag
is combined with the IBRS flag, but no warnings for other combinations.
This is inconsistent and prone to errors.

Improve the situation by adding warnings for all the unexpected
combinations. Add a couple of helpful commentaries too.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/idle/intel_idle.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

Zhang Rui April 20, 2023, 1:50 a.m. UTC | #1 
On Wed, 2023-04-19 at 17:39 +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> The following C-state flags are currently mutually-exclusive and
> should not
> be combined:
>   * IRQ_ENABLE
>   * IBRS
>   * XSTATE
> 
> There is a warning for the situation when the IRQ_ENABLE flag
> is combined with the IBRS flag, but no warnings for other
> combinations.
> This is inconsistent and prone to errors.
> 
> Improve the situation by adding warnings for all the unexpected
> combinations. Add a couple of helpful commentaries too.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  drivers/idle/intel_idle.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 73ddb1d8cfcf..1de36df15d5a 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -1896,20 +1896,28 @@ static void __init
> intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
>  		drv->states[drv->state_count] =
> cpuidle_state_table[cstate];
>  		state = &drv->states[drv->state_count];
>  
> -		if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) ||
> force_irq_on) {
> -			pr_info("forced intel_idle_irq for state %d\n",
> cstate);
> -			state->enter = intel_idle_irq;
> -		}
> -
> -		if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) &&
> -		    state->flags & CPUIDLE_FLAG_IBRS) {
> +		if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {
> +			/*
> +			 * Combining with XSTATE with IBRS or
> IRQ_ENABLE flags
> +			 * is not currently supported but this driver.
> +			 */
> +			WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IBRS);
> +			WARN_ON_ONCE(state->flags &
> CPUIDLE_FLAG_IRQ_ENABLE);
> +			state->enter = intel_idle_xstate;
> +		} else if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS)
> &&
> +			   state->flags & CPUIDLE_FLAG_IBRS) {
> +			/*
> +			 * IBRS mitigation requires that C-states are
> entered
> +			 * with interrupts disabled.
> +			 */
>  			WARN_ON_ONCE(state->flags &
> CPUIDLE_FLAG_IRQ_ENABLE);
>  			state->enter = intel_idle_ibrs;
> +		} else if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) ||
> +			   force_irq_on) {
> +			pr_info("forced intel_idle_irq for state %d\n",
> cstate);
> +			state->enter = intel_idle_irq;
>  		}

what is the expected behavior for a state with CPUIDLE_FLAG_IBRS or
CPUIDLE_FLAG_INIT_XSTATE set when using "force_irq_on"?

the CPUIDLE_FLAG_IBRS/CPUIDLE_FLAG_INIT_XSTATE flag always wins, right?
I think it would be good to add a comment about this, say in patch 6/7.

thanks,
rui

>  
> -		if (state->flags & CPUIDLE_FLAG_INIT_XSTATE)
> -			state->enter = intel_idle_xstate;
> -
>  		if ((disabled_states_mask & BIT(drv->state_count)) ||
>  		    ((icpu->use_acpi || force_use_acpi) &&
>  		     intel_idle_off_by_default(mwait_hint) &&
Artem Bityutskiy April 20, 2023, 6:18 a.m. UTC | #2 
Hi Rui,

On Thu, 2023-04-20 at 01:50 +0000, Zhang, Rui wrote:
> what is the expected behavior for a state with CPUIDLE_FLAG_IBRS or
> CPUIDLE_FLAG_INIT_XSTATE set when using "force_irq_on"?
> the CPUIDLE_FLAG_IBRS/CPUIDLE_FLAG_INIT_XSTATE flag always wins, right?

Yes.

> I think it would be good to add a comment about this, say in patch 6/7.

Sure, will do.

Thanks!
Zhang Rui April 20, 2023, 8:22 a.m. UTC | #3 
On Wed, 2023-04-19 at 17:39 +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> The following C-state flags are currently mutually-exclusive and
> should not
> be combined:
>   * IRQ_ENABLE
>   * IBRS
>   * XSTATE
> 
> There is a warning for the situation when the IRQ_ENABLE flag
> is combined with the IBRS flag, but no warnings for other
> combinations.
> This is inconsistent and prone to errors.
> 
> Improve the situation by adding warnings for all the unexpected
> combinations. Add a couple of helpful commentaries too.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  drivers/idle/intel_idle.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 73ddb1d8cfcf..1de36df15d5a 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -1896,20 +1896,28 @@ static void __init
> intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
>  		drv->states[drv->state_count] =
> cpuidle_state_table[cstate];
>  		state = &drv->states[drv->state_count];
>  
> -		if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) ||
> force_irq_on) {
> -			pr_info("forced intel_idle_irq for state %d\n",
> cstate);
> -			state->enter = intel_idle_irq;
> -		}
> -
> -		if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) &&
> -		    state->flags & CPUIDLE_FLAG_IBRS) {
> +		if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {
> +			/*
> +			 * Combining with XSTATE with IBRS or
> IRQ_ENABLE flags
> +			 * is not currently supported but this driver.
> +			 */
> +			WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IBRS);
> +			WARN_ON_ONCE(state->flags &
> CPUIDLE_FLAG_IRQ_ENABLE);
> +			state->enter = intel_idle_xstate;
> +		} else if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS)
> &&
> +			   state->flags & CPUIDLE_FLAG_IBRS) {
> +			/*
> +			 * IBRS mitigation requires that C-states are
> entered
> +			 * with interrupts disabled.
> +			 */
>  			WARN_ON_ONCE(state->flags &
> CPUIDLE_FLAG_IRQ_ENABLE);
>  			state->enter = intel_idle_ibrs;
> +		} else if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) ||
> +			   force_irq_on) {
> +			pr_info("forced intel_idle_irq for state %d\n",
> cstate);
> +			state->enter = intel_idle_irq;
>  		}
>  
> -		if (state->flags & CPUIDLE_FLAG_INIT_XSTATE)
> -			state->enter = intel_idle_xstate;
> -
>  		if ((disabled_states_mask & BIT(drv->state_count)) ||
>  		    ((icpu->use_acpi || force_use_acpi) &&
>  		     intel_idle_off_by_default(mwait_hint) &&
diff mbox series

Patch

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 73ddb1d8cfcf..1de36df15d5a 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1896,20 +1896,28 @@  static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 		drv->states[drv->state_count] = cpuidle_state_table[cstate];
 		state = &drv->states[drv->state_count];
 
-		if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) {
-			pr_info("forced intel_idle_irq for state %d\n", cstate);
-			state->enter = intel_idle_irq;
-		}
-
-		if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) &&
-		    state->flags & CPUIDLE_FLAG_IBRS) {
+		if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {
+			/*
+			 * Combining with XSTATE with IBRS or IRQ_ENABLE flags
+			 * is not currently supported but this driver.
+			 */
+			WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IBRS);
+			WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IRQ_ENABLE);
+			state->enter = intel_idle_xstate;
+		} else if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) &&
+			   state->flags & CPUIDLE_FLAG_IBRS) {
+			/*
+			 * IBRS mitigation requires that C-states are entered
+			 * with interrupts disabled.
+			 */
 			WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IRQ_ENABLE);
 			state->enter = intel_idle_ibrs;
+		} else if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) ||
+			   force_irq_on) {
+			pr_info("forced intel_idle_irq for state %d\n", cstate);
+			state->enter = intel_idle_irq;
 		}
 
-		if (state->flags & CPUIDLE_FLAG_INIT_XSTATE)
-			state->enter = intel_idle_xstate;
-
 		if ((disabled_states_mask & BIT(drv->state_count)) ||
 		    ((icpu->use_acpi || force_use_acpi) &&
 		     intel_idle_off_by_default(mwait_hint) &&