| Message ID | 20220517222430.24524-2-chang.seok.bae@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | x86/fpu: Make AMX state ready for CPU idle | expand |
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(¤t->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(¤t->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.
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(¤t->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
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).
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(¤t->thread.fpu); + } +}
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(+)