Date:   Mon, 6 Feb 2023 22:22:06 +0000
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, Sandipan Das <sandipan.das@amd.com>,
        Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
Message-ID: <Y+F9jhfJtHEmVXyg@google.com>
References: <20221111102645.82001-1-likexu@tencent.com>
 <20221111102645.82001-7-likexu@tencent.com>
 <Y9BzYzEjAwUA+wuy@google.com>
 <c7e0a5b1-0fd0-e2b5-20ca-fc86a1d883db@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c7e0a5b1-0fd0-e2b5-20ca-fc86a1d883db@gmail.com>
Precedence: bulk

On Mon, Feb 06, 2023, Like Xu wrote:
> On 25/1/2023 8:10 am, Sean Christopherson wrote:
> > > +     }
> > > +
> > > +     /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> > 
> > Please expand this comment.  I'm still not entirely sure I've interpreted it correctly,
> > and I'm not sure that I agree with the code.
> 
> In the first version [1], I used almost the same if-elif-else sequence
> but the concerns from JimM[2] has changed my mind:
> 
> "Nonetheless, for compatibility with old software, Fn8000_0022_EBX can never
> report less than four counters (or six, if Fn8000_0001_ECX[PerfCtrExtCore] is set)."
> 
> Both in amd_pmu_refresh() and in __do_cpuid_func(), KVM implements
> this using the override approach of first applying the semantics of
> AMD_PMU_V2 and then implementing a minimum number of counters
> supported based on whether or not guest have  PERFCTR_CORE,
> the proposed if-elif-else does not fulfill this need.

Jim's comments were in the context of __do_cpuid_func(), i.e. KVM_GET_SUPPORTED_CPUID.
As far as guest CPUID is concerned, that's userspace's responsibility to get correct.

And for KVM_GET_SUPPORTED_CPUID, overriding kvm_pmu_cap.num_counters_gp is not
the correct approach.  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 6 counters and perf says there are 4, then something is wrong and enumerating
6 to the guest is only going to cause more problems.

> [1] 20220905123946.95223-4-likexu@tencent.com/
> [2] CALMp9eQObuiJGV=YrAU9Fw+KoXfJtZMJ-KUs-qCOVd+R9zGBpw@mail.gmail.com