linux-kernel - Re: [PATCH v4 01/26] KVM: SVM: Switch svm_copy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aYVC-1Pk01kQVJqD@google.com>
Date: Thu, 5 Feb 2026 17:25:15 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH v4 01/26] KVM: SVM: Switch svm_copy_lbrs() to a macro

On Fri, Feb 06, 2026, Yosry Ahmed wrote:
> February 5, 2026 at 4:59 PM, "Sean Christopherson" <seanjc@...gle.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?

@@ -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);
+
        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: