Re: [PATCH v5 28/28] x86/fpu/amx: Clear the AMX state when appropriate

Andy Lutomirski
2021-05-24T03:13:42Z

On 5/23/21 12:32 PM, Chang S. Bae wrote:
> When AMX is enabled, and an AMX-task is saved, explicitly initialize the
> AMX state after the XSAVE.
>
> This assures that the kernel will only request idle states with clean AMX
> state. In the case of the C6 idle state, this allows the hardware to get to
> a deeper power saving condition.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Changes from v4:
> * Added as a new patch. (Thomas Gleixner)
> ---
> arch/x86/include/asm/special_insns.h | 6 ++++++
> arch/x86/kernel/fpu/core.c | 8 ++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 2acd6cb62328..f0ed063035eb 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -306,6 +306,12 @@ 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 >= 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 cccfeafe81e5..53a5869078b8 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -106,6 +106,14 @@ int copy_fpregs_to_fpstate(struct fpu *fpu)
> */
> if (fpu->state->xsave.header.xfeatures & XFEATURE_MASK_AVX512)
> fpu->avx512_timestamp = jiffies;
> +
> + /*
> + * Since the current task's state is safely in the XSAVE buffer, TILERELEASE
> + * the TILE registers to guarantee that dirty state will not interfere with the
> + * hardware's ability to enter the core C6 idle state.
> + */
> + if (fpu->state_mask & XFEATURE_MASK_XTILE_DATA)
> + tile_release();
> return 1;
> }
>
>

This looks wrong -- you should also invalidate the state. And doing it
in the save path seems inefficient.

Can we do this just when going idle?

--Andy