Re: [PATCH 1/3] x86 msr: msr goto extension support

[Andy Lutomirski <luto@amacapital.net>]

On 07/31/2014 02:41 AM, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> Currently, {rd,wr}msrl_safe can handle the exception which caused by accessing
> specific MSR.
> However, it will introduce extra conditional branch for testing errors. That
> will impact the "fast" path's performance.
> The newly implemented {rd,wr}msrl_goto function can not only handle the
> exception which caused by accessing specific MSR,
> but also takes advantage of the asm goto extension to eliminate the impact of
> performance.
> 
> The asm goto extension is supported by GCC 4.5 and later versions. If the
> compiler doesn't support goto extension, _safe will be used to replace _goto.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  arch/x86/include/asm/msr.h      | 60 +++++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/paravirt.h | 18 +++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index de36f22..55438da 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -203,6 +203,66 @@ do {                                                            \
>  
>  #define rdtscpll(val, aux) (val) = native_read_tscp(&(aux))
>  
> +#ifdef CC_HAVE_ASM_GOTO
> +
> +/*
> + * The _goto version is rdmsrl/wrmsrl with exception handling
> + * The advantage (than _safe) is that it can directly jump in the
> + * exception handling code, and never test in the "fast" path.
> + *
> + * Since _goto doesn't support output, try to protect the output
> + * registers by clobbers, and process the registers immediately.
> + */
> +#define rdmsrl_goto(msr, result, fail_label)			\
> +do {								\
> +	DECLARE_ARGS(val, low, high);				\
> +	asm_volatile_goto("2: rdmsr\n"				\
> +			"1:\n\t"				\
> +			_ASM_EXTABLE(2b, %l[fail_label])	\
> +			: /* No outputs. */			\
> +			: "c" (msr)				\
> +			: "%rax", "%rdx"			\
> +			: fail_label);				\
> +	asm volatile (""					\
> +			: EAX_EDX_RET(val, low, high)		\
> +			: );					\

This is scary -- the compiler is free to optimize this incorrectly, and
it doesn't even seem very farfetched to me.

> +	result = EAX_EDX_VAL(val, low, high);			\
> +} while (0)
> +
> +#define wrmsrl_goto(msr, val, fail_label)			\
> +do {								\
> +	unsigned low, high;					\
> +	low = (u32)val;						\
> +	high = (u32)(val >> 32);				\
> +	asm_volatile_goto("2: wrmsr\n"				\
> +			"1:\n\t"				\
> +			_ASM_EXTABLE(2b, %l[fail_label])	\
> +			: /* No outputs. */			\
> +			: "c" (msr), "a" (low), "d" (high)	\
> +			: "memory"				\
> +			: fail_label);				\
> +} while (0)

I like this one.

--Andy