| Message ID | 20230214050757.9623-9-likexu@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Add AMD Guest PerfMonV2 PMU support | expand |
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 >
On Thu, Apr 06, 2023, Sean Christopherson wrote: > On Tue, Feb 14, 2023, Like Xu wrote: > > 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) 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 > >
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. + */ + 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;