Re: [PATCH] KVM: SVM: Fix redundant updates of LBR MSR intercepts

Yosry Ahmed
2025-11-14T16:52:44Z

On Fri, Nov 14, 2025 at 08:34:54AM -0800, Sean Christopherson wrote:
> On Wed, Nov 12, 2025, Yosry Ahmed wrote:
> > svm_update_lbrv() always updates LBR MSRs intercepts, even when they are
> > already set correctly. This results in force_msr_bitmap_recalc always
> > being set to true on every nested transition,
>
> Nit, it's only on VMRUN, not on every transition (i.e. not on nested #VMEXIT).

How so? svm_update_lbrv() will also be called in nested_svm_vmexit(),
and it will eventually lead to force_msr_bitmap_recalc being set to
true.

I guess what you meant is the "undoing the Hyper-V optimization" part.
That is indeed only affected by the svm_update_lbrv() call in the nested
VMRUN path.

So we do set force_msr_bitmap_recalc on nested #VMEXIT, but it doesn't
really matter because we set it again on nested VMRUN before
nested_svm_merge_msrpm() is called.

>
> > essentially undoing the hyperv optimization in nested_svm_merge_msrpm().
>
> When something fixes a KVM test failures (selftests or KUT), please call that
> out in the changelog. That way when other people encounter the failure, they'll
> get search hits and won't have to waste their time bisecting and/or debugging.
>
> If you hadn't mentioned off-list that this was detected by hyperv_svm_test, I
> wouldn't have had the first clue as to why that test started failing. Even with
> the hint, it still took me a few minutes to connect the dots.

Noted, makes sense. I thought the fix and the original patch are in such
quick succession that hopefully no one will run into it.

>
> In general, be more explicit/detailed, e.g. "undoing the hyperv optimization" is
> unnecessarily vague, as the reader has to go look at the code to understand what
> you're talking about. My philosophy with changelogs is that they are write-once,
> read-many, and so if you can save any time/effort for readers, it's almost always
> worth the extra time/effort on the "write" side.
>
> And a nit: my strong preference is to lead with what is being changed, and then
> dive into the details of why, what's breaking, etc. This is one of the few
> divergences from the tip-tree preferences. From Documentation/process/maintainer-kvm-x86.rst:
>
> Stating what a patch does before diving into details is preferred by KVM x86
> for several reasons. First and foremost, what code is actually being changed
> is arguably the most important information, and so that info should be easy to
> find. Changelogs that bury the "what's actually changing" in a one-liner after
> 3+ paragraphs of background make it very hard to find that information.

Noted.

>
> E.g.
>
> --
> Don't update the LBR MSR intercept bitmaps if they're already up-to-date,
> as unconditionally updating the intercepts forces KVM to recalculate the
> MSR bitmaps for vmcb02 on every nested VMRUN. Functionally, the redundant
> updates are benign, but forcing an update neuters the Hyper-V optimization
> that allows KVM to skip refreshing the vmcb12 MSR bitmap if L1 marked the
> "nested enlightenments" as being clean, i.e. if L1 told KVM that no
> changes were made to the MSR bitmap since the last VMRUN.
>
> Clobbering the Hyper-V optimization manifests as a failure in the
> hyperv_svm_test KVM selftest, which intentionally changes the MSR bitmap
> "without telling KVM about it" to verify that KVM honors the clean hint.
>
> ==== Test Assertion Failure ====
> x86/hyperv_svm_test.c:120: vmcb->control.exit_code == 0x081
> pid=193558 tid=193558 errno=4 - Interrupted system call
> 1 0x0000000000411361: assert_on_unhandled_exception at processor.c:659
> 2 0x0000000000406186: _vcpu_run at kvm_util.c:1699
> 3 (inlined by) vcpu_run at kvm_util.c:1710
> 4 0x0000000000401f2a: main at hyperv_svm_test.c:175
> 5 0x000000000041d0d3: __libc_start_call_main at libc-start.o:?
> 6 0x000000000041f27c: __libc_start_main_impl at ??:?
> 7 0x00000000004021a0: _start at ??:?
> vmcb->control.exit_code == SVM_EXIT_VMMCALL
>
> Avoid using ....
> --

Thanks!

>
> > Fix it by keeping track of whether LBR MSRs are intercepted or not and
> > only doing the update if needed, similar to x2avic_msrs_intercepted.
> >
> > Avoid using svm_test_msr_bitmap_*() to check the status of the
> > intercepts, as an arbitrary MSR will need to be chosen as a
> > representative of all LBR MSRs, and this could theoretically break if
> > some of the MSRs intercepts are handled differently from the rest.
>
> For posterity, Yosry originally proposed (off-list) fixing this by having
> svm_set_intercept_for_msr() check for redundant updates, but I voted against
> that because updating MSR interception _should_ be rare (full CPUID updates and
> explicit MSR filter updates), and I don't want to risk hiding a bug/flaw elsewhere.
> I.e. if something is triggering frequent/unexpected MSR bitmap changes, I want
> that to be surfaced, not squashed/handled by the low level helpers.

Hmm, that was on-list though :P

https://lore.kernel.org/kvm/aRO5ItX_--ZDfnfM@google.com/

>
>
> > Also, using svm_test_msr_bitmap_*() makes backports difficult as it was
> > only recently introduced with no direct alternatives in older kernels.
> >
> > Fixes: fbe5e5f030c2 ("KVM: nSVM: Always recalculate LBR MSR intercepts in svm_update_lbrv()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@linux.dev>
>
> With an updated changelog,
>
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> Tested-by: Sean Christopherson <seanjc@google.com>

Thanks!

Paolo, do you prefer a updated patch with the updated changelog, or
fixing it up when you apply it?