[PATCH v7 0/4] SRF: Fix offline CPU preventing pc6 entry

[PATCH v7 0/4] SRF: Fix offline CPU preventing pc6 entry

[Patryk Wlazlyn]

Small update, after Gautham's review and few changes around comments that
referenced old mwait_play_dead code, that I noticed after submitting the
v6. Some places referenced the old mwait_play_dead() in the comments, so
I decided to reuse the mwait_play_dead() name, so that the comments
don't have to be updated.

Changes since v6:
 * Renamed mwait_play_dead to mwait_play_dead_cpuid_hint in 1/1, so that
   mwait_play_dead name can be reused for the function that takes the
   MWAIT hint as an argument. This leaves the comments around the
   smpboot.c file that reference the old mwait_play_dead() unchanged.

 * Moved the old comment above mwait_play_dead() to
   mwait_play_dead_with_hints in patch 4/4.

 * Added missing include of asm/smp.h in arch/x86/kernel/acpi/cstate.c

 * Removed empty line added by mistake, spotted by Gautham (Thanks!)
   in arch/x86/include/asm/smp.h

 * Reworded 1/4 commit message to better describe the changes the patch
   is actually making.

Patryk Wlazlyn (4):
  x86/smp: Allow calling mwait_play_dead with an arbitrary hint
  ACPI: processor_idle: Add FFH state handling
  intel_idle: Provide the default enter_dead() handler
  x86/smp native_play_dead: Prefer cpuidle_play_dead() over
    mwait_play_dead()

 arch/x86/include/asm/smp.h    |  3 +++
 arch/x86/kernel/acpi/cstate.c | 10 ++++++++
 arch/x86/kernel/smpboot.c     | 46 ++++-------------------------------
 drivers/acpi/processor_idle.c |  2 ++
 drivers/idle/intel_idle.c     | 18 ++++++++++++--
 include/acpi/processor.h      |  5 ++++
 6 files changed, 41 insertions(+), 43 deletions(-)

-- 
2.47.1

[PATCH v7 1/4] x86/smp: Allow calling mwait_play_dead with an arbitrary hint

[Patryk Wlazlyn]

Introduce a helper function to allow offlined CPUs to enter FFh idle
states with a specific MWAIT hint. The new helper will be used in
subsequent patches by the acpi_idle and intel_idle drivers.

No functional change intended.

Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
---
 arch/x86/include/asm/smp.h |  3 ++
 arch/x86/kernel/smpboot.c  | 90 ++++++++++++++++++++------------------
 2 files changed, 51 insertions(+), 42 deletions(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..dfd09a1e09bf 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -114,6 +114,7 @@ void wbinvd_on_cpu(int cpu);
 int wbinvd_on_all_cpus(void);
 
 void smp_kick_mwait_play_dead(void);
+void mwait_play_dead(unsigned int hint);
 
 void native_smp_send_reschedule(int cpu);
 void native_send_call_func_ipi(const struct cpumask *mask);
@@ -164,6 +165,8 @@ static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 {
 	return (struct cpumask *)cpumask_of(0);
 }
+
+static inline void mwait_play_dead(unsigned int eax_hint) { }
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_DEBUG_NMI_SELFTEST
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b5a8f0891135..8a3545c2cae9 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1272,13 +1272,57 @@ void play_dead_common(void)
 	local_irq_disable();
 }
 
+void __noreturn mwait_play_dead(unsigned int eax_hint)
+{
+	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
+
+	/* Set up state for the kexec() hack below */
+	md->status = CPUDEAD_MWAIT_WAIT;
+	md->control = CPUDEAD_MWAIT_WAIT;
+
+	wbinvd();
+
+	while (1) {
+		/*
+		 * The CLFLUSH is a workaround for erratum AAI65 for
+		 * the Xeon 7400 series.  It's not clear it is actually
+		 * needed, but it should be harmless in either case.
+		 * The WBINVD is insufficient due to the spurious-wakeup
+		 * case where we return around the loop.
+		 */
+		mb();
+		clflush(md);
+		mb();
+		__monitor(md, 0, 0);
+		mb();
+		__mwait(eax_hint, 0);
+
+		if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
+			/*
+			 * Kexec is about to happen. Don't go back into mwait() as
+			 * the kexec kernel might overwrite text and data including
+			 * page tables and stack. So mwait() would resume when the
+			 * monitor cache line is written to and then the CPU goes
+			 * south due to overwritten text, page tables and stack.
+			 *
+			 * Note: This does _NOT_ protect against a stray MCE, NMI,
+			 * SMI. They will resume execution at the instruction
+			 * following the HLT instruction and run into the problem
+			 * which this is trying to prevent.
+			 */
+			WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
+			while(1)
+				native_halt();
+		}
+	}
+}
+
 /*
  * We need to flush the caches before going to sleep, lest we have
  * dirty data in our caches when we come back up.
  */
-static inline void mwait_play_dead(void)
+static inline void mwait_play_dead_cpuid_hint(void)
 {
-	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
 	unsigned int eax, ebx, ecx, edx;
 	unsigned int highest_cstate = 0;
 	unsigned int highest_subcstate = 0;
@@ -1316,45 +1360,7 @@ static inline void mwait_play_dead(void)
 			(highest_subcstate - 1);
 	}
 
-	/* Set up state for the kexec() hack below */
-	md->status = CPUDEAD_MWAIT_WAIT;
-	md->control = CPUDEAD_MWAIT_WAIT;
-
-	wbinvd();
-
-	while (1) {
-		/*
-		 * The CLFLUSH is a workaround for erratum AAI65 for
-		 * the Xeon 7400 series.  It's not clear it is actually
-		 * needed, but it should be harmless in either case.
-		 * The WBINVD is insufficient due to the spurious-wakeup
-		 * case where we return around the loop.
-		 */
-		mb();
-		clflush(md);
-		mb();
-		__monitor(md, 0, 0);
-		mb();
-		__mwait(eax, 0);
-
-		if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
-			/*
-			 * Kexec is about to happen. Don't go back into mwait() as
-			 * the kexec kernel might overwrite text and data including
-			 * page tables and stack. So mwait() would resume when the
-			 * monitor cache line is written to and then the CPU goes
-			 * south due to overwritten text, page tables and stack.
-			 *
-			 * Note: This does _NOT_ protect against a stray MCE, NMI,
-			 * SMI. They will resume execution at the instruction
-			 * following the HLT instruction and run into the problem
-			 * which this is trying to prevent.
-			 */
-			WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
-			while(1)
-				native_halt();
-		}
-	}
+	mwait_play_dead(eax);
 }
 
 /*
@@ -1407,7 +1413,7 @@ void native_play_dead(void)
 	play_dead_common();
 	tboot_shutdown(TB_SHUTDOWN_WFS);
 
-	mwait_play_dead();
+	mwait_play_dead_cpuid_hint();
 	if (cpuidle_play_dead())
 		hlt_play_dead();
 }
-- 
2.47.1

[PATCH v7 2/4] ACPI: processor_idle: Add FFH state handling

[Patryk Wlazlyn]

Recent Intel platforms will depend on the idle driver to pass the
correct hint for playing dead via mwait_play_dead_with_hint(). Expand
the existing enter_dead interface with handling for FFH states and pass
the MWAIT hint to the mwait_play_dead code.

Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
---
 arch/x86/kernel/acpi/cstate.c | 10 ++++++++++
 drivers/acpi/processor_idle.c |  2 ++
 include/acpi/processor.h      |  5 +++++
 3 files changed, 17 insertions(+)

diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index f3ffd0a3a012..8d7b8b02ddb9 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -15,6 +15,7 @@
 #include <acpi/processor.h>
 #include <asm/mwait.h>
 #include <asm/special_insns.h>
+#include <asm/smp.h>
 
 /*
  * Initialize bm_flags based on the CPU cache properties
@@ -204,6 +205,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 }
 EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe);
 
+void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
+{
+	unsigned int cpu = smp_processor_id();
+	struct cstate_entry *percpu_entry;
+
+	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
+	mwait_play_dead(percpu_entry->states[cx->index].eax);
+}
+
 void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
 {
 	unsigned int cpu = smp_processor_id();
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index ce728cf7e301..83213fa47c1b 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 			raw_safe_halt();
 		else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
 			io_idle(cx->address);
+		} else if (cx->entry_method == ACPI_CSTATE_FFH) {
+			acpi_processor_ffh_play_dead(cx);
 		} else
 			return;
 	}
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index a17e97e634a6..63a37e72b721 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 				    struct acpi_processor_cx *cx,
 				    struct acpi_power_register *reg);
 void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
+void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx);
 #else
 static inline void acpi_processor_power_init_bm_check(struct
 						      acpi_processor_flags
@@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
 {
 	return;
 }
+static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
+{
+	return;
+}
 #endif
 
 static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg,
-- 
2.47.1

[PATCH v7 3/4] intel_idle: Provide the default enter_dead() handler

[Patryk Wlazlyn]

Recent Intel platforms require idle driver to provide information about
the MWAIT hint used to enter the deepest idle state in the play_dead
code.

Provide the default enter_dead() handler for all of the platforms and
allow overwriting with a custom handler for each platform if needed.

Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
---
 drivers/idle/intel_idle.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index ac4d8faa3886..c6874a6dbe95 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -56,6 +56,7 @@
 #include <asm/mwait.h>
 #include <asm/spec-ctrl.h>
 #include <asm/fpu/api.h>
+#include <asm/smp.h>
 
 #define INTEL_IDLE_VERSION "0.5.1"
 
@@ -227,6 +228,16 @@ static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev,
 	return 0;
 }
 
+static __cpuidle void intel_idle_enter_dead(struct cpuidle_device *dev,
+					    int index)
+{
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_state *state = &drv->states[index];
+	unsigned long eax = flg2MWAIT(state->flags);
+
+	mwait_play_dead(eax);
+}
+
 /*
  * States are indexed by the cstate number,
  * which is also the index into the MWAIT hint array.
@@ -1798,6 +1809,7 @@ static void __init intel_idle_init_cstates_acpi(struct cpuidle_driver *drv)
 			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
 
 		state->enter = intel_idle;
+		state->enter_dead = intel_idle_enter_dead;
 		state->enter_s2idle = intel_idle_s2idle;
 	}
 }
@@ -2143,10 +2155,12 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 		if (intel_idle_max_cstate_reached(cstate))
 			break;
 
-		if (!cpuidle_state_table[cstate].enter &&
-		    !cpuidle_state_table[cstate].enter_s2idle)
+		if (!cpuidle_state_table[cstate].enter)
 			break;
 
+		if (!cpuidle_state_table[cstate].enter_dead)
+			cpuidle_state_table[cstate].enter_dead = intel_idle_enter_dead;
+
 		/* If marked as unusable, skip this state. */
 		if (cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_UNUSABLE) {
 			pr_debug("state %s is disabled\n",
-- 
2.47.1

[PATCH v7 4/4] x86/smp native_play_dead: Prefer cpuidle_play_dead() over mwait_play_dead()

[Patryk Wlazlyn]

The current algorithm* for looking up the mwait hint for the deepest
cstate, in mwait_play_dead() code works by inspecting CPUID leaf 0x5 and
calculates the mwait hint based on the number of reported substates.
This approach depends on the hints associated with them to be continuous
in the range [0, NUM_SUBSTATES-1]. This continuity is not documented and
is not met on the recent Intel platforms.

 * The current algorithm is implemented in the for loop inspecting edx
   in mwait_play_dead().

For example, Intel's Sierra Forest report two cstates with two substates
each in cpuid leaf 0x5:

  Name*   target cstate    target subcstate (mwait hint)
  ===========================================================
  C1      0x00             0x00
  C1E     0x00             0x01

  --      0x10             ----

  C6S     0x20             0x22
  C6P     0x20             0x23

  --      0x30             ----

  /* No more (sub)states all the way down to the end. */
  ===========================================================

   * Names of the cstates are not included in the CPUID leaf 0x5, they are
     taken from the product specific documentation.

Notice that hints 0x20 and 0x21 are skipped entirely for the target
cstate 0x20 (C6), being a cause of the problem for the current cpuid
leaf 0x5 algorithm.

Remove the old implementation of play_dead MWAIT hint calculation based
on the CPUID leaf 0x5 in mwait_play_dead() and delegate calling of the
mwait_play_dead_with_hint() to the idle driver.

Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
---
 arch/x86/kernel/smpboot.c | 56 +++++----------------------------------
 1 file changed, 7 insertions(+), 49 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 8a3545c2cae9..82801137486d 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1272,6 +1272,10 @@ void play_dead_common(void)
 	local_irq_disable();
 }
 
+/*
+ * We need to flush the caches before going to sleep, lest we have
+ * dirty data in our caches when we come back up.
+ */
 void __noreturn mwait_play_dead(unsigned int eax_hint)
 {
 	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
@@ -1317,52 +1321,6 @@ void __noreturn mwait_play_dead(unsigned int eax_hint)
 	}
 }
 
-/*
- * We need to flush the caches before going to sleep, lest we have
- * dirty data in our caches when we come back up.
- */
-static inline void mwait_play_dead_cpuid_hint(void)
-{
-	unsigned int eax, ebx, ecx, edx;
-	unsigned int highest_cstate = 0;
-	unsigned int highest_subcstate = 0;
-	int i;
-
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
-	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
-		return;
-	if (!this_cpu_has(X86_FEATURE_MWAIT))
-		return;
-	if (!this_cpu_has(X86_FEATURE_CLFLUSH))
-		return;
-	if (__this_cpu_read(cpu_info.cpuid_level) < CPUID_MWAIT_LEAF)
-		return;
-
-	eax = CPUID_MWAIT_LEAF;
-	ecx = 0;
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-
-	/*
-	 * eax will be 0 if EDX enumeration is not valid.
-	 * Initialized below to cstate, sub_cstate value when EDX is valid.
-	 */
-	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED)) {
-		eax = 0;
-	} else {
-		edx >>= MWAIT_SUBSTATE_SIZE;
-		for (i = 0; i < 7 && edx; i++, edx >>= MWAIT_SUBSTATE_SIZE) {
-			if (edx & MWAIT_SUBSTATE_MASK) {
-				highest_cstate = i;
-				highest_subcstate = edx & MWAIT_SUBSTATE_MASK;
-			}
-		}
-		eax = (highest_cstate << MWAIT_SUBSTATE_SIZE) |
-			(highest_subcstate - 1);
-	}
-
-	mwait_play_dead(eax);
-}
-
 /*
  * Kick all "offline" CPUs out of mwait on kexec(). See comment in
  * mwait_play_dead().
@@ -1413,9 +1371,9 @@ void native_play_dead(void)
 	play_dead_common();
 	tboot_shutdown(TB_SHUTDOWN_WFS);
 
-	mwait_play_dead_cpuid_hint();
-	if (cpuidle_play_dead())
-		hlt_play_dead();
+	/* Below returns only on error. */
+	cpuidle_play_dead();
+	hlt_play_dead();
 }
 
 #else /* ... !CONFIG_HOTPLUG_CPU */
-- 
2.47.1

Re: [PATCH v7 1/4] x86/smp: Allow calling mwait_play_dead with an arbitrary hint

[Gautham R. Shenoy]

On Fri, Nov 29, 2024 at 07:22:29PM +0100, Patryk Wlazlyn wrote:
> Introduce a helper function to allow offlined CPUs to enter FFh idle
> states with a specific MWAIT hint. The new helper will be used in
> subsequent patches by the acpi_idle and intel_idle drivers.
> 
> No functional change intended.
> 
> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>

This looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  arch/x86/include/asm/smp.h |  3 ++
>  arch/x86/kernel/smpboot.c  | 90 ++++++++++++++++++++------------------
>  2 files changed, 51 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index ca073f40698f..dfd09a1e09bf 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -114,6 +114,7 @@ void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
>  
>  void smp_kick_mwait_play_dead(void);
> +void mwait_play_dead(unsigned int hint);
>  
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> @@ -164,6 +165,8 @@ static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>  {
>  	return (struct cpumask *)cpumask_of(0);
>  }
> +
> +static inline void mwait_play_dead(unsigned int eax_hint) { }
>  #endif /* CONFIG_SMP */
>  
>  #ifdef CONFIG_DEBUG_NMI_SELFTEST
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index b5a8f0891135..8a3545c2cae9 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1272,13 +1272,57 @@ void play_dead_common(void)
>  	local_irq_disable();
>  }
>  
> +void __noreturn mwait_play_dead(unsigned int eax_hint)
> +{
> +	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
> +
> +	/* Set up state for the kexec() hack below */
> +	md->status = CPUDEAD_MWAIT_WAIT;
> +	md->control = CPUDEAD_MWAIT_WAIT;
> +
> +	wbinvd();
> +
> +	while (1) {
> +		/*
> +		 * The CLFLUSH is a workaround for erratum AAI65 for
> +		 * the Xeon 7400 series.  It's not clear it is actually
> +		 * needed, but it should be harmless in either case.
> +		 * The WBINVD is insufficient due to the spurious-wakeup
> +		 * case where we return around the loop.
> +		 */
> +		mb();
> +		clflush(md);
> +		mb();
> +		__monitor(md, 0, 0);
> +		mb();
> +		__mwait(eax_hint, 0);
> +
> +		if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
> +			/*
> +			 * Kexec is about to happen. Don't go back into mwait() as
> +			 * the kexec kernel might overwrite text and data including
> +			 * page tables and stack. So mwait() would resume when the
> +			 * monitor cache line is written to and then the CPU goes
> +			 * south due to overwritten text, page tables and stack.
> +			 *
> +			 * Note: This does _NOT_ protect against a stray MCE, NMI,
> +			 * SMI. They will resume execution at the instruction
> +			 * following the HLT instruction and run into the problem
> +			 * which this is trying to prevent.
> +			 */
> +			WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
> +			while(1)
> +				native_halt();
> +		}
> +	}
> +}
> +
>  /*
>   * We need to flush the caches before going to sleep, lest we have
>   * dirty data in our caches when we come back up.
>   */
> -static inline void mwait_play_dead(void)
> +static inline void mwait_play_dead_cpuid_hint(void)
>  {
> -	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
>  	unsigned int eax, ebx, ecx, edx;
>  	unsigned int highest_cstate = 0;
>  	unsigned int highest_subcstate = 0;
> @@ -1316,45 +1360,7 @@ static inline void mwait_play_dead(void)
>  			(highest_subcstate - 1);
>  	}
>  
> -	/* Set up state for the kexec() hack below */
> -	md->status = CPUDEAD_MWAIT_WAIT;
> -	md->control = CPUDEAD_MWAIT_WAIT;
> -
> -	wbinvd();
> -
> -	while (1) {
> -		/*
> -		 * The CLFLUSH is a workaround for erratum AAI65 for
> -		 * the Xeon 7400 series.  It's not clear it is actually
> -		 * needed, but it should be harmless in either case.
> -		 * The WBINVD is insufficient due to the spurious-wakeup
> -		 * case where we return around the loop.
> -		 */
> -		mb();
> -		clflush(md);
> -		mb();
> -		__monitor(md, 0, 0);
> -		mb();
> -		__mwait(eax, 0);
> -
> -		if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
> -			/*
> -			 * Kexec is about to happen. Don't go back into mwait() as
> -			 * the kexec kernel might overwrite text and data including
> -			 * page tables and stack. So mwait() would resume when the
> -			 * monitor cache line is written to and then the CPU goes
> -			 * south due to overwritten text, page tables and stack.
> -			 *
> -			 * Note: This does _NOT_ protect against a stray MCE, NMI,
> -			 * SMI. They will resume execution at the instruction
> -			 * following the HLT instruction and run into the problem
> -			 * which this is trying to prevent.
> -			 */
> -			WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
> -			while(1)
> -				native_halt();
> -		}
> -	}
> +	mwait_play_dead(eax);
>  }
>  
>  /*
> @@ -1407,7 +1413,7 @@ void native_play_dead(void)
>  	play_dead_common();
>  	tboot_shutdown(TB_SHUTDOWN_WFS);
>  
> -	mwait_play_dead();
> +	mwait_play_dead_cpuid_hint();
>  	if (cpuidle_play_dead())
>  		hlt_play_dead();
>  }
> -- 
> 2.47.1
> 

Re: [PATCH v7 2/4] ACPI: processor_idle: Add FFH state handling

[Gautham R. Shenoy]

On Fri, Nov 29, 2024 at 07:22:30PM +0100, Patryk Wlazlyn wrote:
> Recent Intel platforms will depend on the idle driver to pass the
> correct hint for playing dead via mwait_play_dead_with_hint(). Expand
> the existing enter_dead interface with handling for FFH states and pass
> the MWAIT hint to the mwait_play_dead code.
> 
> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
> Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

Looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  arch/x86/kernel/acpi/cstate.c | 10 ++++++++++
>  drivers/acpi/processor_idle.c |  2 ++
>  include/acpi/processor.h      |  5 +++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index f3ffd0a3a012..8d7b8b02ddb9 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -15,6 +15,7 @@
>  #include <acpi/processor.h>
>  #include <asm/mwait.h>
>  #include <asm/special_insns.h>
> +#include <asm/smp.h>
>  
>  /*
>   * Initialize bm_flags based on the CPU cache properties
> @@ -204,6 +205,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  }
>  EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe);
>  
> +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cstate_entry *percpu_entry;
> +
> +	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> +	mwait_play_dead(percpu_entry->states[cx->index].eax);
> +}
> +
>  void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
>  {
>  	unsigned int cpu = smp_processor_id();
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index ce728cf7e301..83213fa47c1b 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index)
>  			raw_safe_halt();
>  		else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
>  			io_idle(cx->address);
> +		} else if (cx->entry_method == ACPI_CSTATE_FFH) {
> +			acpi_processor_ffh_play_dead(cx);
>  		} else
>  			return;
>  	}
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index a17e97e634a6..63a37e72b721 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  				    struct acpi_processor_cx *cx,
>  				    struct acpi_power_register *reg);
>  void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
> +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx);
>  #else
>  static inline void acpi_processor_power_init_bm_check(struct
>  						      acpi_processor_flags
> @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
>  {
>  	return;
>  }
> +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> +{
> +	return;
> +}
>  #endif
>  
>  static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg,
> -- 
> 2.47.1
> 

Re: [PATCH v7 4/4] x86/smp native_play_dead: Prefer cpuidle_play_dead() over mwait_play_dead()

[Gautham R. Shenoy]

Hello Patryk,

On Fri, Nov 29, 2024 at 07:22:32PM +0100, Patryk Wlazlyn wrote:

A couple of minor nits in the commit log:

> The current algorithm* for looking up the mwait hint for the deepest
> cstate, in mwait_play_dead() code works by inspecting CPUID leaf 0x5 and
             ^^^^^^^^^^^^^^^^^
	     mwait_play_dead_cpuid_hint() after the rename in Patch 1.
	     

> calculates the mwait hint based on the number of reported substates.
> This approach depends on the hints associated with them to be continuous
> in the range [0, NUM_SUBSTATES-1]. This continuity is not documented and
> is not met on the recent Intel platforms.
> 
>  * The current algorithm is implemented in the for loop inspecting edx
>    in mwait_play_dead().
> 
> For example, Intel's Sierra Forest report two cstates with two substates
> each in cpuid leaf 0x5:
> 
>   Name*   target cstate    target subcstate (mwait hint)
>   ===========================================================
>   C1      0x00             0x00
>   C1E     0x00             0x01
> 
>   --      0x10             ----
> 
>   C6S     0x20             0x22
>   C6P     0x20             0x23
> 
>   --      0x30             ----
> 
>   /* No more (sub)states all the way down to the end. */
>   ===========================================================
> 
>    * Names of the cstates are not included in the CPUID leaf 0x5, they are
>      taken from the product specific documentation.
> 
> Notice that hints 0x20 and 0x21 are skipped entirely for the target
> cstate 0x20 (C6), being a cause of the problem for the current cpuid
> leaf 0x5 algorithm.
> 
> Remove the old implementation of play_dead MWAIT hint calculation based
> on the CPUID leaf 0x5 in mwait_play_dead() and delegate calling of the
                           ^^^^^^^^^^^^^^^^^
                           mwait_play_dead_cpuid_hint()
			   
> mwait_play_dead_with_hint() to the idle driver.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  mwait_play_dead()


This should work since both acpi-idle and intel-idle drivers now
support the .enter_dead() callbacks for FFh idle states.


Otherwise, the patch looks good to me. Feel free to add the following
tag in the next version.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> 
> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
> ---
>  arch/x86/kernel/smpboot.c | 56 +++++----------------------------------
>  1 file changed, 7 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 8a3545c2cae9..82801137486d 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1272,6 +1272,10 @@ void play_dead_common(void)
>  	local_irq_disable();
>  }
>  
> +/*
> + * We need to flush the caches before going to sleep, lest we have
> + * dirty data in our caches when we come back up.
> + */
>  void __noreturn mwait_play_dead(unsigned int eax_hint)
>  {
>  	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
> @@ -1317,52 +1321,6 @@ void __noreturn mwait_play_dead(unsigned int eax_hint)
>  	}
>  }
>  
> -/*
> - * We need to flush the caches before going to sleep, lest we have
> - * dirty data in our caches when we come back up.
> - */
> -static inline void mwait_play_dead_cpuid_hint(void)
> -{
> -	unsigned int eax, ebx, ecx, edx;
> -	unsigned int highest_cstate = 0;
> -	unsigned int highest_subcstate = 0;
> -	int i;
> -
> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> -	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> -		return;
> -	if (!this_cpu_has(X86_FEATURE_MWAIT))
> -		return;
> -	if (!this_cpu_has(X86_FEATURE_CLFLUSH))
> -		return;
> -	if (__this_cpu_read(cpu_info.cpuid_level) < CPUID_MWAIT_LEAF)
> -		return;
> -
> -	eax = CPUID_MWAIT_LEAF;
> -	ecx = 0;
> -	native_cpuid(&eax, &ebx, &ecx, &edx);
> -
> -	/*
> -	 * eax will be 0 if EDX enumeration is not valid.
> -	 * Initialized below to cstate, sub_cstate value when EDX is valid.
> -	 */
> -	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED)) {
> -		eax = 0;
> -	} else {
> -		edx >>= MWAIT_SUBSTATE_SIZE;
> -		for (i = 0; i < 7 && edx; i++, edx >>= MWAIT_SUBSTATE_SIZE) {
> -			if (edx & MWAIT_SUBSTATE_MASK) {
> -				highest_cstate = i;
> -				highest_subcstate = edx & MWAIT_SUBSTATE_MASK;
> -			}
> -		}
> -		eax = (highest_cstate << MWAIT_SUBSTATE_SIZE) |
> -			(highest_subcstate - 1);
> -	}
> -
> -	mwait_play_dead(eax);
> -}
> -
>  /*
>   * Kick all "offline" CPUs out of mwait on kexec(). See comment in
>   * mwait_play_dead().
> @@ -1413,9 +1371,9 @@ void native_play_dead(void)
>  	play_dead_common();
>  	tboot_shutdown(TB_SHUTDOWN_WFS);
>  
> -	mwait_play_dead_cpuid_hint();
> -	if (cpuidle_play_dead())
> -		hlt_play_dead();
> +	/* Below returns only on error. */
> +	cpuidle_play_dead();
> +	hlt_play_dead();
>  }
>  
>  #else /* ... !CONFIG_HOTPLUG_CPU */
> -- 
> 2.47.1
> 

Re: [PATCH v7 1/4] x86/smp: Allow calling mwait_play_dead with an arbitrary hint

[Rafael J. Wysocki]

First, I would change the subject to something like "x86/smp: Add hint
parameter to mwait_play_dead()"

On Fri, Nov 29, 2024 at 7:22 PM Patryk Wlazlyn
<patryk.wlazlyn@linux.intel.com> wrote:
>
> Introduce a helper function to allow offlined CPUs to enter FFh idle
> states with a specific MWAIT hint. The new helper will be used in
> subsequent patches by the acpi_idle and intel_idle drivers.

And the above would become

"Change mwait_play_dead() into a helper function allowing CPUs going
offline to enter idle states via MWAIT with a specific hint passed to
it as an argument.

Add mwait_play_dead_cpuid_hint() as a wrapper around mwait_play_dead()
implementing the existing behavior of the code.

Subsequently, the new helper will also be used by the acpi_idle and
intel_idle drivers in idle-state-specific :enter_dead() callbacks."

> No functional change intended.
>
> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
> ---
>  arch/x86/include/asm/smp.h |  3 ++
>  arch/x86/kernel/smpboot.c  | 90 ++++++++++++++++++++------------------
>  2 files changed, 51 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index ca073f40698f..dfd09a1e09bf 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -114,6 +114,7 @@ void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
>
>  void smp_kick_mwait_play_dead(void);
> +void mwait_play_dead(unsigned int hint);
>
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> @@ -164,6 +165,8 @@ static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>  {
>         return (struct cpumask *)cpumask_of(0);
>  }
> +
> +static inline void mwait_play_dead(unsigned int eax_hint) { }
>  #endif /* CONFIG_SMP */
>
>  #ifdef CONFIG_DEBUG_NMI_SELFTEST
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index b5a8f0891135..8a3545c2cae9 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1272,13 +1272,57 @@ void play_dead_common(void)
>         local_irq_disable();
>  }
>
> +void __noreturn mwait_play_dead(unsigned int eax_hint)
> +{
> +       struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
> +
> +       /* Set up state for the kexec() hack below */
> +       md->status = CPUDEAD_MWAIT_WAIT;
> +       md->control = CPUDEAD_MWAIT_WAIT;
> +
> +       wbinvd();
> +
> +       while (1) {
> +               /*
> +                * The CLFLUSH is a workaround for erratum AAI65 for
> +                * the Xeon 7400 series.  It's not clear it is actually
> +                * needed, but it should be harmless in either case.
> +                * The WBINVD is insufficient due to the spurious-wakeup
> +                * case where we return around the loop.
> +                */
> +               mb();
> +               clflush(md);
> +               mb();
> +               __monitor(md, 0, 0);
> +               mb();
> +               __mwait(eax_hint, 0);
> +
> +               if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
> +                       /*
> +                        * Kexec is about to happen. Don't go back into mwait() as
> +                        * the kexec kernel might overwrite text and data including
> +                        * page tables and stack. So mwait() would resume when the
> +                        * monitor cache line is written to and then the CPU goes
> +                        * south due to overwritten text, page tables and stack.
> +                        *
> +                        * Note: This does _NOT_ protect against a stray MCE, NMI,
> +                        * SMI. They will resume execution at the instruction
> +                        * following the HLT instruction and run into the problem
> +                        * which this is trying to prevent.
> +                        */
> +                       WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
> +                       while(1)
> +                               native_halt();
> +               }
> +       }
> +}
> +
>  /*
>   * We need to flush the caches before going to sleep, lest we have
>   * dirty data in our caches when we come back up.
>   */
> -static inline void mwait_play_dead(void)
> +static inline void mwait_play_dead_cpuid_hint(void)
>  {
> -       struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
>         unsigned int eax, ebx, ecx, edx;
>         unsigned int highest_cstate = 0;
>         unsigned int highest_subcstate = 0;
> @@ -1316,45 +1360,7 @@ static inline void mwait_play_dead(void)
>                         (highest_subcstate - 1);
>         }
>
> -       /* Set up state for the kexec() hack below */
> -       md->status = CPUDEAD_MWAIT_WAIT;
> -       md->control = CPUDEAD_MWAIT_WAIT;
> -
> -       wbinvd();
> -
> -       while (1) {
> -               /*
> -                * The CLFLUSH is a workaround for erratum AAI65 for
> -                * the Xeon 7400 series.  It's not clear it is actually
> -                * needed, but it should be harmless in either case.
> -                * The WBINVD is insufficient due to the spurious-wakeup
> -                * case where we return around the loop.
> -                */
> -               mb();
> -               clflush(md);
> -               mb();
> -               __monitor(md, 0, 0);
> -               mb();
> -               __mwait(eax, 0);
> -
> -               if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
> -                       /*
> -                        * Kexec is about to happen. Don't go back into mwait() as
> -                        * the kexec kernel might overwrite text and data including
> -                        * page tables and stack. So mwait() would resume when the
> -                        * monitor cache line is written to and then the CPU goes
> -                        * south due to overwritten text, page tables and stack.
> -                        *
> -                        * Note: This does _NOT_ protect against a stray MCE, NMI,
> -                        * SMI. They will resume execution at the instruction
> -                        * following the HLT instruction and run into the problem
> -                        * which this is trying to prevent.
> -                        */
> -                       WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
> -                       while(1)
> -                               native_halt();
> -               }
> -       }
> +       mwait_play_dead(eax);
>  }
>
>  /*
> @@ -1407,7 +1413,7 @@ void native_play_dead(void)
>         play_dead_common();
>         tboot_shutdown(TB_SHUTDOWN_WFS);
>
> -       mwait_play_dead();
> +       mwait_play_dead_cpuid_hint();
>         if (cpuidle_play_dead())
>                 hlt_play_dead();
>  }
> --

And honestly I'm wondering why adding a parameter to mwait_play_dead()
is better than introducing mwait_play_dead_with_hint(), in analogy
with the existing mwait_idle_with_hints()?

The latter option would allow you to avoid introducing a function that
is deleted in the same patch series (in patch 4).

Re: [PATCH v7 2/4] ACPI: processor_idle: Add FFH state handling

[Rafael J. Wysocki]

On Fri, Nov 29, 2024 at 7:22 PM Patryk Wlazlyn
<patryk.wlazlyn@linux.intel.com> wrote:
>
> Recent Intel platforms will depend on the idle driver to pass the
> correct hint for playing dead via mwait_play_dead_with_hint(). Expand
> the existing enter_dead interface with handling for FFH states and pass
> the MWAIT hint to the mwait_play_dead code.

First, the changelog doesn't match the code changes because you've
decided to use mwait_play_dead() without changing its name.

Second, "recent" is something that has happened already, so I would
say "new" or "upcoming" instead.

Overall, my changelog for this patch would be something like this:

"The ACPI processor_idle driver's implementation of the :enter_dead()
callback, acpi_idle_play_dead(), is missing the handling of FFH (Fixed
Functional Hardware) idle states that on x86 platforms are entered by
executing the MWAIT instruction.  If such idle states are listed by
_CST for the given CPU, the idle driver should be able to use the
MWAIT hints corresponding to them in order to put the CPU into an
appropriate idle state when it is going offline, so update
acpi_idle_play_dead() to take the FFH idle states into account.

Going forward this will be depended on by Intel platforms if the
(otherwise default) intel_idle driver is disabled."

The code changes look good to me.

> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com>
> Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> ---
>  arch/x86/kernel/acpi/cstate.c | 10 ++++++++++
>  drivers/acpi/processor_idle.c |  2 ++
>  include/acpi/processor.h      |  5 +++++
>  3 files changed, 17 insertions(+)
>
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index f3ffd0a3a012..8d7b8b02ddb9 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -15,6 +15,7 @@
>  #include <acpi/processor.h>
>  #include <asm/mwait.h>
>  #include <asm/special_insns.h>
> +#include <asm/smp.h>
>
>  /*
>   * Initialize bm_flags based on the CPU cache properties
> @@ -204,6 +205,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  }
>  EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe);
>
> +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> +{
> +       unsigned int cpu = smp_processor_id();
> +       struct cstate_entry *percpu_entry;
> +
> +       percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> +       mwait_play_dead(percpu_entry->states[cx->index].eax);
> +}
> +
>  void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
>  {
>         unsigned int cpu = smp_processor_id();
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index ce728cf7e301..83213fa47c1b 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index)
>                         raw_safe_halt();
>                 else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
>                         io_idle(cx->address);
> +               } else if (cx->entry_method == ACPI_CSTATE_FFH) {
> +                       acpi_processor_ffh_play_dead(cx);
>                 } else
>                         return;
>         }
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index a17e97e634a6..63a37e72b721 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>                                     struct acpi_processor_cx *cx,
>                                     struct acpi_power_register *reg);
>  void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
> +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx);
>  #else
>  static inline void acpi_processor_power_init_bm_check(struct
>                                                       acpi_processor_flags
> @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
>  {
>         return;
>  }
> +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> +{
> +       return;
> +}
>  #endif
>
>  static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg,
> --

Re: [PATCH v7 1/4] x86/smp: Allow calling mwait_play_dead with an arbitrary hint

[Patryk Wlazlyn]

> And honestly I'm wondering why adding a parameter to mwait_play_dead()
> is better than introducing mwait_play_dead_with_hint(), in analogy
> with the existing mwait_idle_with_hints()?
>
> The latter option would allow you to avoid introducing a function that
> is deleted in the same patch series (in patch 4).

We need to be able to call part of the old mwait_play_dead() code,
but without the hint calculation.

mwait_idle_with_hints() doesn't have the "kexec hack" logic.

We also need to leave the old code working and on top of that introduce
the acpi_idle and intel_idle patches that use the new API.

Now the old code is there and the new one. The only thing left is remove
the old code. I did it that way because of the comments earlier indicating
that I should not be breaking code in between.

Let me know if I answered your question or if I misunderstood something
now or earlier.

I'll apply your changelog suggestions when we agree on the implementation.

Re: [PATCH v7 1/4] x86/smp: Allow calling mwait_play_dead with an arbitrary hint

[Rafael J. Wysocki]

On Tue, Dec 17, 2024 at 9:09 PM Patryk Wlazlyn
<patryk.wlazlyn@linux.intel.com> wrote:
>
> > And honestly I'm wondering why adding a parameter to mwait_play_dead()
> > is better than introducing mwait_play_dead_with_hint(), in analogy
> > with the existing mwait_idle_with_hints()?
> >
> > The latter option would allow you to avoid introducing a function that
> > is deleted in the same patch series (in patch 4).
>
> We need to be able to call part of the old mwait_play_dead() code,
> but without the hint calculation.
>
> mwait_idle_with_hints() doesn't have the "kexec hack" logic.

Well, "in analogy" doesn't mean to use mwait_idle_with_hints() instead
of the new function.

Just the name of the new function could be similar to
mwait_idle_with_hints() (which is the name of an existing function),
that is mwait_play_dead_with_hint().

> We also need to leave the old code working and on top of that introduce
> the acpi_idle and intel_idle patches that use the new API.

Sure.  If the name of the new function is mwait_play_dead_with_hint(),
that will still work.

Re: [PATCH v7 1/4] x86/smp: Allow calling mwait_play_dead with an arbitrary hint

[Patryk Wlazlyn]

>>> And honestly I'm wondering why adding a parameter to mwait_play_dead()
>>> is better than introducing mwait_play_dead_with_hint(), in analogy
>>> with the existing mwait_idle_with_hints()?

Well.. Maybe that wasn't that good of an idea. I've given the rationale
in the 0/4:

> Changes since v6:
>  * Renamed mwait_play_dead to mwait_play_dead_cpuid_hint in 1/1, so that
>    mwait_play_dead name can be reused for the function that takes the
>    MWAIT hint as an argument. This leaves the comments around the
>    smpboot.c file that reference the old mwait_play_dead() unchanged.

It makes the patches simpler, in a sense that I don't have to update the
comments each patch when moving things around and renaming.