Re: [PATCH v4 1/5] KVM: SVM: Refactor svm_update_lbrv()

[Shivansh Dhiman <shivansh.dhiman@amd.com>]

Hi Sean,

Thanks for reviewing my series. Replying to v4.

On 25-09-26 23:01, Sean Christopherson wrote:
> The shortlog is effectively "do work".  Be more precise.

Ack.

> 
> On Tue, Jul 21, 2026, Shivansh Dhiman wrote:
>> Rewrite the enable_lbrv computation in svm_update_lbrv() as a series of
>> 'if' statements. Rename nested_vmcb12_has_lbrv() to nested_lbrv_enabled(),
>> expose it, and use it instead of open-coding the nested LBRV check.
> 
> This is super duper obviously two separate and *barely* related patches.

Makes sense.

> 
>> No functional change intended.
>>
>> Suggested-by: Yosry Ahmed <yosry@kernel.org>
>> Signed-off-by: Shivansh Dhiman <shivansh.dhiman@amd.com>
>> Reviewed-by: Yosry Ahmed <yosry@kernel.org>
>> ---
>> Changelog:
>> v3 -> v4:
>>  * Rename nested_vmcb12_has_lbrv() to nested_lbrv_enabled() (Yosry).
> 
> I don't like this change.  Unlike nested_vgif_enabled(), this is *only* checking
> the effective vmcb12.  Once the guest_cpu_cap_has() code is moved elsewhere, I
> don't see any point in keeping the helper.

Ack.

> 
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index ef69a51ab27f..e9f2456982d4 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -880,9 +880,13 @@ void svm_update_lbrv(struct kvm_vcpu *vcpu)
>>  {
>>  	struct vcpu_svm *svm = to_svm(vcpu);
>>  	bool current_enable_lbrv = svm->vmcb->control.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR;
>> -	bool enable_lbrv = (svm->vmcb->save.dbgctl & DEBUGCTLMSR_LBR) ||
>> -			    (is_guest_mode(vcpu) && guest_cpu_cap_has(vcpu, X86_FEATURE_LBRV) &&
>> -			    (svm->nested.ctl.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR));
>> +	bool enable_lbrv = false;
>> +
>> +	if (svm->vmcb->save.dbgctl & DEBUGCTLMSR_LBR)
>> +		enable_lbrv = true;
>> +
>> +	if (is_guest_mode(vcpu) && nested_lbrv_enabled(vcpu))
>> +		enable_lbrv = true;
> 
> This is begging for short-circuit logic (which was kinda the point of the existing
> code).  An if-elif is silly, and I agree that squeezing everything into variable
> initialization is hard to read, so I think we should do:
> 
> static bool svm_need_lbr_virtualization(struct kvm_vcpu *vcpu)
> {
> 	struct vcpu_svm *svm = to_svm(vcpu);
> 
> 	if (svm->vmcb->save.dbgctl & (DEBUGCTLMSR_LBR | DEBUGCTLMSR_BUS_LOCK_DETECT))
> 		return true;
> 
> 	return is_guest_mode(vcpu) &&
> 	       (svm->nested.ctl.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR);
> }
> 
> 
> 		bool enable_lbrv = svm_need_lbr_virtualization(vcpu);

That looks cleaner. There's another series I've posted on SVM vLBRv2 which will
benefit from this refactor. Thanks.

-Shivansh