lkml.org 
[lkml]   [2022]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
On Mon, Sep 19, 2022, Like Xu wrote:
> @@ -162,20 +179,43 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
> {
> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> + struct kvm_cpuid_entry2 *entry;
> + union cpuid_0x80000022_ebx ebx;
>
> - if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
> - else
> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
> + pmu->version = 1;
> + if (kvm_pmu_cap.version > 1) {
> + pmu->version = min_t(unsigned int, 2, kvm_pmu_cap.version);

This is wrong, it forces the guest PMU verson to match the max version supported
by KVM. E.g. if userspace wants to expose v1 for whatever reason, pmu->version
will still end up 2+.

> + entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
> + if (entry) {
> + ebx.full = entry->ebx;
> + pmu->nr_arch_gp_counters = min3((unsigned int)ebx.split.num_core_pmc,
> + (unsigned int)kvm_pmu_cap.num_counters_gp,
> + (unsigned int)KVM_AMD_PMC_MAX_GENERIC);

This is technically wrong, the number of counters is supposed to be valid if and
only if v2 is supported. On a related topic, does KVM explode if userspace
specifies a bogus PMU version on Intel? I don't see any sanity checks there...

With a proper feature flag

pmu->version = 1;
if (kvm_cpu_has(X86_FEATURE_AMD_PMU_V2) &&
guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)) {
pmu->version = 2;

entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
if (entry) {
...

Though technically the "if (entry)" check is unnecesary.

> + }
> + }
> +
> + /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> + if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE)) {

Unnecessary braces.

> + pmu->nr_arch_gp_counters = max_t(unsigned int,
> + pmu->nr_arch_gp_counters,
> + AMD64_NUM_COUNTERS_CORE);

What happens if userspace sets X86_FEATURE_PERFCTR_CORE when its not supported?
E.g. will KVM be coerced into taking a #GP on a non-existent counter?


> + } else {
> + pmu->nr_arch_gp_counters = max_t(unsigned int,
> + pmu->nr_arch_gp_counters,
> + AMD64_NUM_COUNTERS);
> + }

\
 
 \ /
  Last update: 2022-10-28 00:48    [W:0.209 / U:0.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site