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: <usc5ysverr7gtt4itnw2s22s5hpfbtgwttm74i25gxbqm3b6cb@x2i4nzlo2wbz>
Date: Fri, 6 Feb 2026 15:13:08 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
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 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@...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?

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: