[PATCH v4 0/2] x86/fpu: Make AMX state ready for CPU idle

[PATCH v4 0/2] x86/fpu: Make AMX state ready for CPU idle

[Chang S. Bae]

Hi,

First of all, my apologies for the long delay with my vacation.

Changes from v3 [1]:
* Call out AMX state only needs to be initialized for CPU idle (Thomas).
* Drop the XCR0 accessor changes as not relevant (Dave).

The series is available here:
  git://github.com/intel/amx-linux.git tilerelease

=== Original Cover Letter ===

AMX state is a large state (at least 8KB or more). Entering CPU idle with
this non-initialized large state may result in shallow states while a
deeper low-power state is available.

We can confirm this behavior is implementation-specific. Section 3.3 in [2]
will be updated to clarify this.

Thanks,
Chang

[1]: https://lore.kernel.org/lkml/20220325022219.829-1-chang.seok.bae@intel.com/
[2]: Intel Architecture Instruction Set Extension Programming Reference
     May 2021, https://software.intel.com/content/dam/develop/external/us/en/documents-tps/architecture-instruction-set-extensions-programming-reference.pdf

Chang S. Bae (2):
  x86/fpu: Add a helper to prepare AMX state for low-power CPU idle
  intel_idle: Add a new flag to initialize the AMX state

 arch/x86/include/asm/fpu/api.h       |  2 ++
 arch/x86/include/asm/special_insns.h |  9 +++++++++
 arch/x86/kernel/fpu/core.c           | 13 +++++++++++++
 drivers/idle/intel_idle.c            | 18 ++++++++++++++++--
 4 files changed, 40 insertions(+), 2 deletions(-)


base-commit: 42226c989789d8da4af1de0c31070c96726d990c
-- 
2.17.1

[PATCH v4 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle

[Chang S. Bae]

When a CPU enters an idle state, non-initialized AMX register state may be
the cause of preventing a deeper low-power state. Other extended register
states whether initialized or not does not impact on the CPU idle state.

The new helper can ensure AMX state initialized before CPU idle, and it
will be used by the intel idle driver.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from v3:
* Call out AMX state in changelog (Thomas Glexiner).

Changes from v2:
* Check the feature flag instead of fpu_state_size_dynamic() (Dave Hansen).

Changes from v1:
* Check the dynamic state flag first, to avoid #UD with XGETBV(1).
---
 arch/x86/include/asm/fpu/api.h       |  2 ++
 arch/x86/include/asm/special_insns.h |  9 +++++++++
 arch/x86/kernel/fpu/core.c           | 13 +++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index c83b3020350a..df48912fd1c8 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -165,4 +165,6 @@ static inline bool fpstate_is_confidential(struct fpu_guest *gfpu)
 struct task_struct;
 extern long fpu_xstate_prctl(struct task_struct *tsk, int option, unsigned long arg2);
 
+extern void fpu_idle_fpregs(void);
+
 #endif /* _ASM_X86_FPU_API_H */
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 68c257a3de0d..d434fbaeb3ff 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -294,6 +294,15 @@ static inline int enqcmds(void __iomem *dst, const void *src)
 	return 0;
 }
 
+static inline void tile_release(void)
+{
+	/*
+	 * Instruction opcode for TILERELEASE; supported in binutils
+	 * version >= 2.36.
+	 */
+	asm volatile(".byte 0xc4, 0xe2, 0x78, 0x49, 0xc0");
+}
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_SPECIAL_INSNS_H */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index e28ab0ecc537..21ca325bd4db 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -836,3 +836,16 @@ int fpu__exception_code(struct fpu *fpu, int trap_nr)
 	 */
 	return 0;
 }
+
+/*
+ * Initialize register state that may prevent from entering low-power idle.
+ * This function will be invoked from the cpuidle driver only when needed.
+ */
+void fpu_idle_fpregs(void)
+{
+	if (cpu_feature_enabled(X86_FEATURE_XGETBV1) &&
+	    (xfeatures_in_use() & XFEATURE_MASK_XTILE)) {
+		tile_release();
+		fpregs_deactivate(&current->thread.fpu);
+	}
+}
-- 
2.17.1

[PATCH v4 2/2] intel_idle: Add a new flag to initialize the AMX state

[Chang S. Bae]

The non-initialized AMX state can be the cause of C-state demotion from C6
to C1E. This low-power idle state may improve power savings and thus result
in a higher available turbo frequency budget.

This behavior is implementation-specific. Initialize the state for the C6
entrance of Sapphire Rapids as needed.

Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Tested-by : Zhang Rui <rui.zhang@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
---
Changes from v2:
* Remove an unnecessary backslash (Rafael Wysocki).

Changes from v1:
* Simplify the code with a new flag (Rui).
* Rebase on Artem's patches for SPR intel_idle.
* Massage the changelog.
---
 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 47551ab73ca8..1f2d0d828a69 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -54,6 +54,7 @@
 #include <asm/intel-family.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
+#include <asm/fpu/api.h>
 
 #define INTEL_IDLE_VERSION "0.5.1"
 
@@ -105,6 +106,11 @@ static unsigned int mwait_substates __initdata;
  */
 #define CPUIDLE_FLAG_ALWAYS_ENABLE	BIT(15)
 
+/*
+ * Initialize large xstate for the C6-state entrance.
+ */
+#define CPUIDLE_FLAG_INIT_XSTATE	BIT(16)
+
 /*
  * MWAIT takes an 8-bit "hint" in EAX "suggesting"
  * the C-state (top nibble) and sub-state (bottom nibble)
@@ -139,6 +145,9 @@ static __cpuidle int intel_idle(struct cpuidle_device *dev,
 	if (state->flags & CPUIDLE_FLAG_IRQ_ENABLE)
 		local_irq_enable();
 
+	if (state->flags & CPUIDLE_FLAG_INIT_XSTATE)
+		fpu_idle_fpregs();
+
 	mwait_idle_with_hints(eax, ecx);
 
 	return index;
@@ -159,8 +168,12 @@ static __cpuidle int intel_idle(struct cpuidle_device *dev,
 static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev,
 				       struct cpuidle_driver *drv, int index)
 {
-	unsigned long eax = flg2MWAIT(drv->states[index].flags);
 	unsigned long ecx = 1; /* break on interrupt flag */
+	struct cpuidle_state *state = &drv->states[index];
+	unsigned long eax = flg2MWAIT(state->flags);
+
+	if (state->flags & CPUIDLE_FLAG_INIT_XSTATE)
+		fpu_idle_fpregs();
 
 	mwait_idle_with_hints(eax, ecx);
 
@@ -795,7 +808,8 @@ static struct cpuidle_state spr_cstates[] __initdata = {
 	{
 		.name = "C6",
 		.desc = "MWAIT 0x20",
-		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
+		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED |
+					   CPUIDLE_FLAG_INIT_XSTATE,
 		.exit_latency = 290,
 		.target_residency = 800,
 		.enter = &intel_idle,
-- 
2.17.1

Re: [PATCH v4 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle

[Dave Hansen]

On 5/17/22 15:24, Chang S. Bae wrote:
> +/*
> + * Initialize register state that may prevent from entering low-power idle.
> + * This function will be invoked from the cpuidle driver only when needed.
> + */
> +void fpu_idle_fpregs(void)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_XGETBV1) &&
> +	    (xfeatures_in_use() & XFEATURE_MASK_XTILE)) {
> +		tile_release();
> +		fpregs_deactivate(&current->thread.fpu);
> +	}
> +}

This is a pretty minor nit, but:

X86_FEATURE_XFD depends on X86_FEATURE_XGETBV1

and

X86_FEATURE_AMX_TILE depends on X86_FEATURE_XFD

via cpu_deps[].  So there is an implicit dependency all the way from AMX
to XGETBV1.  It's also not patently obvious what X86_FEATURE_XGETBV1 has
to do with the rest of the if().

Would this make more logical sense to folks?

	/* Note: AMX_TILE being enabled implies XGETBV1 support */
	if (cpu_feature_enabled(X86_FEATURE_AMX_TILE) &&
	    (xfeatures_in_use() & XFEATURE_MASK_XTILE)) {
		tile_release();
		fpregs_deactivate(&current->thread.fpu);
	}

That also has a nice side effect that non-AMX systems will get to use a
static branch and can also skip over the XGETBV1 entirely.

The downside is that there's no explicit XGETBV1 check before calling
xfeatures_in_use().  But, I don't really expect the AMX->XGETBV1
dependency to go away either.

Re: [PATCH v4 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle

[Chang S. Bae]

On 5/18/2022 8:41 AM, Dave Hansen wrote:
> 
> This is a pretty minor nit, but:
> 
> X86_FEATURE_XFD depends on X86_FEATURE_XGETBV1
> 
> and
> 
> X86_FEATURE_AMX_TILE depends on X86_FEATURE_XFD
> 
> via cpu_deps[].  So there is an implicit dependency all the way from AMX
> to XGETBV1.  It's also not patently obvious what X86_FEATURE_XGETBV1 has
> to do with the rest of the if().
> 
> Would this make more logical sense to folks?
> 
> 	/* Note: AMX_TILE being enabled implies XGETBV1 support */
> 	if (cpu_feature_enabled(X86_FEATURE_AMX_TILE) &&
> 	    (xfeatures_in_use() & XFEATURE_MASK_XTILE)) {
> 		tile_release();
> 		fpregs_deactivate(&current->thread.fpu);
> 	}

With the note, I guess people will have no problem with AMX->XGETBV1. 
But I would leave this question to others who can tell.

> 
> That also has a nice side effect that non-AMX systems will get to use a
> static branch and can also skip over the XGETBV1 entirely.

Yes, but FWIW, as it is non-architectural, the function should be 
consumed only by drivers for AMX systems.

> 
> The downside is that there's no explicit XGETBV1 check before calling
> xfeatures_in_use().  But, I don't really expect the AMX->XGETBV1
> dependency to go away either.

Yes, as long as AMX is wanted as a dynamic feature I think.

Thanks,
Chang

Re: [PATCH v4 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle

[Dave Hansen]

On 5/18/22 10:20, Chang S. Bae wrote:
>> That also has a nice side effect that non-AMX systems will get to use a
>> static branch and can also skip over the XGETBV1 entirely.
> 
> Yes, but FWIW, as it is non-architectural, the function should be
> consumed only by drivers for AMX systems.

Oh, that's a good point.  The new flag is set only on Sapphire Rapids
systems so the new code is only called there as well.  I guess it would
only matter if we end up having systems that need this where AMX isn't
universally available, like if it were fused on or off on certain SKUs
(I have no idea if anyone is actually doing this).