Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 18 Sep 2018 11:50:15 +0200
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
Message-ID: <20180918095015.GE19234@hirez.programming.kicks-ass.net>
References: <87411705-893f-46d3-b899-b09ed9fa8d1b@default>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87411705-893f-46d3-b899-b09ed9fa8d1b@default>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.