Re: [PATCH] x86/speculation: Use AMD specific retpoline for inline asm on AMD

From: Peter Zijlstra <peterz@infradead.org>
To: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: linux-kernel@vger.kernel.org, mingo@redhat.com,
	konrad.wilk@oracle.com, x86@kernel.org, dwmw@amazon.co.uk,
	tglx@linutronix.de,
	Srinivas REDDY Eeda <srinivas.eeda@oracle.com>,
	bp@suse.de, hpa@zytor.com, dhaval.giani@oracle.com
Subject: Re: [PATCH] x86/speculation: Use AMD specific retpoline for inline asm on AMD
Date: Tue, 18 Sep 2018 11:50:15 +0200	[thread overview]
Message-ID: <20180918095015.GE19234@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <87411705-893f-46d3-b899-b09ed9fa8d1b@default>

On Mon, Sep 17, 2018 at 10:17:30PM -0700, Zhenzhong Duan wrote:
> Lfence is preferred than general retpoline on AMD, add this option
> in C / inline asm just as the ASM code does.
> 
> For x86_64, it still help to have minimal retpoline for kernel even
> if gcc doesn't support it, change the inline asm for x86 so that it
> could also be used by x86_64.
> Add ANNOTATE_NOSPEC_ALTERNATIVE for i386 to avoid below warning:
> "warning: objtool: .altinstr_replacement+0x10: unsupported
> intra-function call"
> "warning: objtool: If this is a retpoline, please patch it
> in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE."

This Changelog is almost unreadable, please rewrite.

Reverse engineering the patch you add RETPOLINE_AMD support to the
inline-asm CALL_NOSPEC so that they match the asm CALL_NOSPEC.

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
>  arch/x86/include/asm/nospec-branch.h |   23 ++++++++++++++++-------
>  1 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index fd2a8c1..2d49eab 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -170,21 +170,26 @@
>   */
>  # define CALL_NOSPEC						\
>  	ANNOTATE_NOSPEC_ALTERNATIVE				\
> -	ALTERNATIVE(						\
> +	ALTERNATIVE_2(						\
>  	ANNOTATE_RETPOLINE_SAFE					\
>  	"call *%[thunk_target]\n",				\
>  	"call __x86_indirect_thunk_%V[thunk_target]\n",		\
> -	X86_FEATURE_RETPOLINE)
> +	X86_FEATURE_RETPOLINE,					\
> +	"lfence;\n"						\
> +	ANNOTATE_RETPOLINE_SAFE					\
> +	"call *%[thunk_target]\n",				\
> +	X86_FEATURE_RETPOLINE_AMD)
>  # define THUNK_TARGET(addr) [thunk_target] "r" (addr)

That's OK.

>  
> -#elif defined(CONFIG_X86_32) && defined(CONFIG_RETPOLINE)
> +#elif defined(CONFIG_RETPOLINE)

This doesn't make any sense..

>  /*
>   * For i386 we use the original ret-equivalent retpoline, because
>   * otherwise we'll run out of registers. We don't care about CET
>   * here, anyway.
>   */
>  # define CALL_NOSPEC						\
> -	ALTERNATIVE(						\
> +	ANNOTATE_NOSPEC_ALTERNATIVE				\
> +	ALTERNATIVE_2(						\
>  	ANNOTATE_RETPOLINE_SAFE					\
>  	"call *%[thunk_target]\n",				\
>  	"       jmp    904f;\n"					\
> @@ -194,12 +199,16 @@
>  	"    	lfence;\n"					\
>  	"       jmp    902b;\n"					\
>  	"       .align 16\n"					\
> -	"903:	addl   $4, %%esp;\n"				\
> -	"       pushl  %[thunk_target];\n"			\
> +	"903:	add   $4, %%" _ASM_SP ";\n"			\
> +	"       push  %[thunk_target];\n"			\

Yeah, don't do that.

>  	"       ret;\n"						\
>  	"       .align 16\n"					\
>  	"904:	call   901b;\n",				\
> -	X86_FEATURE_RETPOLINE)
> +	X86_FEATURE_RETPOLINE,					\
> +	"lfence;\n"						\
> +	ANNOTATE_RETPOLINE_SAFE					\
> +	"call *%[thunk_target]\n",				\
> +	X86_FEATURE_RETPOLINE_AMD)

And that's OK again.