diff mbox series

[v4,08/12] KVM: x86/pmu: Disable vPMU if the minimum num of counters isn't met

Message ID 20230214050757.9623-9-likexu@tencent.com
State New
Headers
Series KVM: x86: Add AMD Guest PerfMonV2 PMU support |

Commit Message

Like Xu Feb. 14, 2023, 5:07 a.m. UTC 
  From: Like Xu <likexu@tencent.com>

For compatibility with old software, KVM/AMD should never report less
than four counters if vPMU is supported. 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.

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(+)

Comments

Sean Christopherson April 7, 2023, 12:06 a.m. UTC | #1 
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
>
Sean Christopherson April 7, 2023, 12:23 a.m. UTC | #2 
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 mbox series

Patch

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;