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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1980c78b-d51b-c186-9179-f3c72692ad8a@kernel.org>
Date:   Sun, 23 May 2021 20:13:38 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>, bp@...e.de,
        tglx@...utronix.de, mingo@...nel.org, x86@...nel.org
Cc:     len.brown@...el.com, dave.hansen@...el.com, jing2.liu@...el.com,
        ravi.v.shankar@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 28/28] x86/fpu/amx: Clear the AMX state when
 appropriate

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@...el.com>
> Reviewed-by: Len Brown <len.brown@...el.com>
> Cc: x86@...nel.org
> Cc: linux-kernel@...r.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