[MODERATED] Re: [patch V7 14/15] SBB 14

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

..snip..
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -42,7 +42,8 @@
>  #define MSR_IA32_SPEC_CTRL		0x00000048 /* Speculation Control */
>  #define SPEC_CTRL_IBRS			(1 << 0)   /* Indirect Branch Restricted Speculation */
>  #define SPEC_CTRL_STIBP			(1 << 1)   /* Single Thread Indirect Branch Predictors */
> -#define SPEC_CTRL_RDS			(1 << 2)   /* Reduced Data Speculation */
> +#define SPEC_CTRL_RDS_SHIFT		2	   /* Reduced Data Speculation bit */
> +#define SPEC_CTRL_RDS			(1 << SPEC_CTRL_RDS_SHIFT)   /* Reduced Data Speculation */
>  
>  #define MSR_IA32_PRED_CMD		0x00000049 /* Prediction Command */
>  #define PRED_CMD_IBPB			(1 << 0)   /* Indirect Branch Prediction Barrier */
> --- a/arch/x86/include/asm/specctrl.h
> +++ b/arch/x86/include/asm/specctrl.h
> @@ -2,6 +2,7 @@
>  #ifndef _ASM_X86_SPECCTRL_H_
>  #define _ASM_X86_SPECCTRL_H_
>  
> +#include <linux/thread_info.h>
>  #include <asm/nospec-branch.h>
>  
>  /*
> @@ -18,4 +19,20 @@ extern void x86_restore_host_spec_ctrl(u
>  extern u64 x86_amd_ls_cfg_base;
>  extern u64 x86_amd_ls_cfg_rds_mask;
>  
> +/* The Intel SPEC CTRL MSR base value cache */
> +extern u64 x86_spec_ctrl_base;
> +
> +static inline u64 rds_tif_to_spec_ctrl(u64 tifn)
> +{
> +	BUILD_BUG_ON(TIF_RDS < SPEC_CTRL_RDS_SHIFT);
> +	return (tifn & _TIF_RDS) >> (TIF_RDS - SPEC_CTRL_RDS_SHIFT);

If my math is correct, the right side value is 3, not 2. That is
TIF_RDS (5) - SPEC_CTRL_RDS_SHIFT(2) = 3.

Then if _TIF_RDS is set we do:
	1 >> 3

which is zero. Hm, did you mean to shift it to left? That would
be 8 (also incorrect).

If _TIF_RDS is unset we do:
	0 >> 3

which is also zero.
>  void x86_set_guest_spec_ctrl(u64 guest_spec_ctrl)
>  {
> +	u64 host = x86_spec_ctrl_base;
> +
>  	if (!boot_cpu_has(X86_FEATURE_IBRS))
>  		return;
> -	if (x86_spec_ctrl_base != guest_spec_ctrl)
> +
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		host |=  rds_tif_to_spec_ctrl(current_thread_info()->flags);

Here you have an extra space in front of the rds_tif_to_spec_ctrl.
> + 
> +	if (host != guest_spec_ctrl)
>  		wrmsrl(MSR_IA32_SPEC_CTRL, guest_spec_ctrl);
>  }
>  EXPORT_SYMBOL_GPL(x86_set_guest_spec_ctrl);
>  
>  void x86_restore_host_spec_ctrl(u64 guest_spec_ctrl)
>  {
> +	u64 host = x86_spec_ctrl_base;
> +
>  	if (!boot_cpu_has(X86_FEATURE_IBRS))
>  		return;
> -	if (x86_spec_ctrl_base != guest_spec_ctrl)
> -		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> +
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		host |=  rds_tif_to_spec_ctrl(current_thread_info()->flags);

Ditto here - also an extra space in front of the rds_tif_to_spec_ctrl.

> +
> +	if (host != guest_spec_ctrl)
> +		wrmsrl(MSR_IA32_SPEC_CTRL, host);
>  }
>  EXPORT_SYMBOL_GPL(x86_restore_host_spec_ctrl);
>  
>  static void x86_amd_rds_enable(void)
>  {
> -	u64 msrval = x86_amd_ls_cfg_base | x86_amd_ls_cfg_rds_bit);
> +	u64 msrval = x86_amd_ls_cfg_base | x86_amd_ls_cfg_rds_mask;

Ah, here is the fix for that ')'
>  
>  	if (boot_cpu_has(X86_FEATURE_AMD_RDS))
>  		wrmsrl(MSR_AMD64_LS_CFG, msrval);
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -38,6 +38,7 @@
>  #include <asm/switch_to.h>
>  #include <asm/desc.h>
>  #include <asm/prctl.h>
> +#include <asm/specctrl.h>
>  
>  /*
>   * per-CPU TSS segments. Threads are completely 'soft' on Linux,
> @@ -278,6 +279,24 @@ static inline void switch_to_bitmap(stru
>  	}
>  }
>  
> +static __always_inline void __speculative_store_bypass_update(unsigned long tifn)
> +{
> +	u64 msr;
> +
> +	if (static_cpu_has(X86_FEATURE_AMD_RDS)) {
> +		msr = x86_amd_ls_cfg_base | rds_tif_to_amd_ls_cfg(tifn);
> +		wrmsrl(MSR_AMD64_LS_CFG, msr);
> +	} else {

Would it make sense to test for 'X86_FEATURE_RDS' here? Let me double-check
the next patch - you probably have some check there where the _TIF_RDS never
gets set if X86_FEATURE_RDS does not exist.

> +		msr = x86_spec_ctrl_base | rds_tif_to_spec_ctrl(tifn);
> +		wrmsrl(MSR_IA32_SPEC_CTRL, msr);
> +	}
> +}
> +
> +void speculative_store_bypass_update(void)
> +{
> +	__speculative_store_bypass_update(current_thread_info()->flags);
> +}
> +
>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>  		      struct tss_struct *tss)
>  {
> @@ -309,6 +328,9 @@ void __switch_to_xtra(struct task_struct
>  
>  	if ((tifp ^ tifn) & _TIF_NOCPUID)
>  		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
> +
> +	if ((tifp ^ tifn) & _TIF_RDS)
> +		__speculative_store_bypass_update(tifn);
>  }
>  
>  /*
>