On Thu, Feb 05, 2026 at 05:25:15PM -0800, Sean Christopherson wrote:
> On Fri, Feb 06, 2026, Yosry Ahmed wrote:
> > February 5, 2026 at 4:59 PM, "Sean Christopherson" <seanjc@google.com> wrote:
> > > On Thu, Jan 15, 2026, Yosry Ahmed wrote:
> > > > In preparation for using svm_copy_lbrs() with 'struct vmcb_save_area'
> > > > without a containing 'struct vmcb', and later even 'struct
> > > > vmcb_save_area_cached', make it a macro. Pull the call to
> > > > vmcb_mark_dirty() out to the callers.
> > > >
> > > > Macros are generally not preferred compared to functions, mainly due to
> > > > type-safety. However, in this case it seems like having a simple macro
> > > > copying a few fields is better than copy-pasting the same 5 lines of
> > > > code in different places.
> > > >
> > > > On the bright side, pulling vmcb_mark_dirty() calls to the callers makes
> > > > it clear that in one case, vmcb_mark_dirty() was being called on VMCB12.
> > > > It is not architecturally defined for the CPU to clear arbitrary clean
> > > > bits, and it is not needed, so drop that one call.
> > > >
> > > > Technically fixes the non-architectural behavior of setting the dirty
> > > > bit on VMCB12.
> > > >
> > > Stop. Bundling. Things. Together.
> > >
> > > /shakes fist angrily
> > >
> > > I was absolutely not expecting a patch titled "KVM: SVM: Switch svm_copy_lbrs()
> > > to a macro" to end with a Fixes tag, and I was *really* not expecting it to also
> > > be Cc'd for stable.
> > >
> > > At a glance, I genuinely can't tell if you added a Fixes to scope the backport,
> > > or because of the dirty vmcb12 bits thing.
> > >
> > > First fix the dirty behavior (and probably tag it for stable to avoid creating
> > > an unnecessary backport conflict), then in a separate patch macrofy the helper.
> > > Yeah, checkpatch will "suggest" that the stable@ patch should have Fixes, but
> > > for us humans, that's _useful_ information, because it says "hey you, this is a
> > > dependency for an upcoming fix!". As written, I look at this patch and go "huh?".
> > > (and then I look at the next patch and it all makes sense).
> >
> > I agree, but fixing the dirty behavior on its own requires open-coding the
> > function, then the following patch would change it to a macro and use it
> > again. I was trying to minimize the noise of moving code back and forth..
>
> I don't follow. Isn't it just this?
Yeah ignore the previous comment, I was thinking about something else.
>
> @@ -848,8 +859,6 @@ void svm_copy_lbrs(struct vmcb *to_vmcb, struct vmcb *from_vmcb)
> to_vmcb->save.br_to = from_vmcb->save.br_to;
> to_vmcb->save.last_excp_from = from_vmcb->save.last_excp_from;
> to_vmcb->save.last_excp_to = from_vmcb->save.last_excp_to;
> -
> - vmcb_mark_dirty(to_vmcb, VMCB_LBR);
> }
>
> static void __svm_enable_lbrv(struct kvm_vcpu *vcpu)
> @@ -877,6 +886,8 @@ void svm_update_lbrv(struct kvm_vcpu *vcpu)
> (is_guest_mode(vcpu) && guest_cpu_cap_has(vcpu, X86_FEATURE_LBRV) &&
> (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
>
> + vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> +
Although I would rather keep this in callers of svm_copy_lbrs(), instead
of hiding it here. For example, in nested_svm_vmexit(), this would be
wrong if the call to svm_switch_vmcb() was moved a bit later (which
would be wrong for other reasons, but the clean bit wouldn't be
obvious).
It also makes it obvious that we are specifically not dirtying vmcb12.
> if (enable_lbrv && !current_enable_lbrv)
> __svm_enable_lbrv(vcpu);
> else if (!enable_lbrv && current_enable_lbrv)
> @@ -3079,7 +3090,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> break;
>
> svm->vmcb->save.dbgctl = data;
> - vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> svm_update_lbrv(vcpu);
> break;
> case MSR_VM_HSAVE_PA: