[PATCH 1/4] intel_idle: refactor and split out model specific fixups into its own function

[PATCH 1/4] intel_idle: refactor and split out model specific fixups into its own function

[arjan]

From: Arjan van de Ven <arjan@linux.intel.com>

As this list has been growing over time, lets split it into its own function.
That also makes it possible to move other hw workarounds into this function over time.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/idle/intel_idle.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index b930036edbbe..3bab6500697a 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1955,10 +1955,8 @@ static bool should_verify_mwait(struct cpuidle_state *state)
 	return true;
 }
 
-static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
+static void __init model_specific_fixups(void)
 {
-	int cstate;
-
 	switch (boot_cpu_data.x86_model) {
 	case INTEL_FAM6_IVYBRIDGE_X:
 		ivt_idle_state_table_update();
@@ -1983,6 +1981,13 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 		adl_idle_state_table_update();
 		break;
 	}
+}
+
+static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
+{
+	int cstate;
+
+	model_specific_fixups();
 
 	for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
 		struct cpuidle_state *state;
-- 
2.41.0

[PATCH 2/4] intel_idle: move one more model specific workaround into the newly created seperate function

[arjan]

From: Arjan van de Ven <arjan@linux.intel.com>

byt_auto_demotion_disable_flag (for baytrail) handling can now be moved into the newly created
model_specific_fixups() function.

This frees up intel_idle_init_cstates_icpu() from containing model specific information direclty

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/idle/intel_idle.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 3bab6500697a..f3eba9b147d2 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1981,6 +1981,11 @@ static void __init model_specific_fixups(void)
 		adl_idle_state_table_update();
 		break;
 	}
+
+	if (icpu->byt_auto_demotion_disable_flag) {
+		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
+		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
+	}
 }
 
 static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
@@ -2030,10 +2035,6 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 		drv->state_count++;
 	}
 
-	if (icpu->byt_auto_demotion_disable_flag) {
-		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
-		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
-	}
 }
 
 /**
-- 
2.41.0

[PATCH 3/4] intel_idle: add VM guest (non)handling to model_specific_fixups

[arjan]

From: Arjan van de Ven <arjan@linux.intel.com>

The model specific fixups don't make sense in a VM guest; all the MSR
accesses at best will be fiction, and at worse cause logspam

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/idle/intel_idle.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index f3eba9b147d2..4dd8081d2406 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1957,6 +1957,10 @@ static bool should_verify_mwait(struct cpuidle_state *state)
 
 static void __init model_specific_fixups(void)
 {
+	/* when running in a hypervisor, none of these fixups will work */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return;
+
 	switch (boot_cpu_data.x86_model) {
 	case INTEL_FAM6_IVYBRIDGE_X:
 		ivt_idle_state_table_update();
-- 
2.41.0

[PATCH 4/4] intel_idle: remove intel_idle_hlt_irq_on

[arjan]

From: Arjan van de Ven <arjan@linux.intel.com>

PeterZ pointed out that the whole function is redundant as for hlt, it's always "irq on"

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/idle/intel_idle.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 4dd8081d2406..038322c23e34 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -224,17 +224,6 @@ static __cpuidle int intel_idle_hlt(struct cpuidle_device *dev,
 	return __intel_idle_hlt(dev, drv, index);
 }
 
-static __cpuidle int intel_idle_hlt_irq_on(struct cpuidle_device *dev,
-                                   struct cpuidle_driver *drv, int index)
-{
-       int ret;
-
-       raw_local_irq_enable();
-       ret = __intel_idle_hlt(dev, drv, index);
-       raw_local_irq_disable();
-
-       return ret;
-}
 
 /**
  * intel_idle_s2idle - Ask the processor to enter the given idle state.
@@ -1897,15 +1886,8 @@ static bool __init intel_idle_verify_cstate(unsigned int mwait_hint)
 
 static void state_update_enter_method(struct cpuidle_state *state, int cstate)
 {
-	if (state->enter == intel_idle_hlt) {
-		if (force_irq_on) {
-			pr_info("forced intel_idle_irq for state %d\n", cstate);
-			state->enter = intel_idle_hlt_irq_on;
-		}
+	if (state->enter == intel_idle_hlt)
 		return;
-	}
-	if (state->enter == intel_idle_hlt_irq_on)
-		return; /* no update scenarios */
 
 	if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {
 		/*
@@ -1949,8 +1931,6 @@ static bool should_verify_mwait(struct cpuidle_state *state)
 {
 	if (state->enter == intel_idle_hlt)
 		return false;
-	if (state->enter == intel_idle_hlt_irq_on)
-		return false;
 
 	return true;
 }
-- 
2.41.0

Re: [PATCH 1/4] intel_idle: refactor and split out model specific fixups into its own function

[Rafael J. Wysocki]

On Mon, Jul 17, 2023 at 5:19 PM <arjan@linux.intel.com> wrote:
>
> From: Arjan van de Ven <arjan@linux.intel.com>
>
> As this list has been growing over time, lets split it into its own function.
> That also makes it possible to move other hw workarounds into this function over time.
>
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

After a discussion with Peter I have decided to revert the original
changes, sorry about that.

IMV Peter has a point regarding the lack of data supporting the
claimed performance gain and without the umwait part they are rather
hard to defend.

> ---
>  drivers/idle/intel_idle.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b930036edbbe..3bab6500697a 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -1955,10 +1955,8 @@ static bool should_verify_mwait(struct cpuidle_state *state)
>         return true;
>  }
>
> -static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
> +static void __init model_specific_fixups(void)
>  {
> -       int cstate;
> -
>         switch (boot_cpu_data.x86_model) {
>         case INTEL_FAM6_IVYBRIDGE_X:
>                 ivt_idle_state_table_update();
> @@ -1983,6 +1981,13 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
>                 adl_idle_state_table_update();
>                 break;
>         }
> +}
> +
> +static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
> +{
> +       int cstate;
> +
> +       model_specific_fixups();
>
>         for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
>                 struct cpuidle_state *state;
> --
> 2.41.0
>