KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

[PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Jim Mattson 3 months, 1 week ago

With the VMCB's LBR_VIRTUALIZATION_ENABLE bit set, the CPU will load
the DebugCtl MSR from the VMCB's DBGCTL field at VMRUN. To ensure that
it does not load a stale cached value, clear the VMCB's LBR clean bit
when L1 is running and bit 0 (LBR) of the DBGCTL field is changed from
0 to 1. (Note that this is already handled correctly when L2 is
running.)

There is no need to clear the clean bit in the other direction,
because when the VMCB's DBGCTL.LBR is 0, the VMCB's
LBR_VIRTUALIZATION_ENABLE bit will be clear, and the CPU will not
consult the VMCB's DBGCTL field at VMRUN.

Fixes: 1d5a1b5860ed ("KVM: x86: nSVM: correctly virtualize LBR msrs when L2 is running")
Reported-by: Matteo Rizzo <matteorizzo@google.com>
Reported-by: evn@google.com
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/svm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 153c12dbf3eb..b4e5a0684f57 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -816,6 +816,8 @@ void svm_enable_lbrv(struct kvm_vcpu *vcpu)
 	/* Move the LBR msrs to the vmcb02 so that the guest can see them. */
 	if (is_guest_mode(vcpu))
 		svm_copy_lbrs(svm->vmcb, svm->vmcb01.ptr);
+	else
+		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
 }
 
 static void svm_disable_lbrv(struct kvm_vcpu *vcpu)
-- 
2.51.2.1006.ga50a493c49-goog

Re: [PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Shivansh Dhiman 3 months ago

On 01-11-2025 05:32, Jim Mattson wrote:
> With the VMCB's LBR_VIRTUALIZATION_ENABLE bit set, the CPU will load
> the DebugCtl MSR from the VMCB's DBGCTL field at VMRUN. To ensure that
> it does not load a stale cached value, clear the VMCB's LBR clean bit
> when L1 is running and bit 0 (LBR) of the DBGCTL field is changed from
> 0 to 1. (Note that this is already handled correctly when L2 is
> running.)
> 
> There is no need to clear the clean bit in the other direction,
> because when the VMCB's DBGCTL.LBR is 0, the VMCB's
> LBR_VIRTUALIZATION_ENABLE bit will be clear, and the CPU will not
> consult the VMCB's DBGCTL field at VMRUN.
> 
> Fixes: 1d5a1b5860ed ("KVM: x86: nSVM: correctly virtualize LBR msrs when L2 is running")
> Reported-by: Matteo Rizzo <matteorizzo@google.com>
> Reported-by: evn@google.com
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 153c12dbf3eb..b4e5a0684f57 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -816,6 +816,8 @@ void svm_enable_lbrv(struct kvm_vcpu *vcpu)
>  	/* Move the LBR msrs to the vmcb02 so that the guest can see them. */
>  	if (is_guest_mode(vcpu))
>  		svm_copy_lbrs(svm->vmcb, svm->vmcb01.ptr);
> +	else
> +		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
>  }
>  
>  static void svm_disable_lbrv(struct kvm_vcpu *vcpu)

Hi Jim,
I am thinking, is it possible to add a test in KVM Unit Tests that
covers this? Something where the stale cached value is loaded instead of
the correct one, without your patch.

Best regards,
Shivansh

Re: [PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Jim Mattson 3 months ago

On Thu, Nov 6, 2025 at 8:09 AM Shivansh Dhiman <shivansh.dhiman@amd.com> wrote:
>
> On 01-11-2025 05:32, Jim Mattson wrote:
> > With the VMCB's LBR_VIRTUALIZATION_ENABLE bit set, the CPU will load
> > the DebugCtl MSR from the VMCB's DBGCTL field at VMRUN. To ensure that
> > it does not load a stale cached value, clear the VMCB's LBR clean bit
> > when L1 is running and bit 0 (LBR) of the DBGCTL field is changed from
> > 0 to 1. (Note that this is already handled correctly when L2 is
> > running.)
> >
> > There is no need to clear the clean bit in the other direction,
> > because when the VMCB's DBGCTL.LBR is 0, the VMCB's
> > LBR_VIRTUALIZATION_ENABLE bit will be clear, and the CPU will not
> > consult the VMCB's DBGCTL field at VMRUN.
> >
> > Fixes: 1d5a1b5860ed ("KVM: x86: nSVM: correctly virtualize LBR msrs when L2 is running")
> > Reported-by: Matteo Rizzo <matteorizzo@google.com>
> > Reported-by: evn@google.com
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/svm/svm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 153c12dbf3eb..b4e5a0684f57 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -816,6 +816,8 @@ void svm_enable_lbrv(struct kvm_vcpu *vcpu)
> >       /* Move the LBR msrs to the vmcb02 so that the guest can see them. */
> >       if (is_guest_mode(vcpu))
> >               svm_copy_lbrs(svm->vmcb, svm->vmcb01.ptr);
> > +     else
> > +             vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
> >  }
> >
> >  static void svm_disable_lbrv(struct kvm_vcpu *vcpu)
>
> Hi Jim,
> I am thinking, is it possible to add a test in KVM Unit Tests that
> covers this? Something where the stale cached value is loaded instead of
> the correct one, without your patch.

Though permitted by the architectural specification, I don't know if
there is any hardware that caches the DBGCTL field when LBR
virtualization is disabled.

Re: [PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Shivansh Dhiman 3 months ago

On 06-11-2025 23:30, Jim Mattson wrote:
> On Thu, Nov 6, 2025 at 8:09 AM Shivansh Dhiman <shivansh.dhiman@amd.com> wrote:
>>
>> On 01-11-2025 05:32, Jim Mattson wrote:
>>> With the VMCB's LBR_VIRTUALIZATION_ENABLE bit set, the CPU will load
>>> the DebugCtl MSR from the VMCB's DBGCTL field at VMRUN. To ensure that
>>> it does not load a stale cached value, clear the VMCB's LBR clean bit
>>> when L1 is running and bit 0 (LBR) of the DBGCTL field is changed from
>>> 0 to 1. (Note that this is already handled correctly when L2 is
>>> running.)
>>>
>> Hi Jim,
>> I am thinking, is it possible to add a test in KVM Unit Tests that
>> covers this? Something where the stale cached value is loaded instead of
>> the correct one, without your patch.
> 
> Though permitted by the architectural specification, I don't know if
> there is any hardware that caches the DBGCTL field when LBR
> virtualization is disabled.

Alrighty.

Tested-by: Shivansh Dhiman <shivansh.dhiman@amd.com>

-Shivansh

Re: [PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Jim Mattson 3 months ago

I'm going to rescind this patch. Yosry is going to handle the
situation differently in his LBRV patch series.

Re: [PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Yosry Ahmed 3 months ago

On Fri, Oct 31, 2025 at 05:02:29PM -0700, Jim Mattson wrote:
> With the VMCB's LBR_VIRTUALIZATION_ENABLE bit set, the CPU will load
> the DebugCtl MSR from the VMCB's DBGCTL field at VMRUN. To ensure that
> it does not load a stale cached value, clear the VMCB's LBR clean bit
> when L1 is running and bit 0 (LBR) of the DBGCTL field is changed from
> 0 to 1. (Note that this is already handled correctly when L2 is
> running.)
> 
> There is no need to clear the clean bit in the other direction,
> because when the VMCB's DBGCTL.LBR is 0, the VMCB's
> LBR_VIRTUALIZATION_ENABLE bit will be clear, and the CPU will not
> consult the VMCB's DBGCTL field at VMRUN.

Is it worth the mental load of figuring out why we do it in
svm_enable_lbrv() but not svm_disable_lbrv()?

Maybe we can at least document it in svm_disable_lbrv() with a comment?

> 
> Fixes: 1d5a1b5860ed ("KVM: x86: nSVM: correctly virtualize LBR msrs when L2 is running")
> Reported-by: Matteo Rizzo <matteorizzo@google.com>
> Reported-by: evn@google.com
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 153c12dbf3eb..b4e5a0684f57 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -816,6 +816,8 @@ void svm_enable_lbrv(struct kvm_vcpu *vcpu)
>  	/* Move the LBR msrs to the vmcb02 so that the guest can see them. */
>  	if (is_guest_mode(vcpu))
>  		svm_copy_lbrs(svm->vmcb, svm->vmcb01.ptr);
> +	else
> +		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
>  }
>  
>  static void svm_disable_lbrv(struct kvm_vcpu *vcpu)
> -- 
> 2.51.2.1006.ga50a493c49-goog
>

Re: [PATCH] KVM: x86: SVM: Mark VMCB_LBR dirty when L1 sets DebugCtl[LBR]

Posted by Jim Mattson 3 months ago

On Mon, Nov 3, 2025 at 9:42 AM Yosry Ahmed <yosry.ahmed@linux.dev> wrote:
>
> On Fri, Oct 31, 2025 at 05:02:29PM -0700, Jim Mattson wrote:
> > With the VMCB's LBR_VIRTUALIZATION_ENABLE bit set, the CPU will load
> > the DebugCtl MSR from the VMCB's DBGCTL field at VMRUN. To ensure that
> > it does not load a stale cached value, clear the VMCB's LBR clean bit
> > when L1 is running and bit 0 (LBR) of the DBGCTL field is changed from
> > 0 to 1. (Note that this is already handled correctly when L2 is
> > running.)
> >
> > There is no need to clear the clean bit in the other direction,
> > because when the VMCB's DBGCTL.LBR is 0, the VMCB's
> > LBR_VIRTUALIZATION_ENABLE bit will be clear, and the CPU will not
> > consult the VMCB's DBGCTL field at VMRUN.
>
> Is it worth the mental load of figuring out why we do it in
> svm_enable_lbrv() but not svm_disable_lbrv()?
>
> Maybe we can at least document it in svm_disable_lbrv() with a comment?

I'm happy to do it in svm_disable_lbrv() as well, just to reduce the
cognitive load.