It's really hard to tell which patches are being proposed for which repositories, but assuming that everything else is correct, I don't think your condition is adequate. What if the physical CPU and the virtual CPU both have CPUID.(EAX=7H,ECX=0):EDX[26], but only the physical CPU has CPUID.(EAX=7H,ECX=0):EDX[27]? If the guest has write access to MSR_IA32_SPEC_CTRL, it can set MSR_IA32_SPEC_CTRL[1] (STIBP), even though setting that bit in the guest should raise #GP. On Tue, Jan 30, 2018 at 8:43 AM, Mihai Carabas <mihai.carabas@xxxxxxxxxx> wrote: > On 30.01.2018 18:33, Jim Mattson wrote: >> >> All MSR intercepts are enabled by default, so I don't think this patch >> does anything at all, unless I'm missing some context. >> > > Currently on upstream some MSR are intercepted: > https://github.com/torvalds/linux/blob/master/arch/x86/kvm/vmx.c#L6838 > > In particular to this patch, the MSR_IA32_SPEC_CTRL intercept is disabled in > 3/8: https://patchwork.kernel.org/patch/10151889/ > > > >> On Tue, Jan 30, 2018 at 5:21 AM, Mihai Carabas <mihai.carabas@xxxxxxxxxx> >> wrote: >>> >>> Hello Paolo, >>> >>> I've back ported this patch on 4.1, after adding the per-vcpu MSR bitmap. >>> Also enabled the SPEC_CTRL_MSR intercept if qemu instructed so [1]. >>> >>> Reviewed-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx> >>> >>> [1] >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -8391,6 +8391,16 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm >>> *kvm, unsigned int id) >>> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PRED_CMD, >>> MSR_TYPE_R | MSR_TYPE_W); >>> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_BNDCFGS, >>> MSR_TYPE_R | MSR_TYPE_W); >>> >>> + /* >>> + * If the physical CPU or the vCPU of this VM doesn't >>> + * support SPEC_CTRL feature, catch each access to it. >>> + */ >>> + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || >>> + !guest_cpuid_has_spec_ctrl(&vmx->vcpu)) >>> + vmx_enable_intercept_for_msr( >>> + msr_bitmap, >>> + MSR_IA32_SPEC_CTRL, >>> + MSR_TYPE_R | MSR_TYPE_W); >>> >>> /* >>> * If PML is turned on, failure on enabling PML just results in >>> failure >>> >>> >>> >>> On 09.01.2018 14:03, Paolo Bonzini wrote: >>>> >>>> >>>> MSR_IA32_SPEC_CTRL is not available unless CPU[7,0].EDX[26] is 1. >>>> Check that against host CPUID or guest CPUID, respectively for >>>> host-initiated and guest-initiated accesses. >>>> >>>> Suggested-by: Jim Mattson <jmattson@xxxxxxxxxx> >>>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> >>>> --- >>>> This is for after X86_FEATURE_SPEC_CTRL is added to Linux, but >>>> I still wanted to ack Jim's improvement. >>>> >>>> arch/x86/kvm/svm.c | 8 ++++++++ >>>> arch/x86/kvm/vmx.c | 8 ++++++++ >>>> 2 files changed, 16 insertions(+) >>>> >>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >>>> index 97126c2bd663..3a646580d7c5 100644 >>>> --- a/arch/x86/kvm/svm.c >>>> +++ b/arch/x86/kvm/svm.c >>>> @@ -3648,6 +3648,10 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, >>>> struct msr_data *msr_info) >>>> msr_info->data = svm->nested.vm_cr_msr; >>>> break; >>>> case MSR_IA32_SPEC_CTRL: >>>> + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || >>>> + (!msr_info->host_initiated && >>>> + !guest_cpuid_has(vcpu, >>>> X86_FEATURE_SPEC_CTRL))) >>>> + return 1; >>>> msr_info->data = svm->spec_ctrl; >>>> break; >>>> case MSR_IA32_UCODE_REV: >>>> @@ -3806,6 +3810,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, >>>> struct msr_data *msr) >>>> vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data >>>> 0x%llx\n", ecx, data); >>>> break; >>>> case MSR_IA32_SPEC_CTRL: >>>> + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || >>>> + (!msr_info->host_initiated && >>>> + !guest_cpuid_has(vcpu, >>>> X86_FEATURE_SPEC_CTRL))) >>>> + return 1; >>>> svm->spec_ctrl = data; >>>> break; >>>> case MSR_IA32_APICBASE: >>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>>> index 49b4a2d61603..42bc7ee293e4 100644 >>>> --- a/arch/x86/kvm/vmx.c >>>> +++ b/arch/x86/kvm/vmx.c >>>> @@ -3368,6 +3368,10 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, >>>> struct msr_data *msr_info) >>>> msr_info->data = guest_read_tsc(vcpu); >>>> break; >>>> case MSR_IA32_SPEC_CTRL: >>>> + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || >>>> + (!msr_info->host_initiated && >>>> + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))) >>>> + return 1; >>>> msr_info->data = to_vmx(vcpu)->spec_ctrl; >>>> break; >>>> case MSR_IA32_SYSENTER_CS: >>>> @@ -3510,6 +3514,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, >>>> struct msr_data *msr_info) >>>> kvm_write_tsc(vcpu, msr_info); >>>> break; >>>> case MSR_IA32_SPEC_CTRL: >>>> + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || >>>> + (!msr_info->host_initiated && >>>> + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))) >>>> + return 1; >>>> to_vmx(vcpu)->spec_ctrl = data; >>>> break; >>>> case MSR_IA32_CR_PAT: >>>> >>> >
