Re: [PATCH 4/7] KVM: SVM: Enable hardware-assisted LBR v2 virtualization

On 25-07-26 01:47, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] LBRv2 MSRs and MSR_AMD_DBG_EXTN_CFG are incorrectly intercepted for SEV-ES guests, causing a fatal hardware VMRUN error and exposing a KVM_BUG_ON to host userspace.
> - [High] Nested SVM breaks LBRv2 virtualization due to missing state copying in `svm_copy_lbrs` and missing MSRPM pass-through in KVM's merge array.
> - [High] The patch fails to enumerate AMD LBRv2 in KVM's supported CPUID capabilities, rendering the feature undiscoverable.
> - [High] The new LBRv2 MSRs are not added to KVM's MSR save/restore lists, breaking live migration.
> - [High] LBRv2 virtualization bypasses the global `lbrv` module parameter, allowing guests to enable hardware LBR virtualization even when administrators have explicitly disabled it.
> --
> 
> --- Patch [4]: [PATCH 4/7] KVM: SVM: Enable hardware-assisted LBR v2 virtualization ---
> commit ee1a6e59c9c7f66112084e471f36296d11c1b93d
> Author: Shivansh Dhiman <[email protected]>
> Subject: KVM: SVM: Enable hardware-assisted LBR v2 virtualization
> 
> This patch enables hardware-assisted LBR v2 virtualization in KVM for AMD SVM.
> It enables V_LBR to save and restore the guest's LBR v2 state in the VMCB,
> and lazily disables interception of the LBR v2 MSRs when the guest enables
> branch recording.
> 
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 13ac62604ca05..04e386b838d38 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -750,6 +750,41 @@ static bool svm_lbrv2_supported(struct kvm_vcpu *vcpu)
>>  	       kvm_vcpu_has_mediated_pmu(vcpu);
>>  }
> 
> [Severity: High]
> Is the F(AMD_LBR_V2) capability missing from kvm_cpu_cap_init()?
> 
> It appears F(AMD_LBR_V2) is missing from the initializers in
> kvm_cpu_cap_init(CPUID_8000_0022_EAX) in arch/x86/kvm/cpuid.c. Since KVM
> explicitly masks the host CPUID against registered capabilities, KVM will not
> report AMD_LBR_V2 as supported, making the feature undiscoverable by standard
> userspace VMMs.

This is already implemented in the fifth patch of this series, where LBRv2 is
advertised in kvm_initialize_cpu_caps().

> 
>>  
>> +static bool svm_lbrv2_active(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +	return svm_lbrv2_supported(vcpu) &&
>> +	       (svm->vmcb->save.dbg_extn_cfg & DBG_EXTN_CFG_LBRV2EN);
>> +}
>> +
>> +static void svm_recalc_lbrv2_msr_intercepts(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +	bool intercept = !svm_lbrv2_active(vcpu);
>> +	int i;
>> +
>> +	if (intercept == svm->lbrv2_msrs_intercepted)
>> +		return;
>> +
>> +	for (i = 0; i < 32; i++)
>> +		svm_set_intercept_for_msr(vcpu, MSR_AMD_SAMP_BR_FROM + i, MSR_TYPE_RW, intercept);
>> +
>> +	svm_set_intercept_for_msr(vcpu, MSR_AMD64_LBR_SELECT, MSR_TYPE_RW, intercept);
>> +
>> +	/*
>> +	 * DBG_EXTN_CFG stays permanently intercepted for non-SEV-ES guests so
>> +	 * KVM can observe LBRV2EN and lazily toggle V_LBR.  SEV-ES+ guests must
>> +	 * delegate LBR virtualization to the processor (per the APM), where
>> +	 * intercepting LBR MSRs can be fatal, so toggle it with the rest.  See
>> +	 * commit b7e4be0a224f ("KVM: SEV-ES: Delegate LBR virtualization to the
>> +	 * processor").
>> +	 */
>> +	if (is_sev_es_guest(vcpu))
>> +		svm_set_intercept_for_msr(vcpu, MSR_AMD_DBG_EXTN_CFG, MSR_TYPE_RW, intercept);
>> +
>> +	svm->lbrv2_msrs_intercepted = intercept;
>> +}
> 
> [Severity: High]
> Can this logic incorrectly force the interception of MSR_AMD_DBG_EXTN_CFG and
> LBRv2 MSRs for SEV-ES guests?
> 
> For SEV-ES guests, KVM's shadow copy of dbg_extn_cfg is always 0. Consequently,
> svm_lbrv2_active() evaluates to false, causing svm_recalc_lbrv2_msr_intercepts()
> to set intercept = true.
> 
> As documented in commit b7e4be0a224f, intercepting LBR MSRs when V_LBR is
> enabled triggers a fatal hardware VMRUN consistency check failure.
> 
> Additionally, because MSR_AMD_DBG_EXTN_CFG is intercepted, host userspace can
> write to it. Writing a value with LBRv2 disabled invokes svm_update_lbrv(),
> which then attempts to disable LBRV and hits a KVM_BUG_ON(is_sev_es_guest(vcpu))
> in __svm_disable_lbrv().

Currently, this series doesn't support LBRv2 for secured guests. I'll add this
as part in upcoming versions of this series.

> 
> [ ... ]
> 
>> @@ -886,9 +924,11 @@ 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));
>> +			    (svm->nested.ctl.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR)) ||
>> +			    svm_lbrv2_active(vcpu);
>>  
>>  	if (enable_lbrv && !current_enable_lbrv)
>>  		__svm_enable_lbrv(vcpu);
> 
> [Severity: High]
> Are we bypassing the global lbrv module parameter restriction here?
> 
> Legacy LBR virtualization correctly gates enablement behind the global lbrv
> parameter.
> 
> Because svm_lbrv2_active() checks only the guest state, if a guest sets
> LBRV2EN, enable_lbrv evaluates to true. This unconditionally enables hardware
> LBR virtualization via __svm_enable_lbrv(), even when administrators have
> explicitly disabled it with the lbrv parameter.

Legacy LBR doesn't have a CPUID bit, which is why it is gated through lbrv
parameter. And LBRv2 can't be enabled because KVM capability is cleared in
svm_set_cpu_caps() when lbrv=0.

> 
> [ ... ]
> 
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index f62bf19024cc0..01dae3828a7aa 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -363,6 +363,7 @@ struct vcpu_svm {
>>  	bool avic_irq_window;
>>  	bool x2avic_msrs_intercepted;
>>  	bool lbr_msrs_intercepted;
>> +	bool lbrv2_msrs_intercepted;
>>  
>>  	/* Guest GIF value, used when vGIF is not enabled */
>>  	bool guest_gif;
> 
> [Severity: High]
> Will the omission of the new LBRv2 fields from svm_copy_lbrs() and merge_msrs
> break nested SVM?
> 
> For nested SVM, svm_copy_lbrs() in arch/x86/kvm/svm/svm.h copies legacy LBR
> state but omits the new LBRv2 fields, leading to state loss across nested
> transitions.
> 
> Furthermore, the new LBRv2 MSRs are absent from the merge_msrs array in
> arch/x86/kvm/svm/nested.c, which defeats hardware acceleration by breaking
> MSRPM pass-through for L2.

Currently nested virtualization of LBRv2 isn't part of this series. Will add
that in upcoming versions.

> 
> [Severity: High]
> Are there missing entries in the MSR save/restore lists required for live
> migration?
> 
> The new LBRv2 MSRs are missing from msrs_to_save_base or msrs_to_save_pmu
> in arch/x86/kvm/msrs.c. Without these entries, they will not be included in
> KVM_GET_MSR_INDEX_LIST, which leads to silent data loss of hardware state
> across live migrations.

Already replied in reply to patch 3. Will add that in next version.