From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from userp2130.oracle.com ([156.151.31.86]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fCyLJ-0006qc-4N for speck@linutronix.de; Mon, 30 Apr 2018 04:15:05 +0200 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w3U25wwU144365 for ; Mon, 30 Apr 2018 02:14:58 GMT Received: from aserv0021.oracle.com (aserv0021.oracle.com [141.146.126.233]) by userp2130.oracle.com with ESMTP id 2hmgdjabfm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 30 Apr 2018 02:14:58 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w3U2Ev4i021605 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 30 Apr 2018 02:14:57 GMT Received: from abhmp0015.oracle.com (abhmp0015.oracle.com [141.146.116.21]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w3U2Euhr010669 for ; Mon, 30 Apr 2018 02:14:56 GMT Date: Sun, 29 Apr 2018 22:14:55 -0400 From: Konrad Rzeszutek Wilk Subject: [MODERATED] Re: [patch V7 14/15] SBB 14 Message-ID: <20180430021455.GA30984@char.us.oracle.com> References: <20180429193045.711908246@linutronix.de> <20180429193938.637125129@linutronix.de> MIME-Version: 1.0 In-Reply-To: <20180429193938.637125129@linutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: ..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 > #include > > /* > @@ -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 > #include > #include > +#include > > /* > * 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); > } > > /* >