On Thu, Apr 06, 2023, Sean Christopherson wrote:
> On Tue, Feb 14, 2023, Like Xu wrote:
> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
> > ---
> > 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)
Actually, rather than bleed AMD details into common code, define a const int
kvm_pmu_ops, e.g.
const int MIN_NR_GP_COUNTERS
and then the above !kvm_pmu_cap.num_counters_gp can get replaced with a more generic
check. That will also give us a convenient location to document _why_ Intel
and AMD have different mins (in particular, AMD's lack of any way to enumerate
less than four counters to the guest).
E.g. end up with
if (enable_pmu) {
perf_get_x86_pmu_capability(&kvm_pmu_cap);
/*
* For Intel, only support guest architectural pmu
* on a host with architectural pmu.
*/
if ((is_intel && !kvm_pmu_cap.version) ||
(kvm_pmu_cap.num_counters_gp < pmu_ops->MIN_NR_GP_COUNTERS))
enable_pmu = false;
}
Hmm, and doesn't have to be done in this series, but we could do the same for the
min/max PMU versions.
> > + enable_pmu = false;
> > +
> > if (!enable_pmu) {
> > memset(&kvm_pmu_cap, 0, sizeof(kvm_pmu_cap));
> > return;
> > --
> > 2.39.1
> >
