Re: [PATCH v7 03/26] KVM: SVM: Add missing save/restore handling of LBR MSRs

[Yosry Ahmed]

> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index f7d5db0af69ac..3bf758c9cb85c 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1100,6 +1100,11 @@ void svm_copy_vmrun_state(struct vmcb_save_area *to_save,
> >               to_save->isst_addr = from_save->isst_addr;
> >               to_save->ssp = from_save->ssp;
> >       }
> > +
> > +     if (lbrv) {
>
> Tomato, tomato, but maybe make this
>
>         if (kvm_cpu_cap_has(X86_FEATURE_LBRV)) {
>
> to capture that this requires nested support.  I can't imagine we'll ever disable
> X86_FEATURE_LBRV when nested=1 && lbrv=1, but I don't see any harm in being
> paranoid in this case.

Sounds good.

>
> > +             svm_copy_lbrs(to_save, from_save);
> > +             to_save->dbgctl &= ~DEBUGCTL_RESERVED_BITS;
> > +     }
> >  }
> >
> >  void svm_copy_vmloadsave_state(struct vmcb *to_vmcb, struct vmcb *from_vmcb)
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index f52e588317fcf..cb53174583a26 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3071,6 +3071,30 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> >               vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> >               svm_update_lbrv(vcpu);
> >               break;
> > +     case MSR_IA32_LASTBRANCHFROMIP:
>
> Shouldn't these be gated on lbrv?  If LBRV is truly unsupported, KVM would be
> writing "undefined" fields and clearing "unknown" clean bits.
>
> Specifically, if we do:
>
>                 if (!lbrv)
>                         return KVM_MSR_RET_UNSUPPORTED;
>
> then kvm_do_msr_access() will allow writes of '0' from the host, via this code:
>
>         if (host_initiated && !*data && kvm_is_advertised_msr(msr))
>                 return 0;
>
> And then in the read side, do e.g.:
>
>         msr_info->data = lbrv ? svm->vmcb->save.dbgctl : 0;
>
> to ensure KVM won't feed userspace garbage (the VMCB fields should be '0', but
> there's no reason to risk that).

Good call.

>
> The changelog also needs to call out that kvm_set_msr_common() returns
> KVM_MSR_RET_UNSUPPORTED for unhandled MSRs (i.e. for VMX and TDX), and that
> kvm_get_msr_common() explicitly zeros the data for MSR_IA32_LASTxxx (because per
> b5e2fec0ebc3 ("KVM: Ignore DEBUGCTL MSRs with no effect"), old and crust kernels
> would read the MSRs on Intel...).

That was captured (somehow):

For VMX, this also adds save/restore handling of KVM_GET_MSR_INDEX_LIST.
For unspported MSR_IA32_LAST* MSRs, kvm_do_msr_access() should 0 these
MSRs on userspace reads, and ignore KVM_MSR_RET_UNSUPPORTED on userspace
writes.

>
> So all in all (not yet tested), this?  If this is the only issue in the series,
> or at least in the stable@ part of the series, no need for a v8 (I've obviously
> already done the fixup).

Looks good with a minor nit below (could be a followup).

> @@ -3075,6 +3075,38 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>                 vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
>                 svm_update_lbrv(vcpu);
>                 break;
> +       case MSR_IA32_LASTBRANCHFROMIP:
> +               if (!lbrv)
> +                       return KVM_MSR_RET_UNSUPPORTED;
> +               if (!msr->host_initiated)
> +                       return 1;
> +               svm->vmcb->save.br_from = data;
> +               vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> +               break;
> +       case MSR_IA32_LASTBRANCHTOIP:
> +               if (!lbrv)
> +                       return KVM_MSR_RET_UNSUPPORTED;
> +               if (!msr->host_initiated)
> +                       return 1;
> +               svm->vmcb->save.br_to = data;
> +               vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> +               break;
> +       case MSR_IA32_LASTINTFROMIP:
> +               if (!lbrv)
> +                       return KVM_MSR_RET_UNSUPPORTED;
> +               if (!msr->host_initiated)
> +                       return 1;
> +               svm->vmcb->save.last_excp_from = data;
> +               vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> +               break;
> +       case MSR_IA32_LASTINTTOIP:
> +               if (!lbrv)
> +                       return KVM_MSR_RET_UNSUPPORTED;
> +               if (!msr->host_initiated)
> +                       return 1;
> +               svm->vmcb->save.last_excp_to = data;
> +               vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> +               break;

There's so much repeated code here. We can use gotos to share code,
but I am not sure if that's a strict improvement. We can also use a
helper, perhaps?

static int svm_set_lbr_msr(struct vcpu_svm *svm, struct msr_data *msr,
u64 data, u64 *field)
{
       if (!lbrv)
               return KVM_MSR_RET_UNSUPPORTED;
       if (!msr->host_initiated)
               return 1;
       *field = data;
        vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
        return 0;
}

...

       case MSR_IA32_LASTBRANCHFROMIP:
             ret = svm_set_lbr_msr(svm, msr, data, &svm->vmcb->save.br_from);
             if (ret)
                        return ret;
              break;
...