diff mbox series

[v4,12/12] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022

Message ID 20230214050757.9623-13-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>

CPUID leaf 0x80000022 i.e. ExtPerfMonAndDbg advertises some new
performance monitoring features for AMD processors.

Bit 0 of EAX indicates support for Performance Monitoring Version 2
(PerfMonV2) features. If found to be set during PMU initialization,
the EBX bits of the same CPUID function can be used to determine
the number of available PMCs for different PMU types.

Expose the relevant bits via KVM_GET_SUPPORTED_CPUID so that
guests can make use of the PerfMonV2 features.

Co-developed-by: Sandipan Das <sandipan.das@amd.com>
Signed-off-by: Sandipan Das <sandipan.das@amd.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/cpuid.c   | 24 +++++++++++++++++++++++-
 arch/x86/kvm/svm/svm.c |  6 ++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

Comments

Sean Christopherson April 7, 2023, 1:50 a.m. UTC | #1 
On Tue, Feb 14, 2023, Like Xu wrote:
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f4a4691b4f4e..2472fa8746c2 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4916,6 +4916,12 @@ static __init void svm_set_cpu_caps(void)
>  		} else {
>  			/* AMD PMU PERFCTR_CORE CPUID */
>  			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
> +			/*
> +			 * KVM only supports AMD PerfMon V2, even if it supports V3+.

Ha!  A perfect example of why I strongly prefer that changelogs and comments avoid
pronouns.  The above "it" reads as:

			 * KVM only supports AMD PerfMon V2, even if KVM supports V3+.

which is clearly nonsensical.


> +			 * For PerfMon V3+, it's unsafe to expect V2 bit is set or cleared.

If it's unsafe to assume anything v3+ implying v2 support, then it's definitely
not safe to assume that KVM can blindly set v2 without future changes.  I don't
see any reason not to do

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index bd324962bb7e..1192f605ad47 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -756,6 +756,10 @@ void kvm_set_cpu_caps(void)
                F(NULL_SEL_CLR_BASE) | F(AUTOIBRS) | 0 /* PrefetchCtlMsr */
        );
 
+       kvm_cpu_cap_mask(CPUID_8000_0022_EAX,
+               F(PERFMON_V2)
+       );
+
        /*
         * Synthesize "LFENCE is serializing" into the AMD-defined entry in
         * KVM's supported CPUID if the feature is reported as supported by the


and then this code can be:

			if (kvm_pmu_cap.version != 2)
				kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);

Ah, but presumably the

		if (kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS_CORE)

path also needs to clear PERFMON_V2.  I think I'd still vote to grab host CPUID
and clear here (instead of setting).

What is the relationship between PERFCTR_CORE and PERFMON_V2?  E.g. if v2 depends
on having PERFCTR_CORE, then we can do:

	if (enable_pmu) {
		if (kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS_CORE)
			kvm_pmu_cap.num_counters_gp = AMD64_NUM_COUNTERS;
		else
			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);

		if (kvm_pmu_cap.version != 2 ||
		    !kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
			kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);
Like Xu April 7, 2023, 7:19 a.m. UTC | #2 
On 7/4/2023 9:50 am, Sean Christopherson wrote:
> On Tue, Feb 14, 2023, Like Xu wrote:
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index f4a4691b4f4e..2472fa8746c2 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -4916,6 +4916,12 @@ static __init void svm_set_cpu_caps(void)
>>   		} else {
>>   			/* AMD PMU PERFCTR_CORE CPUID */
>>   			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
>> +			/*
>> +			 * KVM only supports AMD PerfMon V2, even if it supports V3+.
> 
> Ha!  A perfect example of why I strongly prefer that changelogs and comments avoid
> pronouns.  The above "it" reads as:
> 
> 			 * KVM only supports AMD PerfMon V2, even if KVM supports V3+.
> 
> which is clearly nonsensical.

I get your point. Thanks.

> 
> 
>> +			 * For PerfMon V3+, it's unsafe to expect V2 bit is set or cleared.
> 
> If it's unsafe to assume anything v3+ implying v2 support, then it's definitely
> not safe to assume that KVM can blindly set v2 without future changes.  I don't
> see any reason not to do
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bd324962bb7e..1192f605ad47 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -756,6 +756,10 @@ void kvm_set_cpu_caps(void)
>                  F(NULL_SEL_CLR_BASE) | F(AUTOIBRS) | 0 /* PrefetchCtlMsr */
>          );
>   
> +       kvm_cpu_cap_mask(CPUID_8000_0022_EAX,
> +               F(PERFMON_V2)
> +       );
> +
>          /*
>           * Synthesize "LFENCE is serializing" into the AMD-defined entry in
>           * KVM's supported CPUID if the feature is reported as supported by the
> 
> 
> and then this code can be:
> 
> 			if (kvm_pmu_cap.version != 2)
> 				kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);
> 
> Ah, but presumably the
> 
> 		if (kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS_CORE)
> 
> path also needs to clear PERFMON_V2.  I think I'd still vote to grab host CPUID
> and clear here (instead of setting).

Looks good to me.

> 
> What is the relationship between PERFCTR_CORE and PERFMON_V2?  E.g. if v2 depends
> on having PERFCTR_CORE, then we can do:

Yes, the PERFCTR_CORE bit will always be set if the v2 bit is set.

> 
> 	if (enable_pmu) {
> 		if (kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS_CORE)
> 			kvm_pmu_cap.num_counters_gp = AMD64_NUM_COUNTERS;
> 		else
> 			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
> 
> 		if (kvm_pmu_cap.version != 2 ||
> 		    !kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
> 			kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b0bb5f9f5307..274cae531d7f 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1124,7 +1124,7 @@  static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->edx = 0;
 		break;
 	case 0x80000000:
-		entry->eax = min(entry->eax, 0x80000021);
+		entry->eax = min(entry->eax, 0x80000022);
 		/*
 		 * Serializing LFENCE is reported in a multitude of ways, and
 		 * NullSegClearsBase is not reported in CPUID on Zen2; help
@@ -1247,6 +1247,28 @@  static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		if (!static_cpu_has_bug(X86_BUG_NULL_SEG))
 			entry->eax |= BIT(6);
 		break;
+	/* AMD Extended Performance Monitoring and Debug */
+	case 0x80000022: {
+		union cpuid_0x80000022_ebx ebx;
+
+		entry->ecx = entry->edx = 0;
+		if (!enable_pmu || !kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2)) {
+			entry->eax = entry->ebx;
+			break;
+		}
+
+		cpuid_entry_override(entry, CPUID_8000_0022_EAX);
+
+		if (kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2))
+			ebx.split.num_core_pmc = kvm_pmu_cap.num_counters_gp;
+		else if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
+			ebx.split.num_core_pmc = AMD64_NUM_COUNTERS_CORE;
+		else
+			ebx.split.num_core_pmc = AMD64_NUM_COUNTERS;
+
+		entry->ebx = ebx.full;
+		break;
+	}
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
 		/*Just support up to 0xC0000004 now*/
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f4a4691b4f4e..2472fa8746c2 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4916,6 +4916,12 @@  static __init void svm_set_cpu_caps(void)
 		} else {
 			/* AMD PMU PERFCTR_CORE CPUID */
 			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
+			/*
+			 * KVM only supports AMD PerfMon V2, even if it supports V3+.
+			 * For PerfMon V3+, it's unsafe to expect V2 bit is set or cleared.
+			 */
+			if (kvm_pmu_cap.version > 1)
+				kvm_cpu_cap_set(X86_FEATURE_PERFMON_V2);
 		}
 	}