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

[Andy Lutomirski]

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@xxxxxxxxx>
> Reviewed-by: Len Brown <len.brown@xxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> 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