Re: [9/8] KVM: x86: limit MSR_IA32_SPEC_CTRL access based on CPUID availability

[Mihai Carabas]

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: