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

[Sean Christopherson <seanjc@google.com>]

On Tue, Mar 03, 2026, Yosry Ahmed wrote:
> > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > 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. 

Ya :-(

> 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?


Where's your sense of adventure?

	case MSR_IA32_LASTBRANCHFROMIP:
	case MSR_IA32_LASTBRANCHTOIP:
	case MSR_IA32_LASTINTFROMIP:
	case MSR_IA32_LASTINTTOIP:
		if (!lbrv)
			return KVM_MSR_RET_UNSUPPORTED;
		if (!msr->host_initiated)
			return 1;
		*(&svm->vmcb->save.br_from + (ecx - MSR_IA32_LASTBRANCHFROMIP)) = data;
		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
		break;

Jokes aside, maybe this, to dedup get() at the same time?

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 68b747a94294..f1811105e89f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2720,6 +2720,23 @@ static int svm_get_feature_msr(u32 msr, u64 *data)
        return 0;
 }
 
+static __always_inline u64 *svm_vmcb_lbr(struct vcpu_svm *svm, u32 msr)
+{
+       switch (msr) {
+       case MSR_IA32_LASTBRANCHFROMIP:
+               return &svm->vmcb->save.br_from;
+       case MSR_IA32_LASTBRANCHTOIP:
+               return &svm->vmcb->save.br_to;
+       case MSR_IA32_LASTINTFROMIP:
+               return &svm->vmcb->save.last_excp_from;
+       case MSR_IA32_LASTINTTOIP:
+               return &svm->vmcb->save.last_excp_to;
+       default:
+               break;
+       }
+       BUILD_BUG();
+}
+
 static bool sev_es_prevent_msr_access(struct kvm_vcpu *vcpu,
                                      struct msr_data *msr_info)
 {
@@ -2838,16 +2855,10 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                msr_info->data = lbrv ? svm->vmcb->save.dbgctl : 0;
                break;
        case MSR_IA32_LASTBRANCHFROMIP:
-               msr_info->data = lbrv ? svm->vmcb->save.br_from : 0;
-               break;
        case MSR_IA32_LASTBRANCHTOIP:
-               msr_info->data = lbrv ? svm->vmcb->save.br_to : 0;
-               break;
        case MSR_IA32_LASTINTFROMIP:
-               msr_info->data = lbrv ? svm->vmcb->save.last_excp_from : 0;
-               break;
        case MSR_IA32_LASTINTTOIP:
-               msr_info->data = lbrv ? svm->vmcb->save.last_excp_to : 0;
+               msr_info->data = lbrv ? *svm_vmcb_lbr(svm, msr_info->index) : 0;
                break;
        case MSR_VM_HSAVE_PA:
                msr_info->data = svm->nested.hsave_msr;
@@ -3122,35 +3133,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
                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;
+               *svm_vmcb_lbr(svm, ecx) = data;
                vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
                break;
        case MSR_VM_HSAVE_PA: