Re: [PATCH v2 07/12] KVM: SVM: Advertise LbrExtV2 to userspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: x86: __do_cpuid_func() reports lbr_v2_stack_sz when LBRv2 is disabled
- [Medium] KVM: SVM: AMD_LBR_V2 not cleared in svm_set_cpu_caps() when !enable_pmu
--

commit 7e80a77e67ad5de914e6e6583e6c7b559aacbce2
Author: Shivansh Dhiman <shivansh.dhiman@amd.com>

KVM: SVM: Advertise LbrExtV2 to userspace

This commit advertises X86_FEATURE_AMD_LBR_V2 and reports the LBRv2 stack
depth in CPUID leaf 0x80000022 when LBR virtualization, the mediated PMU,
PerfMonV2, and a 16-entry host LBR stack are available.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index adafd6fa2c542..bb24d83621328 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1258,6 +1258,7 @@ void kvm_initialize_cpu_caps(void)
>  
>  	kvm_cpu_cap_init(CPUID_8000_0022_EAX,
>  		F(PERFMON_V2),
> +		SCATTERED_F(AMD_LBR_V2),
>  	);
>  
>  	if (!static_cpu_has_bug(X86_BUG_NULL_SEG))
> @@ -1904,6 +1905,8 @@ 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;
> +		ebx.split.lbr_v2_stack_sz = kvm_pmu_cap.num_branches_lbr;

[Severity: Medium]
Should __do_cpuid_func() in arch/x86/kvm/cpuid.c check
kvm_cpu_cap_has(X86_FEATURE_AMD_LBR_V2) before populating
ebx.split.lbr_v2_stack_sz?

When enable_pmu is true on an LBRv2-capable host, kvm_init_pmu_capability()
copies kvm_host_pmu into kvm_pmu_cap:

arch/x86/kvm/pmu.c:kvm_init_pmu_capability() {
    ...
    memcpy(&kvm_pmu_cap, &kvm_host_pmu, sizeof(kvm_host_pmu));
    ...
}

In svm_set_cpu_caps(), if lbrv is false, enable_mediated_pmu is false, or
kvm_pmu_cap.num_branches_lbr is not SVM_LBR_V2_STACK_SIZE, KVM clears
X86_FEATURE_AMD_LBR_V2 while X86_FEATURE_PERFMON_V2 remains set and
kvm_pmu_cap.num_branches_lbr remains non-zero.

When userspace invokes KVM_GET_SUPPORTED_CPUID, __do_cpuid_func() for leaf
0x80000022 only checks enable_pmu and
kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2). Wouldn't this cause
KVM_GET_SUPPORTED_CPUID to report a non-zero lbr_v2_stack_sz in EBX even
when X86_FEATURE_AMD_LBR_V2 is cleared in EAX?

> +
>  		entry->ebx = ebx.full;
>  		break;
>  	}
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 69f9ea746bb6f..ea7535ccc3fe6 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5843,6 +5843,18 @@ 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);
> +
> +		/*
> +		 * LBRv2 requires LBR virtualization and the mediated PMU to
> +		 * let the guest own the LBR stack, and is enumerated through
> +		 * PerfMonV2's CPUID leaf.  KVM also can't faithfully virtualize
> +		 * a stack depth other than the architectural 16 entries, as the
> +		 * VMCB save area provides exactly that many entries.
> +		 */
> +		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);
>  	}

[Severity: Medium]
In svm_set_cpu_caps() in arch/x86/kvm/svm/svm.c, both
kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2) and
kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2) are inside the if (enable_pmu)
block:

arch/x86/kvm/svm/svm.c:svm_set_cpu_caps() {
    ...
    if (enable_pmu) {
        ...
        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_pmu_cap.num_branches_lbr != SVM_LBR_V2_STACK_SIZE)
            kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2);
    }
    ...
}

When enable_pmu is false on an LBRv2-capable host, kvm_initialize_cpu_caps()
still initializes CPUID_8000_0022_EAX with F(PERFMON_V2) and
SCATTERED_F(AMD_LBR_V2), and svm_set_cpu_caps() skips clearing them.

Because kvm_vcpu_after_set_cpuid() populates vcpu->arch.cpu_caps from
kvm_cpu_caps, doesn't this leave kvm_cpu_cap_has(X86_FEATURE_AMD_LBR_V2)
true globally and allow guest_cpu_cap_has(vcpu, X86_FEATURE_AMD_LBR_V2) to
be set if userspace passes leaf 0x80000022 in KVM_SET_CPUID2 when enable_pmu
is false?

With the earlier commit ("KVM: nSVM: Propagate LBRv2 state across nested
transitions") in this series, svm_copy_vmrun_state() and
__nested_copy_vmcb_save_to_cache() check
kvm_cpu_cap_has(X86_FEATURE_AMD_LBR_V2), while nested_vmcb02_prepare_save(),
nested_svm_vmexit_update_vmcb12(), and nested_svm_vmexit() check
guest_cpu_cap_has(vcpu, X86_FEATURE_AMD_LBR_V2). Wouldn't leaving
X86_FEATURE_AMD_LBR_V2 set when enable_pmu is false cause KVM to copy LBRv2
state into the L1 MSR_VM_HSAVE_PA area during SMM transitions and propagate
DBG_EXTN_CFG_LBRV2EN across nested transitions without mediated PMU support?

>  
>  	/* CPUID 0x8000001F (SME/SEV features) */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019210532.378628-1-shivansh.dhiman@amd.com?part=7