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, 6 Apr 2023 17:06:55 -0700
In-Reply-To: <20230214050757.9623-9-likexu@tencent.com>
Mime-Version: 1.0
References: <20230214050757.9623-1-likexu@tencent.com> <20230214050757.9623-9-likexu@tencent.com>
Message-ID: <ZC9enygbWzJ59Ssb@google.com>
Subject: Re: [PATCH v4 08/12] KVM: x86/pmu: Disable vPMU if the minimum num of
 counters isn't met
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Tue, Feb 14, 2023, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> For compatibility with old software, KVM/AMD should never report less
> than four counters if vPMU is supported.

Explain _why_.  Anchor what "should" be done in hardware specifications and
architecture.

> Thus KVM should sanity check the number of counters enumerated by perf and
> explicitly disable vPMU support if the min isn't met. E.g. if KVM needs 4
> counters and perf says there are 3, then something is wrong and enumerating 4
> to the guest is only going to cause more troubles.

Again, state what the patch actually does, not what KVM "should do".  E.g.

  Disable PMU support when running on AMD and perf reports fewer than four
  general purpose counters.  All AMD PMUs must define at least four counters
  due to AMD's legacy architecture hardcoding the number of counters
  without providing a way to enumerate the number of counters to software,
  e.g. from AMD's APM.

    The legacy architecture defines four performance counters

  Virtualizing fewer than four counters can lead to guest instability as
  software expects four counters to be available.

> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/pmu.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index d1cc02c8da88..46db5404894e 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -170,6 +170,12 @@ static inline void kvm_init_pmu_capability(const struct kvm_pmu_ops *pmu_ops)
>  	if ((is_intel && !kvm_pmu_cap.version) || !kvm_pmu_cap.num_counters_gp)
>  		enable_pmu = false;
>  
> +	/*
> +	 * For AMD, disable vPMU if the minimum number of counters isn't met.
> +	 */

Doesn't need to be a multiple line comment.  This comment is also useless.  It's
quite clear from the code that PMU support is being disabled when there aren't
enough counters, what's missing is _why_.

> +	if (!is_intel && kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS)
> +		enable_pmu = false;
> +
>  	if (!enable_pmu) {
>  		memset(&kvm_pmu_cap, 0, sizeof(kvm_pmu_cap));
>  		return;
> -- 
> 2.39.1
>