Re: [PATCH 5/7] KVM: SVM: Advertise LbrExtV2 to userspace

On 25-07-26 01:36, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] KVM fails to gate the advertisement of AMD_LBR_V2 on a 16-entry host stack, violating the KVM CPUID API contract and causing a guest #GP upon PMU usage.
> --
> 
> --- Patch [5]: [PATCH 5/7] KVM: SVM: Advertise LbrExtV2 to userspace ---
> commit 9dead9faa6c004cfaa9aa0580ca255569674a36b
> Author: Shivansh Dhiman <[email protected]>
> 
> KVM: SVM: Advertise LbrExtV2 to userspace
>     
> This commit advertises AMD LBR v2 to userspace and reports the LBR stack
> depth in CPUID. It aims to gate this advertisement on LBR virtualization,
> the mediated PMU, PerfMonV2, and a 16-entry host stack.
> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 2698fa42cd971..d450a5b2499d9 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
> [ ... ]
>> @@ -1883,6 +1884,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>  		cpuid_entry_override(entry, CPUID_8000_0022_EAX);
>>  
>>  		ebx.split.num_core_pmc = kvm_pmu_cap.num_counters_gp;
>> +
>> +		if (kvm_cpu_cap_has(X86_FEATURE_AMD_LBR_V2))
>> +			ebx.split.lbr_v2_stack_sz =
>> +				min(kvm_pmu_cap.num_branches_lbr, SVM_LBR_V2_STACK_SIZE);
>> +
> 
> [Severity: Medium]
> If the host has kvm_pmu_cap.num_branches_lbr < 16, will this advertise an
> unsupported stack size to userspace?
> 
> The commit message indicates the advertisement should be gated on a 16-entry
> host stack, but it appears the host stack size is not checked before
> X86_FEATURE_AMD_LBR_V2 is set.

See below ...

> 
>>  		entry->ebx = ebx.full;
>>  		break;
>>  	}
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 04e386b838d38..4b84498e93be7 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
> [ ... ]
>> @@ -4846,6 +4860,10 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  	if (guest_cpuid_is_intel_compatible(vcpu))
>>  		guest_cpu_cap_clear(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
>>  
>> +	if (guest_cpu_cap_has(vcpu, X86_FEATURE_AMD_LBR_V2) &&
>> +	    cpuid_query_lbrv2_stack_size(vcpu) != SVM_LBR_V2_STACK_SIZE)
>> +		guest_cpu_cap_clear(vcpu, X86_FEATURE_AMD_LBR_V2);
>> +
> 
> [Severity: Medium]
> If userspace configures the vCPU with the advertised CPUID (where stack size
> is less than 16), will this silently drop the feature internally?
> 
> If the internal capability is cleared but the guest-visible CPUID remains set,
> would the guest receive a fatal #GP when attempting to access the LBR MSRs
> that it thinks are supported?

See below ...

> 
>>  	if (is_sev_guest(vcpu))
>>  		sev_vcpu_after_set_cpuid(svm);
>>  }
>> @@ -5692,6 +5710,10 @@ static __init void svm_set_cpu_caps(void)
>>  		if (kvm_pmu_cap.version != 2 ||
>>  		    !kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
>>  			kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);
>> +
>> +		if (!lbrv || !enable_mediated_pmu ||
>> +		    !kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2))
>> +			kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2);
> 
> [Severity: Medium]
> Should there be an additional check here for the host stack size?
> 
> If kvm_pmu_cap.num_branches_lbr != SVM_LBR_V2_STACK_SIZE, it seems
> X86_FEATURE_AMD_LBR_V2 is not cleared. This appears to lead to the issues
> mentioned above where KVM advertises a configuration it cannot faithfully
> virtualize.

Agreed, the set/clear logic is bit ambiguous. In v2 I'll gate the cap once
in svm_set_cpu_caps(). Emit the size unconditionally, and drop the
per-vCPU revoke that caused the cap/CPUID mismatch.

if (!lbrv || !enable_mediated_pmu ||
    !kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2) ||
    kvm_pmu_cap.num_branches_lbr != SVM_LBR_V2_STACK_SIZE)
	kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2);