Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 22:47:27 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Sandipan Das <sandipan.das@amd.com>
Subject: Re: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
Message-ID: <Y1sKf/PgwHwtAibK@google.com>
References: <20220919093453.71737-1-likexu@tencent.com>
 <20220919093453.71737-3-likexu@tencent.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220919093453.71737-3-likexu@tencent.com>
Precedence: bulk

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);
> +	}