The following patches are based on v3 from Tim Chen
https://www.mail-archive.com/[email protected]/msg1582043.html
This patch set supports exposing MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD
for user space.
Thomas is steam blowing v3 :-).. but I didn't want to keep holding this
much longer for the rebase to be complete in tip/x86/pti.
Ashok Raj (4):
x86/ibrs: Introduce native_rdmsrl, and native_wrmsrl
x86/ibrs: Add new helper macros to save/restore MSR_IA32_SPEC_CTRL
x86/ibrs: Add direct access support for MSR_IA32_SPEC_CTRL
x86/feature: Detect the x86 feature Indirect Branch Prediction Barrier
Paolo Bonzini (1):
x86/svm: Direct access to MSR_IA32_SPEC_CTRL
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/msr-index.h | 3 +++
arch/x86/include/asm/spec_ctrl.h | 29 +++++++++++++++++++++-
arch/x86/kernel/cpu/spec_ctrl.c | 19 ++++++++++++++
arch/x86/kvm/cpuid.c | 3 ++-
arch/x86/kvm/svm.c | 51 ++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx.c | 51 ++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/x86.c | 1 +
8 files changed, 156 insertions(+), 2 deletions(-)
--
2.7.4
- Remove including microcode.h, and use native macros from asm/msr.h
- added license header for spec_ctrl.c
Signed-off-by: Ashok Raj <[email protected]>
---
arch/x86/include/asm/spec_ctrl.h | 17 ++++++++++++++++-
arch/x86/kernel/cpu/spec_ctrl.c | 1 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
index 948959b..2dfa31b 100644
--- a/arch/x86/include/asm/spec_ctrl.h
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -3,12 +3,27 @@
#ifndef _ASM_X86_SPEC_CTRL_H
#define _ASM_X86_SPEC_CTRL_H
-#include <asm/microcode.h>
+#include <asm/processor.h>
+#include <asm/msr.h>
void spec_ctrl_scan_feature(struct cpuinfo_x86 *c);
void spec_ctrl_unprotected_begin(void);
void spec_ctrl_unprotected_end(void);
+static inline u64 native_rdmsrl(unsigned int msr)
+{
+ u64 val;
+
+ val = __rdmsr(msr);
+
+ return val;
+}
+
+static inline void native_wrmsrl(unsigned int msr, u64 val)
+{
+ __wrmsr(msr, (u32) (val & 0xffffffffULL), (u32) (val >> 32));
+}
+
static inline void __disable_indirect_speculation(void)
{
native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_ENABLE_IBRS);
diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
index 843b4e6..9e9d013 100644
--- a/arch/x86/kernel/cpu/spec_ctrl.c
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
#include <linux/string.h>
#include <asm/spec_ctrl.h>
--
2.7.4
From: Paolo Bonzini <[email protected]>
Direct access to MSR_IA32_SPEC_CTRL is important
for performance. Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set restore host values on VM exit.
it yet).
TBD: need to check msr's can be passed through even if feature is not
emuerated by the CPU.
[Ashok: Modified to reuse V3 spec-ctrl patches from Tim]
Signed-off-by: Paolo Bonzini <[email protected]>
Signed-off-by: Ashok Raj <[email protected]>
---
arch/x86/kvm/svm.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0e68f0b..7c14471a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -183,6 +183,8 @@ struct vcpu_svm {
u64 gs_base;
} host;
+ u64 spec_ctrl;
+
u32 *msrpm;
ulong nmi_iret_rip;
@@ -248,6 +250,7 @@ static const struct svm_direct_access_msrs {
{ .index = MSR_CSTAR, .always = true },
{ .index = MSR_SYSCALL_MASK, .always = true },
#endif
+ { .index = MSR_IA32_SPEC_CTRL, .always = true },
{ .index = MSR_IA32_LASTBRANCHFROMIP, .always = false },
{ .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
{ .index = MSR_IA32_LASTINTFROMIP, .always = false },
@@ -917,6 +920,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
}
+
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+ set_msr_interception(msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
}
static void add_msr_offset(u32 offset)
@@ -3576,6 +3582,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_VM_CR:
msr_info->data = svm->nested.vm_cr_msr;
break;
+ case MSR_IA32_SPEC_CTRL:
+ msr_info->data = svm->spec_ctrl;
+ break;
case MSR_IA32_UCODE_REV:
msr_info->data = 0x01000065;
break;
@@ -3724,6 +3733,9 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
case MSR_VM_IGNNE:
vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
break;
+ case MSR_IA32_SPEC_CTRL:
+ svm->spec_ctrl = data;
+ break;
case MSR_IA32_APICBASE:
if (kvm_vcpu_apicv_active(vcpu))
avic_update_vapic_bar(to_svm(vcpu), data);
@@ -4871,6 +4883,19 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
svm_complete_interrupts(svm);
}
+
+/*
+ * Save guest value of spec_ctrl and also restore host value
+ */
+static void save_guest_spec_ctrl(struct vcpu_svm *svm)
+{
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+ svm->spec_ctrl = spec_ctrl_get();
+ spec_ctrl_restriction_on();
+ } else
+ rmb();
+}
+
static void svm_vcpu_run(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
@@ -4910,6 +4935,14 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
clgi();
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+ /*
+ * FIXME: lockdep_assert_irqs_disabled();
+ */
+ WARN_ON_ONCE(!irqs_disabled());
+ spec_ctrl_set(svm->spec_ctrl);
+ }
+
local_irq_enable();
asm volatile (
@@ -4985,6 +5018,8 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
#endif
);
+ save_guest_spec_ctrl(svm);
+
#ifdef CONFIG_X86_64
wrmsrl(MSR_GS_BASE, svm->host.gs_base);
#else
--
2.7.4
cpuid ax=0x7, return rdx bit 26 to indicate presence of both
IA32_SPEC_CTRL(MSR 0x48) and IA32_PRED_CMD(MSR 0x49)
BIT0: Indirect Branch Prediction Barrier
When this MSR is written with IBPB=1 it ensures that earlier code's behavior
doesn't control later indirect branch predictions.
Note this MSR is only writable and does not carry any state. Its a barrier
so the code should perform a wrmsr when the barrier is needed.
Signed-off-by: Ashok Raj <[email protected]>
---
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/msr-index.h | 3 +++
arch/x86/kernel/cpu/spec_ctrl.c | 7 +++++++
arch/x86/kvm/svm.c | 16 ++++++++++++++++
arch/x86/kvm/vmx.c | 10 ++++++++++
5 files changed, 37 insertions(+)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 624b58e..52f37fc 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -213,6 +213,7 @@
#define X86_FEATURE_MBA ( 7*32+18) /* Memory Bandwidth Allocation */
#define X86_FEATURE_SPEC_CTRL ( 7*32+19) /* Speculation Control */
#define X86_FEATURE_SPEC_CTRL_IBRS ( 7*32+20) /* Speculation Control, use IBRS */
+#define X86_FEATURE_PRED_CMD ( 7*32+21) /* Indirect Branch Prediction Barrier */
/* Virtualization flags: Linux defined, word 8 */
#define X86_FEATURE_TPR_SHADOW ( 8*32+ 0) /* Intel TPR Shadow */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3e1cb18..1888e19 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -46,6 +46,9 @@
#define SPEC_CTRL_DISABLE_IBRS (0 << 0)
#define SPEC_CTRL_ENABLE_IBRS (1 << 0)
+#define MSR_IA32_PRED_CMD 0x00000049
+#define FEATURE_SET_IBPB (1<<0)
+
#define MSR_IA32_PERFCTR0 0x000000c1
#define MSR_IA32_PERFCTR1 0x000000c2
#define MSR_FSB_FREQ 0x000000cd
diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
index 02fc630..6cfec19 100644
--- a/arch/x86/kernel/cpu/spec_ctrl.c
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -15,6 +15,13 @@ void spec_ctrl_scan_feature(struct cpuinfo_x86 *c)
if (!c->cpu_index)
static_branch_enable(&spec_ctrl_dynamic_ibrs);
}
+ /*
+ * For Intel CPU's this MSR is shared the same cpuid
+ * enumeration. When MSR_IA32_SPEC_CTRL is present
+ * MSR_IA32_SPEC_CTRL is also available
+ * TBD: AMD might have a separate enumeration for each.
+ */
+ set_cpu_cap(c, X86_FEATURE_PRED_CMD);
}
}
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 7c14471a..36924c9 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -251,6 +251,7 @@ static const struct svm_direct_access_msrs {
{ .index = MSR_SYSCALL_MASK, .always = true },
#endif
{ .index = MSR_IA32_SPEC_CTRL, .always = true },
+ { .index = MSR_IA32_PRED_CMD, .always = false },
{ .index = MSR_IA32_LASTBRANCHFROMIP, .always = false },
{ .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
{ .index = MSR_IA32_LASTINTFROMIP, .always = false },
@@ -531,6 +532,7 @@ struct svm_cpu_data {
struct kvm_ldttss_desc *tss_desc;
struct page *save_area;
+ struct vmcb *current_vmcb;
};
static DEFINE_PER_CPU(struct svm_cpu_data *, svm_data);
@@ -923,6 +925,8 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
set_msr_interception(msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
+ if (boot_cpu_has(X86_FEATURE_PRED_CMD))
+ set_msr_interception(msrpm, MSR_IA32_PRED_CMD, 1, 1);
}
static void add_msr_offset(u32 offset)
@@ -1711,11 +1715,18 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
__free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
kvm_vcpu_uninit(vcpu);
kmem_cache_free(kvm_vcpu_cache, svm);
+ /*
+ * The VMCB could be recycled, causing a false negative in svm_vcpu_load;
+ * block speculative execution.
+ */
+ if (boot_cpu_has(X86_FEATURE_PRED_CMD))
+ native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
}
static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
+ struct svm_cpu_data *sd = per_cpu(svm_data, cpu);
int i;
if (unlikely(cpu != vcpu->cpu)) {
@@ -1744,6 +1755,11 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
if (static_cpu_has(X86_FEATURE_RDTSCP))
wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
+ if (sd->current_vmcb != svm->vmcb) {
+ sd->current_vmcb = svm->vmcb;
+ if (boot_cpu_has(X86_FEATURE_PRED_CMD))
+ native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
+ }
avic_vcpu_load(vcpu, cpu);
}
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 1913896..caeb9ff 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2280,6 +2280,8 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
vmcs_load(vmx->loaded_vmcs->vmcs);
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+ native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
}
if (!already_loaded) {
@@ -3837,6 +3839,12 @@ static void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
free_vmcs(loaded_vmcs->vmcs);
loaded_vmcs->vmcs = NULL;
WARN_ON(loaded_vmcs->shadow_vmcs != NULL);
+ /*
+ * The VMCS could be recycled, causing a false negative in vmx_vcpu_load
+ * block speculative execution.
+ */
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+ native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
}
static void free_kvm_area(void)
@@ -6804,6 +6812,8 @@ static __init int hardware_setup(void)
*/
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+ if (boot_cpu_has(X86_FEATURE_PRED_CMD))
+ vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
vmx_disable_intercept_for_msr(MSR_FS_BASE, false);
vmx_disable_intercept_for_msr(MSR_GS_BASE, false);
--
2.7.4
Add direct access to MSR_IA32_SPEC_CTRL from a guest. Also save/restore
IBRS values during exits and guest resume path.
Rebasing based on Tim's patch
Signed-off-by: Ashok Raj <[email protected]>
---
arch/x86/kvm/cpuid.c | 3 ++-
arch/x86/kvm/vmx.c | 41 +++++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/x86.c | 1 +
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0099e10..6fa81c7 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -70,6 +70,7 @@ u64 kvm_supported_xcr0(void)
/* These are scattered features in cpufeatures.h. */
#define KVM_CPUID_BIT_AVX512_4VNNIW 2
#define KVM_CPUID_BIT_AVX512_4FMAPS 3
+#define KVM_CPUID_BIT_SPEC_CTRL 26
#define KF(x) bit(KVM_CPUID_BIT_##x)
int kvm_update_cpuid(struct kvm_vcpu *vcpu)
@@ -392,7 +393,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
/* cpuid 7.0.edx*/
const u32 kvm_cpuid_7_0_edx_x86_features =
- KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS);
+ KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS) | KF(SPEC_CTRL);
/* all calls to cpuid_count() should be made on the same cpu */
get_cpu();
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 62ee436..1913896 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -50,6 +50,7 @@
#include <asm/apic.h>
#include <asm/irq_remapping.h>
#include <asm/mmu_context.h>
+#include <asm/spec_ctrl.h>
#include "trace.h"
#include "pmu.h"
@@ -579,6 +580,7 @@ struct vcpu_vmx {
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
u32 secondary_exec_control;
+ u64 spec_ctrl;
/*
* loaded_vmcs points to the VMCS currently used in this vcpu. For a
@@ -3259,6 +3261,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_IA32_TSC:
msr_info->data = guest_read_tsc(vcpu);
break;
+ case MSR_IA32_SPEC_CTRL:
+ msr_info->data = to_vmx(vcpu)->spec_ctrl;
+ break;
case MSR_IA32_SYSENTER_CS:
msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
break;
@@ -3366,6 +3371,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_IA32_TSC:
kvm_write_tsc(vcpu, msr_info);
break;
+ case MSR_IA32_SPEC_CTRL:
+ to_vmx(vcpu)->spec_ctrl = msr_info->data;
+ break;
case MSR_IA32_CR_PAT:
if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -6790,6 +6798,13 @@ static __init int hardware_setup(void)
kvm_tsc_scaling_ratio_frac_bits = 48;
}
+ /*
+ * If feature is available then setup MSR_IA32_SPEC_CTRL to be in
+ * passthrough mode for the guest.
+ */
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+ vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+
vmx_disable_intercept_for_msr(MSR_FS_BASE, false);
vmx_disable_intercept_for_msr(MSR_GS_BASE, false);
vmx_disable_intercept_for_msr(MSR_KERNEL_GS_BASE, true);
@@ -9242,6 +9257,15 @@ static void vmx_arm_hv_timer(struct kvm_vcpu *vcpu)
vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, delta_tsc);
}
+static void save_guest_spec_ctrl(struct vcpu_vmx *vmx)
+{
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+ vmx->spec_ctrl = spec_ctrl_get();
+ spec_ctrl_restriction_on();
+ } else
+ rmb();
+}
+
static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -9298,6 +9322,21 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
vmx_arm_hv_timer(vcpu);
vmx->__launched = vmx->loaded_vmcs->launched;
+
+ /*
+ * Just update whatever the value was set for the MSR in guest.
+ * If this is unlaunched: Assume that initialized value is 0.
+ * IRQ's also need to be disabled. If guest value is 0, an interrupt
+ * could start running in unprotected mode (i.e with IBRS=0).
+ */
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+ /*
+ * FIXME: lockdep_assert_irqs_disabled();
+ */
+ WARN_ON_ONCE(!irqs_disabled());
+ spec_ctrl_set(vmx->spec_ctrl);
+ }
+
asm(
/* Store host registers */
"push %%" _ASM_DX "; push %%" _ASM_BP ";"
@@ -9403,6 +9442,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
#endif
);
+ save_guest_spec_ctrl(vmx);
+
/* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
if (debugctlmsr)
update_debugctlmsr(debugctlmsr);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 03869eb..9ffb9d6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1006,6 +1006,7 @@ static u32 msrs_to_save[] = {
#endif
MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
MSR_IA32_FEATURE_CONTROL, MSR_IA32_BNDCFGS, MSR_TSC_AUX,
+ MSR_IA32_SPEC_CTRL,
};
static unsigned num_msrs_to_save;
--
2.7.4
Add some helper macros to save/restore MSR_IA32_SPEC_CTRL.
Although we could use the spec_ctrl_unprotected_begin/end macros they seem
be bit unreadable for some uses.
spec_ctrl_get - read MSR_IA32_SPEC_CTRL to save
spec_ctrl_set - write value restore MSR_IA32_SPEC_CTRL
spec_ctrl_restriction_off - same as spec_ctrl_unprotected_begin
spec_ctrl_restriction_on - same as spec_ctrl_unprotected_end
Signed-off-by: Ashok Raj <[email protected]>
---
arch/x86/include/asm/spec_ctrl.h | 12 ++++++++++++
arch/x86/kernel/cpu/spec_ctrl.c | 11 +++++++++++
2 files changed, 23 insertions(+)
diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
index 2dfa31b..926feb2 100644
--- a/arch/x86/include/asm/spec_ctrl.h
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -9,6 +9,10 @@
void spec_ctrl_scan_feature(struct cpuinfo_x86 *c);
void spec_ctrl_unprotected_begin(void);
void spec_ctrl_unprotected_end(void);
+void spec_ctrl_set(u64 val);
+
+#define spec_ctrl_restriction_on spec_ctrl_unprotected_end
+#define spec_ctrl_restriction_off spec_ctrl_unprotected_begin
static inline u64 native_rdmsrl(unsigned int msr)
{
@@ -34,4 +38,12 @@ static inline void __enable_indirect_speculation(void)
native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_DISABLE_IBRS);
}
+static inline u64 spec_ctrl_get(void)
+{
+ u64 val;
+
+ val = native_rdmsrl(MSR_IA32_SPEC_CTRL);
+
+ return val;
+}
#endif /* _ASM_X86_SPEC_CTRL_H */
diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
index 9e9d013..02fc630 100644
--- a/arch/x86/kernel/cpu/spec_ctrl.c
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -47,3 +47,14 @@ void spec_ctrl_unprotected_end(void)
__disable_indirect_speculation();
}
EXPORT_SYMBOL_GPL(spec_ctrl_unprotected_end);
+
+void spec_ctrl_set(u64 val)
+{
+ if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+ if (!val) {
+ spec_ctrl_restriction_off();
+ } else
+ spec_ctrl_restriction_on();
+ }
+}
+EXPORT_SYMBOL(spec_ctrl_set);
--
2.7.4
On Thu, Jan 11, 2018 at 5:32 PM, Ashok Raj <[email protected]> wrote:
> - Remove including microcode.h, and use native macros from asm/msr.h
> - added license header for spec_ctrl.c
>
> Signed-off-by: Ashok Raj <[email protected]>
> ---
> arch/x86/include/asm/spec_ctrl.h | 17 ++++++++++++++++-
> arch/x86/kernel/cpu/spec_ctrl.c | 1 +
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
> index 948959b..2dfa31b 100644
> --- a/arch/x86/include/asm/spec_ctrl.h
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -3,12 +3,27 @@
> #ifndef _ASM_X86_SPEC_CTRL_H
> #define _ASM_X86_SPEC_CTRL_H
>
> -#include <asm/microcode.h>
> +#include <asm/processor.h>
> +#include <asm/msr.h>
>
> void spec_ctrl_scan_feature(struct cpuinfo_x86 *c);
> void spec_ctrl_unprotected_begin(void);
> void spec_ctrl_unprotected_end(void);
>
> +static inline u64 native_rdmsrl(unsigned int msr)
> +{
> + u64 val;
> +
> + val = __rdmsr(msr);
> +
> + return val;
> +}
What's wrong with native_read_msr()?
> +
> +static inline void native_wrmsrl(unsigned int msr, u64 val)
> +{
> + __wrmsr(msr, (u32) (val & 0xffffffffULL), (u32) (val >> 32));
> +}
What's wrong with just wrmsrl()? If you really need a native helper
like this, please add it to arch/x86/asm/msr.h.
On Thu, Jan 11, 2018 at 05:41:34PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 11, 2018 at 5:32 PM, Ashok Raj <[email protected]> wrote:
> > - Remove including microcode.h, and use native macros from asm/msr.h
> > - added license header for spec_ctrl.c
> >
> > Signed-off-by: Ashok Raj <[email protected]>
[snip]
> > +static inline u64 native_rdmsrl(unsigned int msr)
> > +{
> > + u64 val;
> > +
> > + val = __rdmsr(msr);
> > +
> > + return val;
> > +}
>
> What's wrong with native_read_msr()?
Yes, i think i should have added to msr.h. The names didn't read as a
pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
defined?
>
> > +
> > +static inline void native_wrmsrl(unsigned int msr, u64 val)
> > +{
> > + __wrmsr(msr, (u32) (val & 0xffffffffULL), (u32) (val >> 32));
> > +}
>
> What's wrong with just wrmsrl()? If you really need a native helper
> like this, please add it to arch/x86/asm/msr.h.
I should have done that.. will move these to msr.h instead.
Cheers,
Ashok
On 01/11/2018 05:32 PM, Ashok Raj wrote:
> +static void save_guest_spec_ctrl(struct vcpu_vmx *vmx)
> +{
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> + vmx->spec_ctrl = spec_ctrl_get();
> + spec_ctrl_restriction_on();
> + } else
> + rmb();
> +}
Does this need to be "ifence()"? Better yet, do we just need to create
a helper for boot_cpu_has(X86_FEATURE_SPEC_CTRL) that does the barrier?
On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
> On Thu, Jan 11, 2018 at 05:41:34PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 11, 2018 at 5:32 PM, Ashok Raj <[email protected]> wrote:
>> > - Remove including microcode.h, and use native macros from asm/msr.h
>> > - added license header for spec_ctrl.c
>> >
>> > Signed-off-by: Ashok Raj <[email protected]>
>
> [snip]
>> > +static inline u64 native_rdmsrl(unsigned int msr)
>> > +{
>> > + u64 val;
>> > +
>> > + val = __rdmsr(msr);
>> > +
>> > + return val;
>> > +}
>>
>> What's wrong with native_read_msr()?
>
> Yes, i think i should have added to msr.h. The names didn't read as a
> pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
> defined?
Why do you need to override paravirt?
On Thu, Jan 11, 2018 at 06:20:13PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
> >>
> >> What's wrong with native_read_msr()?
> >
> > Yes, i think i should have added to msr.h. The names didn't read as a
> > pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
> > defined?
>
> Why do you need to override paravirt?
The idea was since these MSR's are passed through we shouldn't need to
handle them any differently. Also its best to do this as soon as possible
and avoid longer paths to get this barrier to hardware.
On Thu, Jan 11, 2018 at 05:58:11PM -0800, Dave Hansen wrote:
> On 01/11/2018 05:32 PM, Ashok Raj wrote:
> > +static void save_guest_spec_ctrl(struct vcpu_vmx *vmx)
> > +{
> > + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> > + vmx->spec_ctrl = spec_ctrl_get();
> > + spec_ctrl_restriction_on();
> > + } else
> > + rmb();
> > +}
>
> Does this need to be "ifence()"? Better yet, do we just need to create
> a helper for boot_cpu_has(X86_FEATURE_SPEC_CTRL) that does the barrier?
Yes... Didn't keep track of ifence() evolution :-)..
We could do a helper, will look into other uses and see we can make find a
common way to comprehend usages like above.
Cheers,
Ashok
On 01/11/2018 07:01 PM, Raj, Ashok wrote:
> On Thu, Jan 11, 2018 at 06:20:13PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
>>>>
>>>> What's wrong with native_read_msr()?
>>>
>>> Yes, i think i should have added to msr.h. The names didn't read as a
>>> pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
>>> defined?
>>
>> Why do you need to override paravirt?
>
> The idea was since these MSR's are passed through we shouldn't need to
> handle them any differently. Also its best to do this as soon as possible
> and avoid longer paths to get this barrier to hardware.
We were also worried about the indirect calls that are part of the
paravirt interfaces when retpolines are not in place.
On Thu, 2018-01-11 at 17:32 -0800, Ashok Raj wrote:
>
> @@ -4910,6 +4935,14 @@ static void svm_vcpu_run(struct kvm_vcpu
> *vcpu)
>
> clgi();
>
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> + /*
> + * FIXME: lockdep_assert_irqs_disabled();
> + */
> + WARN_ON_ONCE(!irqs_disabled());
> + spec_ctrl_set(svm->spec_ctrl);
> + }
> +
> local_irq_enable();
>
Same comments here as we've had previously. If you do this without an
'else lfence' then you need a comment showing that you've proved it's
safe.
And I don't think even using static_cpu_has() is good enough. We don't
already "rely" on that for anything but optimisations, AFAICT. Turning
a missed GCC optimisation into a security hole is not a good idea.
On Thu, Jan 11, 2018 at 05:32:15PM -0800, Ashok Raj wrote:
> - Remove including microcode.h, and use native macros from asm/msr.h
> - added license header for spec_ctrl.c
Worst changlog ever :(
Why are you touching spec_ctrl.c in this patch? How does it belong here
in this series?
Come on, you know better than this...
greg k-h
On Thu, Jan 11, 2018 at 05:58:11PM -0800, Dave Hansen wrote:
> On 01/11/2018 05:32 PM, Ashok Raj wrote:
> > +static void save_guest_spec_ctrl(struct vcpu_vmx *vmx)
> > +{
> > + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> > + vmx->spec_ctrl = spec_ctrl_get();
> > + spec_ctrl_restriction_on();
> > + } else
> > + rmb();
> > +}
>
> Does this need to be "ifence()"? Better yet, do we just need to create
> a helper for boot_cpu_has(X86_FEATURE_SPEC_CTRL) that does the barrier?
static_cpu_has() + hard asm-goto requirement. Please drop all the above
nonsense on the floor hard.
Let backporters sort out whever they need, don't introduce crap like
that upstream.
On Fri, Jan 12, 2018 at 07:23:53AM +0000, David Woodhouse wrote:
> On Thu, 2018-01-11 at 17:32 -0800, Ashok Raj wrote:
> >
> > @@ -4910,6 +4935,14 @@ static void svm_vcpu_run(struct kvm_vcpu
> > *vcpu)
> > ?
> > ????????clgi();
> > ?
> > +???????if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> > +???????????????/*
> > +??????????????? * FIXME: lockdep_assert_irqs_disabled();
> > +??????????????? */
> > +???????????????WARN_ON_ONCE(!irqs_disabled());
> > +???????????????spec_ctrl_set(svm->spec_ctrl);
> > +???????}
> > +
> > ????????local_irq_enable();
> > ?
>
> Same comments here as we've had previously. If you do this without an
> 'else lfence' then you need a comment showing that you've proved it's
> safe.
>
> And I don't think even using static_cpu_has() is good enough. We don't
> already "rely" on that for anything but optimisations, AFAICT. Turning
> a missed GCC optimisation into a security hole is not a good idea.
I disagree, and if you worry about that, we should write a testcase. But
we rely on GCC for correct code generation in lots of places, this isn't
different.
On Thu, Jan 11, 2018 at 05:32:19PM -0800, Ashok Raj wrote:
> @@ -1711,11 +1715,18 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
> __free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
> kvm_vcpu_uninit(vcpu);
> kmem_cache_free(kvm_vcpu_cache, svm);
> + /*
> + * The VMCB could be recycled, causing a false negative in svm_vcpu_load;
> + * block speculative execution.
> + */
> + if (boot_cpu_has(X86_FEATURE_PRED_CMD))
> + native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> }
> @@ -3837,6 +3839,12 @@ static void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
> free_vmcs(loaded_vmcs->vmcs);
> loaded_vmcs->vmcs = NULL;
> WARN_ON(loaded_vmcs->shadow_vmcs != NULL);
> + /*
> + * The VMCS could be recycled, causing a false negative in vmx_vcpu_load
> + * block speculative execution.
> + */
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> + native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> }
Whitespace damage.
Also, why not introduce a helper like:
static inline flush_ibpb(void)
{
if (static_cpu_has(X86_FEATURE_SPEC_CTRL))
native_write_msr(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
}
?
On Fri, 2018-01-12 at 10:51 +0100, Peter Zijlstra wrote:
> On Thu, Jan 11, 2018 at 05:58:11PM -0800, Dave Hansen wrote:
> > On 01/11/2018 05:32 PM, Ashok Raj wrote:
> > > +static void save_guest_spec_ctrl(struct vcpu_vmx *vmx)
> > > +{
> > > + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> > > + vmx->spec_ctrl = spec_ctrl_get();
> > > + spec_ctrl_restriction_on();
> > > + } else
> > > + rmb();
> > > +}
> >
> > Does this need to be "ifence()"? Better yet, do we just need to create
> > a helper for boot_cpu_has(X86_FEATURE_SPEC_CTRL) that does the barrier?
>
> static_cpu_has() + hard asm-goto requirement. Please drop all the above
> nonsense on the floor hard.
Peter, NO!
static_cpu_has() + asm-goto is NOT SUFFICIENT.
It's still *possible* for a missed optimisation in GCC to still leave
us with a conditional branch around the wrmsr, letting the CPU
speculate around it too.
Come on, Peter, we've learned this lesson long and hard since the 1990s
when every GCC update would break some stupid¹ shit we did. Don't
regress. WE DO NOT RELY ON UNGUARANTEED BEHAVIOUR OF THE COMPILER.
Devise a sanity check which will force a build-fail if GCC ever misses
the optimisation, and that's fine. (Or point me to an existing way that
it's guaranteed to fail, if such is true already. Don't just ignore the
objection.)
Do not just ASSUME that GCC will always and forever manage to make that
optimisation in every case.
--
dwmw2
¹ In our defence, back then there was often no *other* way to do some
of the things we did, except the "stupid" way. These days, it's much
easier to add features to GCC and elicit guarantees. Although I'm
getting rather pissed off at them bikeshedding the retpoline patches
*so* hard they can't even agree on a command-line option and the name
of the thunk symbol, regardless of the internal implementation
details we don't give a shit about.
On Fri, 2018-01-12 at 10:58 +0100, Peter Zijlstra wrote:
> I disagree, and if you worry about that, we should write a testcase. But
> we rely on GCC for correct code generation in lots of places, this isn't
> different.
It's different because it's not a *correctness* issue... unless we let
you make it one.
On Thu, Jan 11, 2018 at 05:32:15PM -0800, Ashok Raj wrote:
> - Remove including microcode.h, and use native macros from asm/msr.h
> - added license header for spec_ctrl.c
>
> Signed-off-by: Ashok Raj <[email protected]>
> ---
> arch/x86/include/asm/spec_ctrl.h | 17 ++++++++++++++++-
> arch/x86/kernel/cpu/spec_ctrl.c | 1 +
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
> index 948959b..2dfa31b 100644
> --- a/arch/x86/include/asm/spec_ctrl.h
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -3,12 +3,27 @@
> #ifndef _ASM_X86_SPEC_CTRL_H
> #define _ASM_X86_SPEC_CTRL_H
>
> -#include <asm/microcode.h>
> +#include <asm/processor.h>
> +#include <asm/msr.h>
>
> void spec_ctrl_scan_feature(struct cpuinfo_x86 *c);
> void spec_ctrl_unprotected_begin(void);
> void spec_ctrl_unprotected_end(void);
>
> +static inline u64 native_rdmsrl(unsigned int msr)
> +{
> + u64 val;
> +
> + val = __rdmsr(msr);
> +
> + return val;
> +}
There's no need to add new ones but simply lift the ones from
microcode.h and use them.
No duplication of *MSR functions pls.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Thu, Jan 11, 2018 at 05:32:19PM -0800, Ashok Raj wrote:
> cpuid ax=0x7, return rdx bit 26 to indicate presence of both
> IA32_SPEC_CTRL(MSR 0x48) and IA32_PRED_CMD(MSR 0x49)
So why do we need two X86_FEATURE flags then?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 12/01/2018 02:32, Ashok Raj wrote:
> From: Paolo Bonzini <[email protected]>
>
> Direct access to MSR_IA32_SPEC_CTRL is important
> for performance. Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set restore host values on VM exit.
> it yet).
>
> TBD: need to check msr's can be passed through even if feature is not
> emuerated by the CPU.
>
> [Ashok: Modified to reuse V3 spec-ctrl patches from Tim]
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> Signed-off-by: Ashok Raj <[email protected]>
> ---
> arch/x86/kvm/svm.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
If you want to do this, please include the vmx.c part as well...
Paolo
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0e68f0b..7c14471a 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -183,6 +183,8 @@ struct vcpu_svm {
> u64 gs_base;
> } host;
>
> + u64 spec_ctrl;
> +
> u32 *msrpm;
>
> ulong nmi_iret_rip;
> @@ -248,6 +250,7 @@ static const struct svm_direct_access_msrs {
> { .index = MSR_CSTAR, .always = true },
> { .index = MSR_SYSCALL_MASK, .always = true },
> #endif
> + { .index = MSR_IA32_SPEC_CTRL, .always = true },
> { .index = MSR_IA32_LASTBRANCHFROMIP, .always = false },
> { .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
> { .index = MSR_IA32_LASTINTFROMIP, .always = false },
> @@ -917,6 +920,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
>
> set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
> }
> +
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> + set_msr_interception(msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
> }
>
> static void add_msr_offset(u32 offset)
> @@ -3576,6 +3582,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_VM_CR:
> msr_info->data = svm->nested.vm_cr_msr;
> break;
> + case MSR_IA32_SPEC_CTRL:
> + msr_info->data = svm->spec_ctrl;
> + break;
> case MSR_IA32_UCODE_REV:
> msr_info->data = 0x01000065;
> break;
> @@ -3724,6 +3733,9 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> case MSR_VM_IGNNE:
> vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
> break;
> + case MSR_IA32_SPEC_CTRL:
> + svm->spec_ctrl = data;
> + break;
> case MSR_IA32_APICBASE:
> if (kvm_vcpu_apicv_active(vcpu))
> avic_update_vapic_bar(to_svm(vcpu), data);
> @@ -4871,6 +4883,19 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
> svm_complete_interrupts(svm);
> }
>
> +
> +/*
> + * Save guest value of spec_ctrl and also restore host value
> + */
> +static void save_guest_spec_ctrl(struct vcpu_svm *svm)
> +{
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> + svm->spec_ctrl = spec_ctrl_get();
> + spec_ctrl_restriction_on();
> + } else
> + rmb();
> +}
> +
> static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4910,6 +4935,14 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>
> clgi();
>
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> + /*
> + * FIXME: lockdep_assert_irqs_disabled();
> + */
> + WARN_ON_ONCE(!irqs_disabled());
> + spec_ctrl_set(svm->spec_ctrl);
> + }
> +
> local_irq_enable();
>
> asm volatile (
> @@ -4985,6 +5018,8 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> #endif
> );
>
> + save_guest_spec_ctrl(svm);
> +
> #ifdef CONFIG_X86_64
> wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> #else
>
On Fri, 2018-01-12 at 13:32 +0100, Borislav Petkov wrote:
> On Thu, Jan 11, 2018 at 05:32:19PM -0800, Ashok Raj wrote:
> > cpuid ax=0x7, return rdx bit 26 to indicate presence of both
> > IA32_SPEC_CTRL(MSR 0x48) and IA32_PRED_CMD(MSR 0x49)
>
> So why do we need two X86_FEATURE flags then?
AMD has only the latter and enumerates them differently.
On 1/11/2018 7:32 PM, Ashok Raj wrote:
> From: Paolo Bonzini <[email protected]>
>
> Direct access to MSR_IA32_SPEC_CTRL is important
> for performance. Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set restore host values on VM exit.
> it yet).
>
> TBD: need to check msr's can be passed through even if feature is not
> emuerated by the CPU.
>
> [Ashok: Modified to reuse V3 spec-ctrl patches from Tim]
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> Signed-off-by: Ashok Raj <[email protected]>
> ---
> arch/x86/kvm/svm.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0e68f0b..7c14471a 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -183,6 +183,8 @@ struct vcpu_svm {
> u64 gs_base;
> } host;
>
> + u64 spec_ctrl;
> +
> u32 *msrpm;
>
> ulong nmi_iret_rip;
> @@ -248,6 +250,7 @@ static const struct svm_direct_access_msrs {
> { .index = MSR_CSTAR, .always = true },
> { .index = MSR_SYSCALL_MASK, .always = true },
> #endif
> + { .index = MSR_IA32_SPEC_CTRL, .always = true },
> { .index = MSR_IA32_LASTBRANCHFROMIP, .always = false },
> { .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
> { .index = MSR_IA32_LASTINTFROMIP, .always = false },
> @@ -917,6 +920,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
>
> set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
> }
> +
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> + set_msr_interception(msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
This isn't needed. The entry in the direct_access_msrs will do this in
the loop above.
Thanks,
Tom
> }
>
> static void add_msr_offset(u32 offset)
> @@ -3576,6 +3582,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_VM_CR:
> msr_info->data = svm->nested.vm_cr_msr;
> break;
> + case MSR_IA32_SPEC_CTRL:
> + msr_info->data = svm->spec_ctrl;
> + break;
> case MSR_IA32_UCODE_REV:
> msr_info->data = 0x01000065;
> break;
> @@ -3724,6 +3733,9 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> case MSR_VM_IGNNE:
> vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
> break;
> + case MSR_IA32_SPEC_CTRL:
> + svm->spec_ctrl = data;
> + break;
> case MSR_IA32_APICBASE:
> if (kvm_vcpu_apicv_active(vcpu))
> avic_update_vapic_bar(to_svm(vcpu), data);
> @@ -4871,6 +4883,19 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
> svm_complete_interrupts(svm);
> }
>
> +
> +/*
> + * Save guest value of spec_ctrl and also restore host value
> + */
> +static void save_guest_spec_ctrl(struct vcpu_svm *svm)
> +{
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> + svm->spec_ctrl = spec_ctrl_get();
> + spec_ctrl_restriction_on();
> + } else
> + rmb();
> +}
> +
> static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4910,6 +4935,14 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>
> clgi();
>
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> + /*
> + * FIXME: lockdep_assert_irqs_disabled();
> + */
> + WARN_ON_ONCE(!irqs_disabled());
> + spec_ctrl_set(svm->spec_ctrl);
> + }
> +
> local_irq_enable();
>
> asm volatile (
> @@ -4985,6 +5018,8 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> #endif
> );
>
> + save_guest_spec_ctrl(svm);
> +
> #ifdef CONFIG_X86_64
> wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> #else
>
On 1/12/2018 6:39 AM, Woodhouse, David wrote:
> On Fri, 2018-01-12 at 13:32 +0100, Borislav Petkov wrote:
>> On Thu, Jan 11, 2018 at 05:32:19PM -0800, Ashok Raj wrote:
>>> cpuid ax=0x7, return rdx bit 26 to indicate presence of both
>>> IA32_SPEC_CTRL(MSR 0x48) and IA32_PRED_CMD(MSR 0x49)
>>
>> So why do we need two X86_FEATURE flags then?
>
> AMD has only the latter and enumerates them differently.
Correct. Both 0x48 and 0x49 are tied to the same cpuid bit. AMD has
a separate cpuid bit for 0x49 (IBPB) alone.
Thanks,
Tom
>
On 1/11/2018 7:32 PM, Ashok Raj wrote:
> cpuid ax=0x7, return rdx bit 26 to indicate presence of both
> IA32_SPEC_CTRL(MSR 0x48) and IA32_PRED_CMD(MSR 0x49)
>
> BIT0: Indirect Branch Prediction Barrier
>
> When this MSR is written with IBPB=1 it ensures that earlier code's behavior
> doesn't control later indirect branch predictions.
>
> Note this MSR is only writable and does not carry any state. Its a barrier
> so the code should perform a wrmsr when the barrier is needed.
>
> Signed-off-by: Ashok Raj <[email protected]>
> ---
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/msr-index.h | 3 +++
> arch/x86/kernel/cpu/spec_ctrl.c | 7 +++++++
> arch/x86/kvm/svm.c | 16 ++++++++++++++++
> arch/x86/kvm/vmx.c | 10 ++++++++++
> 5 files changed, 37 insertions(+)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 624b58e..52f37fc 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -213,6 +213,7 @@
> #define X86_FEATURE_MBA ( 7*32+18) /* Memory Bandwidth Allocation */
> #define X86_FEATURE_SPEC_CTRL ( 7*32+19) /* Speculation Control */
> #define X86_FEATURE_SPEC_CTRL_IBRS ( 7*32+20) /* Speculation Control, use IBRS */
> +#define X86_FEATURE_PRED_CMD ( 7*32+21) /* Indirect Branch Prediction Barrier */
>
> /* Virtualization flags: Linux defined, word 8 */
> #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0) /* Intel TPR Shadow */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 3e1cb18..1888e19 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -46,6 +46,9 @@
> #define SPEC_CTRL_DISABLE_IBRS (0 << 0)
> #define SPEC_CTRL_ENABLE_IBRS (1 << 0)
>
> +#define MSR_IA32_PRED_CMD 0x00000049
> +#define FEATURE_SET_IBPB (1<<0)
> +
> #define MSR_IA32_PERFCTR0 0x000000c1
> #define MSR_IA32_PERFCTR1 0x000000c2
> #define MSR_FSB_FREQ 0x000000cd
> diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
> index 02fc630..6cfec19 100644
> --- a/arch/x86/kernel/cpu/spec_ctrl.c
> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
> @@ -15,6 +15,13 @@ void spec_ctrl_scan_feature(struct cpuinfo_x86 *c)
> if (!c->cpu_index)
> static_branch_enable(&spec_ctrl_dynamic_ibrs);
> }
> + /*
> + * For Intel CPU's this MSR is shared the same cpuid
> + * enumeration. When MSR_IA32_SPEC_CTRL is present
> + * MSR_IA32_SPEC_CTRL is also available
> + * TBD: AMD might have a separate enumeration for each.
AMD will follow the specification that if cpuid ax=0x7, return rdx[26]
is set, it will indicate both MSR registers and features are supported.
But AMD also has a separate bit for IBPB (X86_FEATURE_PRED_CMD) alone.
As all of the IBRS/IBPB stuff happens, that patch will follow.
> + */
> + set_cpu_cap(c, X86_FEATURE_PRED_CMD);> }
> }
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 7c14471a..36924c9 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -251,6 +251,7 @@ static const struct svm_direct_access_msrs {
> { .index = MSR_SYSCALL_MASK, .always = true },
> #endif
> { .index = MSR_IA32_SPEC_CTRL, .always = true },
> + { .index = MSR_IA32_PRED_CMD, .always = false },
This should be .always = true
> { .index = MSR_IA32_LASTBRANCHFROMIP, .always = false },
> { .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
> { .index = MSR_IA32_LASTINTFROMIP, .always = false },
> @@ -531,6 +532,7 @@ struct svm_cpu_data {
> struct kvm_ldttss_desc *tss_desc;
>
> struct page *save_area;
> + struct vmcb *current_vmcb;
> };
>
> static DEFINE_PER_CPU(struct svm_cpu_data *, svm_data);
> @@ -923,6 +925,8 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
>
> if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> set_msr_interception(msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
> + if (boot_cpu_has(X86_FEATURE_PRED_CMD))
> + set_msr_interception(msrpm, MSR_IA32_PRED_CMD, 1, 1);
Similar to the comment about SPEC_CTRL, this should be removed as it will
be covered by the loop.
> }
>
> static void add_msr_offset(u32 offset)
> @@ -1711,11 +1715,18 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
> __free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
> kvm_vcpu_uninit(vcpu);
> kmem_cache_free(kvm_vcpu_cache, svm);
> + /*
> + * The VMCB could be recycled, causing a false negative in svm_vcpu_load;
> + * block speculative execution.
> + */
> + if (boot_cpu_has(X86_FEATURE_PRED_CMD))
> + native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> }
>
> static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> + struct svm_cpu_data *sd = per_cpu(svm_data, cpu);
> int i;
>
> if (unlikely(cpu != vcpu->cpu)) {
> @@ -1744,6 +1755,11 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> if (static_cpu_has(X86_FEATURE_RDTSCP))
> wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
>
> + if (sd->current_vmcb != svm->vmcb) {
> + sd->current_vmcb = svm->vmcb;
> + if (boot_cpu_has(X86_FEATURE_PRED_CMD))
> + native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> + }
> avic_vcpu_load(vcpu, cpu);
> }
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1913896..caeb9ff 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2280,6 +2280,8 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
> per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
> vmcs_load(vmx->loaded_vmcs->vmcs);
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
This should probably use X86_FEATURE_PRED_CMD.
> + native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);> }
>
> if (!already_loaded) {
> @@ -3837,6 +3839,12 @@ static void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
> free_vmcs(loaded_vmcs->vmcs);
> loaded_vmcs->vmcs = NULL;
> WARN_ON(loaded_vmcs->shadow_vmcs != NULL);
> + /*
> + * The VMCS could be recycled, causing a false negative in vmx_vcpu_load
> + * block speculative execution.
> + */
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
Again, X86_FEATURE_PRED_CMD.
> + native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> }
>
> static void free_kvm_area(void)
> @@ -6804,6 +6812,8 @@ static __init int hardware_setup(void)
> */
> if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> + if (boot_cpu_has(X86_FEATURE_PRED_CMD))
> + vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>
> vmx_disable_intercept_for_msr(MSR_FS_BASE, false);
> vmx_disable_intercept_for_msr(MSR_GS_BASE, false);
>
On Fri, 2018-01-12 at 09:31 -0600, Tom Lendacky wrote:
>
> AMD will follow the specification that if cpuid ax=0x7, return rdx[26]
> is set, it will indicate both MSR registers and features are supported.
>
> But AMD also has a separate bit for IBPB (X86_FEATURE_PRED_CMD) alone.
> As all of the IBRS/IBPB stuff happens, that patch will follow.
Please let's roll it into the patch set. I don't want Intel posting
deliberately AMD-ignoring patches. Sort it out, guys.
On Thu, Jan 11, 2018 at 09:03:56PM -0800, Dave Hansen wrote:
> On 01/11/2018 07:01 PM, Raj, Ashok wrote:
> > On Thu, Jan 11, 2018 at 06:20:13PM -0800, Andy Lutomirski wrote:
> >> On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
> >>>>
> >>>> What's wrong with native_read_msr()?
> >>>
> >>> Yes, i think i should have added to msr.h. The names didn't read as a
> >>> pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
> >>> defined?
> >>
> >> Why do you need to override paravirt?
> >
> > The idea was since these MSR's are passed through we shouldn't need to
> > handle them any differently. Also its best to do this as soon as possible
> > and avoid longer paths to get this barrier to hardware.
>
> We were also worried about the indirect calls that are part of the
> paravirt interfaces when retpolines are not in place.
But aren't those patched with direct calls during boot?
--
Josh
On Thu, 2018-01-11 at 21:03 -0800, Dave Hansen wrote:
> On 01/11/2018 07:01 PM, Raj, Ashok wrote:
> > On Thu, Jan 11, 2018 at 06:20:13PM -0800, Andy Lutomirski wrote:
> >> On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
> >>>>
> >>>> What's wrong with native_read_msr()?
> >>>
> >>> Yes, i think i should have added to msr.h. The names didn't read as a
> >>> pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
> >>> defined?
> >>
> >> Why do you need to override paravirt?
> >
> > The idea was since these MSR's are passed through we shouldn't need to
> > handle them any differently. Also its best to do this as soon as possible
> > and avoid longer paths to get this barrier to hardware.
>
> We were also worried about the indirect calls that are part of the
> paravirt interfaces when retpolines are not in place.
I have repeatedly been told that these go away, and are converted to
direct branches by the time the kernel is running userspace.
I suppose I really ought to validate that with qemu -d in_asm or
something. I couldn't see it in the code... but I haven't looked that
hard yet.
On 1/12/2018 9:36 AM, Woodhouse, David wrote:
> On Fri, 2018-01-12 at 09:31 -0600, Tom Lendacky wrote:
>>
>> AMD will follow the specification that if cpuid ax=0x7, return rdx[26]
>> is set, it will indicate both MSR registers and features are supported.
>>
>> But AMD also has a separate bit for IBPB (X86_FEATURE_PRED_CMD) alone.
>> As all of the IBRS/IBPB stuff happens, that patch will follow.
>
> Please let's roll it into the patch set. I don't want Intel posting
> deliberately AMD-ignoring patches. Sort it out, guys.
>
Based on the current patches, here is what it should be for the
standalone IBPB support:
x86/cpu: Detect standalone IBPB support
From: Tom Lendacky <[email protected]>
Add support to detect standalone IBPB feature support. This feature is
indicated as follows:
CPUID EAX=0x80000008, ECX=0x00 return EBX[12] indicates support for
IBPB
Signed-off-by: Tom Lendacky <[email protected]>
---
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/kernel/cpu/spec_ctrl.c | 9 +++++----
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 52f37fc..33f0215 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -273,6 +273,7 @@
#define X86_FEATURE_CLZERO (13*32+ 0) /* CLZERO instruction */
#define X86_FEATURE_IRPERF (13*32+ 1) /* Instructions Retired Count */
#define X86_FEATURE_XSAVEERPTR (13*32+ 2) /* Always save/restore FP error pointers */
+#define X86_FEATURE_IBPB (13*32+12) /* Indirect Branch Prediction Barrier */
/* Thermal and Power Management Leaf, CPUID level 0x00000006 (EAX), word 14 */
#define X86_FEATURE_DTHERM (14*32+ 0) /* Digital Thermal Sensor */
diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
index 6cfec19..1aadd73 100644
--- a/arch/x86/kernel/cpu/spec_ctrl.c
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -16,12 +16,13 @@ void spec_ctrl_scan_feature(struct cpuinfo_x86 *c)
static_branch_enable(&spec_ctrl_dynamic_ibrs);
}
/*
- * For Intel CPU's this MSR is shared the same cpuid
- * enumeration. When MSR_IA32_SPEC_CTRL is present
- * MSR_IA32_SPEC_CTRL is also available
- * TBD: AMD might have a separate enumeration for each.
+ * The PRED_CMD MSR is shared with the cpuid enumeration
+ * for SPEC_CTRL. When MSR_IA32_SPEC_CTRL is present,
+ * then MSR_IA32_PRED_CMD is, too.
*/
set_cpu_cap(c, X86_FEATURE_PRED_CMD);
+ } else if (boot_cpu_has(X86_FEATURE_IBPB)) {
+ set_cpu_cap(c, X86_FEATURE_PRED_CMD);
}
}
>
>
> Amazon Web Services UK Limited. Registered in England and Wales with
> registration number 08650665 and which has its registered office at 60
> Holborn Viaduct, London EC1A 2FD, United Kingdom.
On Thu, Jan 11, 2018 at 7:01 PM, Raj, Ashok <[email protected]> wrote:
> On Thu, Jan 11, 2018 at 06:20:13PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
>> >>
>> >> What's wrong with native_read_msr()?
>> >
>> > Yes, i think i should have added to msr.h. The names didn't read as a
>> > pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
>> > defined?
>>
>> Why do you need to override paravirt?
>
> The idea was since these MSR's are passed through we shouldn't need to
> handle them any differently. Also its best to do this as soon as possible
> and avoid longer paths to get this barrier to hardware.
>
It should end up with essentially the same instructions at runtime.
I'm generally in favor of using the less weird code when it will work.
On Thu, Jan 11, 2018 at 9:03 PM, Dave Hansen <[email protected]> wrote:
> On 01/11/2018 07:01 PM, Raj, Ashok wrote:
>> On Thu, Jan 11, 2018 at 06:20:13PM -0800, Andy Lutomirski wrote:
>>> On Thu, Jan 11, 2018 at 5:52 PM, Raj, Ashok <[email protected]> wrote:
>>>>>
>>>>> What's wrong with native_read_msr()?
>>>>
>>>> Yes, i think i should have added to msr.h. The names didn't read as a
>>>> pair, one was native_read_msr, wrmsrl could be taken over when paravirt is
>>>> defined?
>>>
>>> Why do you need to override paravirt?
>>
>> The idea was since these MSR's are passed through we shouldn't need to
>> handle them any differently. Also its best to do this as soon as possible
>> and avoid longer paths to get this barrier to hardware.
>
> We were also worried about the indirect calls that are part of the
> paravirt interfaces when retpolines are not in place.
>
How could those possibly be any worse than any other indirect call in
the kernel?
> On Jan 13, 2018, at 5:52 AM, Van De Ven, Arjan <[email protected]> wrote:
>
>
>>> We were also worried about the indirect calls that are part of the
>>> paravirt interfaces when retpolines are not in place.
>>>
>>
>> How could those possibly be any worse than any other indirect call in
>> the kernel?
>
> they're worse if they happen before you write the MSR that then protects them?
I haven't looked at the latest IBRS code, but I can see only two ways to do it:
1. Write the MSRs from asm. Get exactly what you expect.
2. Write them from C. Trust the compiler to be sane. Failure to optimize asm goto correctly or failure of the paravirt code to patch correctly is very low on the list of things to worry about.
> > We were also worried about the indirect calls that are part of the
> > paravirt interfaces when retpolines are not in place.
> >
>
> How could those possibly be any worse than any other indirect call in
> the kernel?
they're worse if they happen before you write the MSR that then protects them?
On Fri, Jan 12, 2018 at 10:09:08AM +0000, David Woodhouse wrote:
> static_cpu_has() + asm-goto is NOT SUFFICIENT.
>
> It's still *possible* for a missed optimisation in GCC to still leave
> us with a conditional branch around the wrmsr, letting the CPU
> speculate around it too.
OK, so GCC would have to be bloody retarded to mess this up; but would
something like the below work for you?
The usage is like:
if (static_branch_unlikely(key)) {
arch_static_assert();
stuff();
}
And then objtool will fail things if the first instruction into that
branch is not immediately after a NOP/JMP patch site (on either the NOP
or the JMP+disp side of things).
---
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index 8c0de4282659..6a1a893145ca 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -62,6 +62,15 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
return true;
}
+static __always_inline void arch_static_assert(void)
+{
+ asm volatile ("1:\n\t"
+ ".pushsection .discard.jump_assert, \"aw\" \n\t"
+ _ASM_ALIGN "\n\t"
+ _ASM_PTR "1b \n\t"
+ ".popsection \n\t");
+}
+
#ifdef CONFIG_X86_64
typedef u64 jump_label_t;
#else
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f40d46e24bcc..657bfc706bb6 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -687,8 +687,17 @@ static int handle_jump_alt(struct objtool_file *file,
struct instruction *orig_insn,
struct instruction **new_insn)
{
- if (orig_insn->type == INSN_NOP)
+ struct instruction *next_insn = list_next_entry(orig_insn, list);
+
+ if (orig_insn->type == INSN_NOP) {
+ /*
+ * If orig_insn is a NOP, then new_insn is the branch target
+ * for when it would've been a JMP.
+ */
+ next_insn->br_static = true;
+ (*new_insn)->br_static = true;
return 0;
+ }
if (orig_insn->type != INSN_JUMP_UNCONDITIONAL) {
WARN_FUNC("unsupported instruction at jump label",
@@ -696,7 +705,16 @@ static int handle_jump_alt(struct objtool_file *file,
return -1;
}
- *new_insn = list_next_entry(orig_insn, list);
+ /*
+ * Otherwise, orig_insn is a JMP and it will have orig_insn->jump_dest.
+ * In this case we'll effectively NOP the alt by pointing new_insn at
+ * next_insn.
+ */
+ orig_insn->jump_dest->br_static = true;
+ next_insn->br_static = true;
+
+ *new_insn = next_insn;
+
return 0;
}
@@ -1067,6 +1085,50 @@ static int read_unwind_hints(struct objtool_file *file)
return 0;
}
+static int read_jump_assertions(struct objtool_file *file)
+{
+ struct section *sec, *relasec;
+ struct instruction *insn;
+ struct rela *rela;
+ int i;
+
+ sec = find_section_by_name(file->elf, ".discard.jump_assert");
+ if (!sec)
+ return 0;
+
+ relasec = sec->rela;
+ if (!relasec) {
+ WARN("missing .rela.discard.jump_assert section");
+ return -1;
+ }
+
+ if (sec->len % sizeof(unsigned long)) {
+ WARN("jump_assert size mismatch: %d %ld", sec->len, sizeof(unsigned long));
+ return -1;
+ }
+
+ for (i = 0; i < sec->len / sizeof(unsigned long); i++) {
+ rela = find_rela_by_dest(sec, i * sizeof(unsigned long));
+ if (!rela) {
+ WARN("can't find rela for jump_assert[%d]", i);
+ return -1;
+ }
+
+ insn = find_insn(file, rela->sym->sec, rela->addend);
+ if (!insn) {
+ WARN("can't find insn for jump_assert[%d]", i);
+ return -1;
+ }
+
+ if (!insn->br_static) {
+ WARN_FUNC("static assert FAIL", insn->sec, insn->offset);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
static int decode_sections(struct objtool_file *file)
{
int ret;
@@ -1105,6 +1167,10 @@ static int decode_sections(struct objtool_file *file)
if (ret)
return ret;
+ ret = read_jump_assertions(file);
+ if (ret)
+ return ret;
+
return 0;
}
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index dbadb304a410..12e0a3cf0350 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -45,6 +46,7 @@ struct instruction {
unsigned char type;
unsigned long immediate;
bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
+ bool br_static;
struct symbol *call_dest;
struct instruction *jump_dest;
struct list_head alts;
On Mon, 2018-01-15 at 14:45 +0100, Peter Zijlstra wrote:
> On Fri, Jan 12, 2018 at 10:09:08AM +0000, David Woodhouse wrote:
> > static_cpu_has() + asm-goto is NOT SUFFICIENT.
> >
> > It's still *possible* for a missed optimisation in GCC to still leave
> > us with a conditional branch around the wrmsr, letting the CPU
> > speculate around it too.
>
> OK, so GCC would have to be bloody retarded to mess this up;
Like *that's* never happened before? In corner cases where it just gets
confused and certain optimisations go out the window?
> but would something like the below work for you?
>
> The usage is like:
>
> if (static_branch_unlikely(key)) {
> arch_static_assert();
> stuff();
> }
>
> And then objtool will fail things if the first instruction into that
> branch is not immediately after a NOP/JMP patch site (on either the NOP
> or the JMP+disp side of things).
That seems reasonable; thanks. Bonus points if you can make the
arch_static_assert happen() automatically with vile tricks like
#define IF_FEATURE(ftr) if (static_cpu_has(ftr)) arch_static_assert,
So then it just becomes
IF_FEATURE(key) {
stuff();
}
There might not be a sane way to do that though. And it's OK to have to
manually annotate the call sites where this is for correctness and not
purely optimisation.
On Mon, Jan 15, 2018 at 02:59:22PM +0100, David Woodhouse wrote:
> #define IF_FEATURE(ftr) if (static_cpu_has(ftr)) arch_static_assert,?
>
> ? ?IF_FEATURE(key) {
> ? ? ? ?stuff();
> ? ?}
>
> There might not be a sane way to do that though. And it's OK to have to
> manually annotate the call sites where this is for correctness and not
> purely optimisation.
Something like:
#define if_static_likely(_key) \
if (static_branch_likely(_key) && (arch_static_assert(), true))
should work I think. The thing about static_cpu_has() is that it doesn't
use jump_labels but alternatives. I could of course also construct an
assert for that, but it needs different things.