[PATCH v2 0/4] Support Perf Extension on AMD KVM guests

[PATCH v2 0/4] Support Perf Extension on AMD KVM guests

[Janakarajan Natarajan]

This patchset adds support for Perf Extension on AMD KVM guests.

When perf runs on a guest with family = 15h || 17h, the MSRs that are
accessed, when the Perf Extension flag is made available, differ from
the existing K7 MSRs. The accesses are to the AMD Core Performance
Extension counters which provide 2 extra counters and new MSRs for both
the event select and counter registers.

Routines are introduced to choose the proper MSR based on the guest
family. Since the new event select and counter MSRs are interleaved
and K7 MSRs are contiguous, the logic to map them to the gp_counters[]
is changed.

Additionally, a fix is provided for CPUID_8000_0001_ECX in reverse_cpuid[]
to change the CPUID function from 0xc0000001 to 0x80000001.

This patchset has been tested with Family 17h and Opteron G1 guests.

v1->v2:
* Rearranged MSR #defines based on Boris's suggestion.

Janakarajan Natarajan (4):
  x86/kvm/cpuid: Fix CPUID function for word 6 (80000001_ECX)
  Add AMD Core Perf Extension MSRs
  Add support for AMD Core Perf Extension in guest
  Expose AMD Core Perf Extension flag to guests

 arch/x86/include/asm/msr-index.h |  14 +++++
 arch/x86/kvm/cpuid.c             |   8 ++-
 arch/x86/kvm/cpuid.h             |   2 +-
 arch/x86/kvm/pmu_amd.c           | 133 ++++++++++++++++++++++++++++++++++-----
 arch/x86/kvm/x86.c               |   1 +
 5 files changed, 142 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] x86/kvm/cpuid: Fix CPUID function for word 6 (80000001_ECX)

[Janakarajan Natarajan]

The function for CPUID 80000001 ECX is set to 0xc0000001. Set it to
0x80000001.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/kvm/cpuid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 0bc5c13..b21b1d2 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -43,7 +43,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
 	[CPUID_8086_0001_EDX] = {0x80860001, 0, CPUID_EDX},
 	[CPUID_1_ECX]         = {         1, 0, CPUID_ECX},
 	[CPUID_C000_0001_EDX] = {0xc0000001, 0, CPUID_EDX},
-	[CPUID_8000_0001_ECX] = {0xc0000001, 0, CPUID_ECX},
+	[CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
 	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
 	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
 	[CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},
-- 
2.7.4

[PATCH v2 2/4] Add AMD Core Perf Extension MSRs

[Janakarajan Natarajan]

Add the EventSelect and Counter MSRs for AMD Core Perf Extension.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/include/asm/msr-index.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 17f5c12..1dcbd29 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -337,7 +337,21 @@
 
 /* Fam 15h MSRs */
 #define MSR_F15H_PERF_CTL		0xc0010200
+#define MSR_F15H_PERF_CTL0		MSR_F15H_PERF_CTL
+#define MSR_F15H_PERF_CTL1		(MSR_F15H_PERF_CTL + 2)
+#define MSR_F15H_PERF_CTL2		(MSR_F15H_PERF_CTL + 4)
+#define MSR_F15H_PERF_CTL3		(MSR_F15H_PERF_CTL + 6)
+#define MSR_F15H_PERF_CTL4		(MSR_F15H_PERF_CTL + 8)
+#define MSR_F15H_PERF_CTL5		(MSR_F15H_PERF_CTL + 10)
+
 #define MSR_F15H_PERF_CTR		0xc0010201
+#define MSR_F15H_PERF_CTR0		MSR_F15H_PERF_CTR
+#define MSR_F15H_PERF_CTR1		(MSR_F15H_PERF_CTR + 2)
+#define MSR_F15H_PERF_CTR2		(MSR_F15H_PERF_CTR + 4)
+#define MSR_F15H_PERF_CTR3		(MSR_F15H_PERF_CTR + 6)
+#define MSR_F15H_PERF_CTR4		(MSR_F15H_PERF_CTR + 8)
+#define MSR_F15H_PERF_CTR5		(MSR_F15H_PERF_CTR + 10)
+
 #define MSR_F15H_NB_PERF_CTL		0xc0010240
 #define MSR_F15H_NB_PERF_CTR		0xc0010241
 #define MSR_F15H_PTSC			0xc0010280
-- 
2.7.4

[PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Janakarajan Natarajan]

This patch adds support for AMD Core Performance counters in the guest.
The base event select and counter MSRs are changed. In addition, with
the core extension, there are 2 extra counters available for performance
measurements for a total of 6.

With the new MSRs, the logic to map them to the gp_counters[] is changed.
New functions are introduced to get the right base MSRs and to check the
validity of the get/set MSRs.

If the guest has vcpus of either family 16h or a generation < 15h, it
falls back to using K7 MSRs and the number of counters the guest can
access is set to 4.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/kvm/pmu_amd.c | 133 +++++++++++++++++++++++++++++++++++++++++++------
 arch/x86/kvm/x86.c     |   1 +
 2 files changed, 120 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/pmu_amd.c b/arch/x86/kvm/pmu_amd.c
index cd94443..2c694446 100644
--- a/arch/x86/kvm/pmu_amd.c
+++ b/arch/x86/kvm/pmu_amd.c
@@ -19,6 +19,11 @@
 #include "lapic.h"
 #include "pmu.h"
 
+enum pmu_type {
+	PMU_TYPE_COUNTER = 0,
+	PMU_TYPE_EVNTSEL,
+};
+
 /* duplicated from amd_perfmon_event_map, K7 and above should work. */
 static struct kvm_event_hw_type_mapping amd_event_mapping[] = {
 	[0] = { 0x76, 0x00, PERF_COUNT_HW_CPU_CYCLES },
@@ -31,6 +36,86 @@ static struct kvm_event_hw_type_mapping amd_event_mapping[] = {
 	[7] = { 0xd1, 0x00, PERF_COUNT_HW_STALLED_CYCLES_BACKEND },
 };
 
+static unsigned int get_msr_base(struct kvm_pmu *pmu, enum pmu_type type)
+{
+	struct kvm_vcpu *vcpu = pmu_to_vcpu(pmu);
+	int family;
+	bool has_perf_ext;
+
+	family = guest_cpuid_family(vcpu);
+	has_perf_ext = guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE);
+
+	switch (family) {
+	case 0x15:
+	case 0x17:
+		if (has_perf_ext) {
+			if (type == PMU_TYPE_COUNTER)
+				return MSR_F15H_PERF_CTR;
+			else
+				return MSR_F15H_PERF_CTL;
+			break;
+		}
+		/*
+		 * Fall-through because the K7 MSRs are
+		 * backwards compatible
+		 */
+	default:
+		if (type == PMU_TYPE_COUNTER)
+			return MSR_K7_PERFCTR0;
+		else
+			return MSR_K7_EVNTSEL0;
+	}
+}
+
+static inline struct kvm_pmc *get_gp_pmc_amd(struct kvm_pmu *pmu, u32 msr,
+					     enum pmu_type type)
+{
+	unsigned int base = get_msr_base(pmu, type);
+
+	if (base == MSR_F15H_PERF_CTL) {
+		switch (msr) {
+		case MSR_F15H_PERF_CTL0:
+		case MSR_F15H_PERF_CTL1:
+		case MSR_F15H_PERF_CTL2:
+		case MSR_F15H_PERF_CTL3:
+		case MSR_F15H_PERF_CTL4:
+		case MSR_F15H_PERF_CTL5:
+			/*
+			 * AMD Perf Extension MSRs are not continuous.
+			 *
+			 * E.g. MSR_F15H_PERF_CTR0 -> 0xc0010201
+			 *	MSR_F15H_PERF_CTR1 -> 0xc0010203
+			 *
+			 * These are mapped to work with gp_counters[].
+			 * The index into the array is calculated by
+			 * dividing the difference between the requested
+			 * msr and the msr base by 2.
+			 *
+			 * E.g. MSR_F15H_PERF_CTR1 uses
+			 *	->gp_counters[(0xc0010203-0xc0010201)/2]
+			 *	->gp_counters[1]
+			 */
+			return &pmu->gp_counters[(msr - base) >> 1];
+		default:
+			return NULL;
+		}
+	} else if (base == MSR_F15H_PERF_CTR) {
+		switch (msr) {
+		case MSR_F15H_PERF_CTR0:
+		case MSR_F15H_PERF_CTR1:
+		case MSR_F15H_PERF_CTR2:
+		case MSR_F15H_PERF_CTR3:
+		case MSR_F15H_PERF_CTR4:
+		case MSR_F15H_PERF_CTR5:
+			return &pmu->gp_counters[(msr - base) >> 1];
+		default:
+			return NULL;
+		}
+	} else {
+		return get_gp_pmc(pmu, msr, base);
+	}
+}
+
 static unsigned amd_find_arch_event(struct kvm_pmu *pmu,
 				    u8 event_select,
 				    u8 unit_mask)
@@ -64,7 +149,20 @@ static bool amd_pmc_is_enabled(struct kvm_pmc *pmc)
 
 static struct kvm_pmc *amd_pmc_idx_to_pmc(struct kvm_pmu *pmu, int pmc_idx)
 {
-	return get_gp_pmc(pmu, MSR_K7_EVNTSEL0 + pmc_idx, MSR_K7_EVNTSEL0);
+	unsigned int base = get_msr_base(pmu, PMU_TYPE_COUNTER);
+	unsigned int family;
+
+	family = guest_cpuid_family(pmu_to_vcpu(pmu));
+
+	if (family == 0x15 || family == 0x17) {
+		/*
+		 * The idx is contiguous. The MSRs are not. The counter MSRs
+		 * are interleaved with the event select MSRs.
+		 */
+		pmc_idx *= 2;
+	}
+
+	return get_gp_pmc_amd(pmu, base + pmc_idx, PMU_TYPE_COUNTER);
 }
 
 /* returns 0 if idx's corresponding MSR exists; otherwise returns 1. */
@@ -96,8 +194,8 @@ static bool amd_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	int ret = false;
 
-	ret = get_gp_pmc(pmu, msr, MSR_K7_PERFCTR0) ||
-		get_gp_pmc(pmu, msr, MSR_K7_EVNTSEL0);
+	ret = get_gp_pmc_amd(pmu, msr, PMU_TYPE_COUNTER) ||
+		get_gp_pmc_amd(pmu, msr, PMU_TYPE_EVNTSEL);
 
 	return ret;
 }
@@ -107,14 +205,14 @@ static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	struct kvm_pmc *pmc;
 
-	/* MSR_K7_PERFCTRn */
-	pmc = get_gp_pmc(pmu, msr, MSR_K7_PERFCTR0);
+	/* MSR_PERFCTRn */
+	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_COUNTER);
 	if (pmc) {
 		*data = pmc_read_counter(pmc);
 		return 0;
 	}
-	/* MSR_K7_EVNTSELn */
-	pmc = get_gp_pmc(pmu, msr, MSR_K7_EVNTSEL0);
+	/* MSR_EVNTSELn */
+	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_EVNTSEL);
 	if (pmc) {
 		*data = pmc->eventsel;
 		return 0;
@@ -130,14 +228,14 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u32 msr = msr_info->index;
 	u64 data = msr_info->data;
 
-	/* MSR_K7_PERFCTRn */
-	pmc = get_gp_pmc(pmu, msr, MSR_K7_PERFCTR0);
+	/* MSR_PERFCTRn */
+	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_COUNTER);
 	if (pmc) {
 		pmc->counter += data - pmc_read_counter(pmc);
 		return 0;
 	}
-	/* MSR_K7_EVNTSELn */
-	pmc = get_gp_pmc(pmu, msr, MSR_K7_EVNTSEL0);
+	/* MSR_EVNTSELn */
+	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_EVNTSEL);
 	if (pmc) {
 		if (data == pmc->eventsel)
 			return 0;
@@ -153,8 +251,15 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+	int family, nr_counters;
+
+	family = guest_cpuid_family(vcpu);
+	if (family == 0x15 || family == 0x17)
+		nr_counters = AMD64_NUM_COUNTERS_CORE;
+	else
+		nr_counters = AMD64_NUM_COUNTERS;
 
-	pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
+	pmu->nr_arch_gp_counters = nr_counters;
 	pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << 48) - 1;
 	pmu->reserved_bits = 0xffffffff00200000ull;
 	/* not applicable to AMD; but clean them to prevent any fall out */
@@ -169,7 +274,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	int i;
 
-	for (i = 0; i < AMD64_NUM_COUNTERS ; i++) {
+	for (i = 0; i < AMD64_NUM_COUNTERS_CORE ; i++) {
 		pmu->gp_counters[i].type = KVM_PMC_GP;
 		pmu->gp_counters[i].vcpu = vcpu;
 		pmu->gp_counters[i].idx = i;
@@ -181,7 +286,7 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu)
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	int i;
 
-	for (i = 0; i < AMD64_NUM_COUNTERS; i++) {
+	for (i = 0; i < AMD64_NUM_COUNTERS_CORE; i++) {
 		struct kvm_pmc *pmc = &pmu->gp_counters[i];
 
 		pmc_stop_counter(pmc);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 03869eb..5a6ad6f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2433,6 +2433,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_AMD64_DC_CFG:
 		msr_info->data = 0;
 		break;
+	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5:
 	case MSR_K7_EVNTSEL0 ... MSR_K7_EVNTSEL3:
 	case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
 	case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
-- 
2.7.4

[PATCH v2 4/4] Expose AMD Core Perf Extension flag to guests

[Janakarajan Natarajan]

Expose the AMD Core Perf Extension flag to the guests.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0099e10..8c95a7c 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -55,6 +55,11 @@ bool kvm_mpx_supported(void)
 }
 EXPORT_SYMBOL_GPL(kvm_mpx_supported);
 
+bool perf_ext_supported(void)
+{
+	return boot_cpu_has(X86_FEATURE_PERFCTR_CORE);
+}
+
 u64 kvm_supported_xcr0(void)
 {
 	u64 xcr0 = KVM_SUPPORTED_XCR0 & host_xcr0;
@@ -327,6 +332,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	unsigned f_invpcid = kvm_x86_ops->invpcid_supported() ? F(INVPCID) : 0;
 	unsigned f_mpx = kvm_mpx_supported() ? F(MPX) : 0;
 	unsigned f_xsaves = kvm_x86_ops->xsaves_supported() ? F(XSAVES) : 0;
+	unsigned f_perfext = perf_ext_supported() ? F(PERFCTR_CORE) : 0;
 
 	/* cpuid 1.edx */
 	const u32 kvm_cpuid_1_edx_x86_features =
@@ -365,7 +371,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		F(LAHF_LM) | F(CMP_LEGACY) | 0 /*SVM*/ | 0 /* ExtApicSpace */ |
 		F(CR8_LEGACY) | F(ABM) | F(SSE4A) | F(MISALIGNSSE) |
 		F(3DNOWPREFETCH) | F(OSVW) | 0 /* IBS */ | F(XOP) |
-		0 /* SKINIT, WDT, LWP */ | F(FMA4) | F(TBM);
+		0 /* SKINIT, WDT, LWP */ | F(FMA4) | F(TBM) | f_perfext;
 
 	/* cpuid 0xC0000001.edx */
 	const u32 kvm_cpuid_C000_0001_edx_x86_features =
-- 
2.7.4

Re: [PATCH v2 1/4] x86/kvm/cpuid: Fix CPUID function for word 6 (80000001_ECX)

[Jim Mattson]

Reviewed-by: Jim Mattson <jmattson@google.com>

On Mon, Nov 6, 2017 at 9:44 AM, Janakarajan Natarajan
<Janakarajan.Natarajan@amd.com> wrote:
> The function for CPUID 80000001 ECX is set to 0xc0000001. Set it to
> 0x80000001.
>
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  arch/x86/kvm/cpuid.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 0bc5c13..b21b1d2 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -43,7 +43,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
>         [CPUID_8086_0001_EDX] = {0x80860001, 0, CPUID_EDX},
>         [CPUID_1_ECX]         = {         1, 0, CPUID_ECX},
>         [CPUID_C000_0001_EDX] = {0xc0000001, 0, CPUID_EDX},
> -       [CPUID_8000_0001_ECX] = {0xc0000001, 0, CPUID_ECX},
> +       [CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
>         [CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
>         [CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
>         [CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},
> --
> 2.7.4
>

Re: [PATCH v2 1/4] x86/kvm/cpuid: Fix CPUID function for word 6 (80000001_ECX)

[Krish Sadhukhan]

On 11/06/2017 09:44 AM, Janakarajan Natarajan wrote:

> The function for CPUID 80000001 ECX is set to 0xc0000001. Set it to
> 0x80000001.
>
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>   arch/x86/kvm/cpuid.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 0bc5c13..b21b1d2 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -43,7 +43,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
>   	[CPUID_8086_0001_EDX] = {0x80860001, 0, CPUID_EDX},
>   	[CPUID_1_ECX]         = {         1, 0, CPUID_ECX},
>   	[CPUID_C000_0001_EDX] = {0xc0000001, 0, CPUID_EDX},
> -	[CPUID_8000_0001_ECX] = {0xc0000001, 0, CPUID_ECX},
> +	[CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
>   	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
>   	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
>   	[CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Re: [PATCH v2 1/4] x86/kvm/cpuid: Fix CPUID function for word 6 (80000001_ECX)

[Janakarajan Natarajan]

I forgot to add Boris's Reviewed-by Tag. If the patchset is acceptable,
please let me know if I should send another version with the Tag or if
the Tag can be added when it is merged.

On 11/06/2017 11:44 AM, Janakarajan Natarajan wrote:
> The function for CPUID 80000001 ECX is set to 0xc0000001. Set it to
> 0x80000001.
>
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>   arch/x86/kvm/cpuid.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 0bc5c13..b21b1d2 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -43,7 +43,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
>   	[CPUID_8086_0001_EDX] = {0x80860001, 0, CPUID_EDX},
>   	[CPUID_1_ECX]         = {         1, 0, CPUID_ECX},
>   	[CPUID_C000_0001_EDX] = {0xc0000001, 0, CPUID_EDX},
> -	[CPUID_8000_0001_ECX] = {0xc0000001, 0, CPUID_ECX},
> +	[CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
>   	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
>   	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
>   	[CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},

Re: [PATCH v2 2/4] Add AMD Core Perf Extension MSRs

[Borislav Petkov]

On Mon, Nov 06, 2017 at 11:44:24AM -0600, Janakarajan Natarajan wrote:
> Add the EventSelect and Counter MSRs for AMD Core Perf Extension.
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  arch/x86/include/asm/msr-index.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 17f5c12..1dcbd29 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -337,7 +337,21 @@
>  
>  /* Fam 15h MSRs */
>  #define MSR_F15H_PERF_CTL		0xc0010200
> +#define MSR_F15H_PERF_CTL0		MSR_F15H_PERF_CTL
> +#define MSR_F15H_PERF_CTL1		(MSR_F15H_PERF_CTL + 2)
> +#define MSR_F15H_PERF_CTL2		(MSR_F15H_PERF_CTL + 4)
> +#define MSR_F15H_PERF_CTL3		(MSR_F15H_PERF_CTL + 6)
> +#define MSR_F15H_PERF_CTL4		(MSR_F15H_PERF_CTL + 8)
> +#define MSR_F15H_PERF_CTL5		(MSR_F15H_PERF_CTL + 10)
> +
>  #define MSR_F15H_PERF_CTR		0xc0010201
> +#define MSR_F15H_PERF_CTR0		MSR_F15H_PERF_CTR
> +#define MSR_F15H_PERF_CTR1		(MSR_F15H_PERF_CTR + 2)
> +#define MSR_F15H_PERF_CTR2		(MSR_F15H_PERF_CTR + 4)
> +#define MSR_F15H_PERF_CTR3		(MSR_F15H_PERF_CTR + 6)
> +#define MSR_F15H_PERF_CTR4		(MSR_F15H_PERF_CTR + 8)
> +#define MSR_F15H_PERF_CTR5		(MSR_F15H_PERF_CTR + 10)
> +
>  #define MSR_F15H_NB_PERF_CTL		0xc0010240
>  #define MSR_F15H_NB_PERF_CTR		0xc0010241
>  #define MSR_F15H_PTSC			0xc0010280
> -- 

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Borislav Petkov]

> Subject: Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

Btw, your subjects need a prefix:

"x86/kvm: Add guest support for the AMD core performance counters"

for example.

On Mon, Nov 06, 2017 at 11:44:25AM -0600, Janakarajan Natarajan wrote:
> This patch adds support for AMD Core Performance counters in the guest.

Never say "This patch" in the commit message of a patch. It is
tautologically useless.

> The base event select and counter MSRs are changed. In addition, with
> the core extension, there are 2 extra counters available for performance
> measurements for a total of 6.
> 
> With the new MSRs, the logic to map them to the gp_counters[] is changed.
> New functions are introduced to get the right base MSRs and to check the
> validity of the get/set MSRs.
> 
> If the guest has vcpus of either family 16h or a generation < 15h, it

You're only talking about families here, the "generation" thing is
confusing.

> falls back to using K7 MSRs and the number of counters the guest can
> access is set to 4.
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  arch/x86/kvm/pmu_amd.c | 133 +++++++++++++++++++++++++++++++++++++++++++------
>  arch/x86/kvm/x86.c     |   1 +
>  2 files changed, 120 insertions(+), 14 deletions(-)

...

> +static inline struct kvm_pmc *get_gp_pmc_amd(struct kvm_pmu *pmu, u32 msr,
> +					     enum pmu_type type)
> +{
> +	unsigned int base = get_msr_base(pmu, type);
> +
> +	if (base == MSR_F15H_PERF_CTL) {
> +		switch (msr) {
> +		case MSR_F15H_PERF_CTL0:
> +		case MSR_F15H_PERF_CTL1:
> +		case MSR_F15H_PERF_CTL2:
> +		case MSR_F15H_PERF_CTL3:
> +		case MSR_F15H_PERF_CTL4:
> +		case MSR_F15H_PERF_CTL5:
> +			/*
> +			 * AMD Perf Extension MSRs are not continuous.
> +			 *
> +			 * E.g. MSR_F15H_PERF_CTR0 -> 0xc0010201
> +			 *	MSR_F15H_PERF_CTR1 -> 0xc0010203
> +			 *
> +			 * These are mapped to work with gp_counters[].
> +			 * The index into the array is calculated by
> +			 * dividing the difference between the requested
> +			 * msr and the msr base by 2.
> +			 *
> +			 * E.g. MSR_F15H_PERF_CTR1 uses
> +			 *	->gp_counters[(0xc0010203-0xc0010201)/2]
> +			 *	->gp_counters[1]
> +			 */
> +			return &pmu->gp_counters[(msr - base) >> 1];

Ok, it took me a bit of staring to understand what you're doing here.
And frankly, this scheme is silly and fragile. You're relying on the
fact that you can do math with the MSR numbers to get you the GP counter
number. The moment that changes in future families, you are going to
have to devise a new scheme for the new family.

And instead of doing that, you're much better off producing a simple
MSR -> counter mapping for each family which is a simple switch-case.
No need to do get_msr_base() and whatnot - you simply feed in the MSR
number and the function spits out a gp_counters index. You only need to
check the family of the vcpu.

Then lookers will be able to understand the code at a quick glance too.

...

> @@ -153,8 +251,15 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> +	int family, nr_counters;
> +
> +	family = guest_cpuid_family(vcpu);
> +	if (family == 0x15 || family == 0x17)
> +		nr_counters = AMD64_NUM_COUNTERS_CORE;
> +	else
> +		nr_counters = AMD64_NUM_COUNTERS;
>  
> -	pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
> +	pmu->nr_arch_gp_counters = nr_counters;
>  	pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << 48) - 1;
>  	pmu->reserved_bits = 0xffffffff00200000ull;
>  	/* not applicable to AMD; but clean them to prevent any fall out */
> @@ -169,7 +274,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>  	int i;
>  
> -	for (i = 0; i < AMD64_NUM_COUNTERS ; i++) {
> +	for (i = 0; i < AMD64_NUM_COUNTERS_CORE ; i++) {

This all works because INTEL_PMC_MAX_GENERIC is bigger than the AMD num
counters but you need to check all that.

Also, the finding out of the nr_counters you do in amd_pmu_refresh()
should happen here, in the init function so that you have
pmu->nr_arch_gp_counters properly set and then when you iterate over
counters in the remaining functions, you do:

	for (i = 0; i < pmu->nr_arch_gp_counters ; i++) {

instead of using those defines which are not always correct, depending
on the family.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2 1/4] x86/kvm/cpuid: Fix CPUID function for word 6 (80000001_ECX)

[Radim Krcmar]

2017-11-06 14:38-0600, Janakarajan Natarajan:
> I forgot to add Boris's Reviewed-by Tag. If the patchset is acceptable,
> please let me know if I should send another version with the Tag or if
> the Tag can be added when it is merged.

No problem, I have added that while applying, thanks for mentioning it.
I have applied only the first patch for now as there were comment for
the latter patches.

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Natarajan, Janakarajan]

On 11/9/2017 12:34 PM, Borislav Petkov wrote:
>> Subject: Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest
> Btw, your subjects need a prefix:
>
> "x86/kvm: Add guest support for the AMD core performance counters"
>
> for example.

Okay.

> On Mon, Nov 06, 2017 at 11:44:25AM -0600, Janakarajan Natarajan wrote:
>> This patch adds support for AMD Core Performance counters in the guest.
> Never say "This patch" in the commit message of a patch. It is
> tautologically useless.

Okay.

>> The base event select and counter MSRs are changed. In addition, with
>> the core extension, there are 2 extra counters available for performance
>> measurements for a total of 6.
>>
>> With the new MSRs, the logic to map them to the gp_counters[] is changed.
>> New functions are introduced to get the right base MSRs and to check the
>> validity of the get/set MSRs.
>>
>> If the guest has vcpus of either family 16h or a generation < 15h, it
> You're only talking about families here, the "generation" thing is
> confusing.

I'll change that.

>> falls back to using K7 MSRs and the number of counters the guest can
>> access is set to 4.
>>
>> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
>> ---
>>   arch/x86/kvm/pmu_amd.c | 133 +++++++++++++++++++++++++++++++++++++++++++------
>>   arch/x86/kvm/x86.c     |   1 +
>>   2 files changed, 120 insertions(+), 14 deletions(-)
> ...
>
>> +static inline struct kvm_pmc *get_gp_pmc_amd(struct kvm_pmu *pmu, u32 msr,
>> +					     enum pmu_type type)
>> +{
>> +	unsigned int base = get_msr_base(pmu, type);
>> +
>> +	if (base == MSR_F15H_PERF_CTL) {
>> +		switch (msr) {
>> +		case MSR_F15H_PERF_CTL0:
>> +		case MSR_F15H_PERF_CTL1:
>> +		case MSR_F15H_PERF_CTL2:
>> +		case MSR_F15H_PERF_CTL3:
>> +		case MSR_F15H_PERF_CTL4:
>> +		case MSR_F15H_PERF_CTL5:
>> +			/*
>> +			 * AMD Perf Extension MSRs are not continuous.
>> +			 *
>> +			 * E.g. MSR_F15H_PERF_CTR0 -> 0xc0010201
>> +			 *	MSR_F15H_PERF_CTR1 -> 0xc0010203
>> +			 *
>> +			 * These are mapped to work with gp_counters[].
>> +			 * The index into the array is calculated by
>> +			 * dividing the difference between the requested
>> +			 * msr and the msr base by 2.
>> +			 *
>> +			 * E.g. MSR_F15H_PERF_CTR1 uses
>> +			 *	->gp_counters[(0xc0010203-0xc0010201)/2]
>> +			 *	->gp_counters[1]
>> +			 */
>> +			return &pmu->gp_counters[(msr - base) >> 1];
> Ok, it took me a bit of staring to understand what you're doing here.
> And frankly, this scheme is silly and fragile. You're relying on the
> fact that you can do math with the MSR numbers to get you the GP counter
> number. The moment that changes in future families, you are going to
> have to devise a new scheme for the new family.
>
> And instead of doing that, you're much better off producing a simple
> MSR -> counter mapping for each family which is a simple switch-case.
> No need to do get_msr_base() and whatnot - you simply feed in the MSR
> number and the function spits out a gp_counters index. You only need to
> check the family of the vcpu.
>
> Then lookers will be able to understand the code at a quick glance too.
>
> ...

I'll put out a v3 with those changes.

>> @@ -153,8 +251,15 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
>>   {
>>   	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>> +	int family, nr_counters;
>> +
>> +	family = guest_cpuid_family(vcpu);
>> +	if (family == 0x15 || family == 0x17)
>> +		nr_counters = AMD64_NUM_COUNTERS_CORE;
>> +	else
>> +		nr_counters = AMD64_NUM_COUNTERS;
>>   
>> -	pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
>> +	pmu->nr_arch_gp_counters = nr_counters;
>>   	pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << 48) - 1;
>>   	pmu->reserved_bits = 0xffffffff00200000ull;
>>   	/* not applicable to AMD; but clean them to prevent any fall out */
>> @@ -169,7 +274,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
>>   	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>>   	int i;
>>   
>> -	for (i = 0; i < AMD64_NUM_COUNTERS ; i++) {
>> +	for (i = 0; i < AMD64_NUM_COUNTERS_CORE ; i++) {
> This all works because INTEL_PMC_MAX_GENERIC is bigger than the AMD num
> counters but you need to check all that.
>
> Also, the finding out of the nr_counters you do in amd_pmu_refresh()
> should happen here, in the init function so that you have
> pmu->nr_arch_gp_counters properly set and then when you iterate over
> counters in the remaining functions, you do:
>
> 	for (i = 0; i < pmu->nr_arch_gp_counters ; i++) {
>
> instead of using those defines which are not always correct, depending
> on the family.

So, when the amd_pmu_init is called, a query to guest_cpuid_family() 
gives a value of -1.
If this is the case where qemu sets the family details later on with a 
kvm ioctl, it would make sense
to just initialize the maximum number of gp_counters that will be used 
and set the nr_arch_gp_counters
based on the family during the amd_pmu_refresh().

Thanks,
Janakarajan
>

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Borislav Petkov]

On Wed, Nov 15, 2017 at 01:04:03PM -0600, Natarajan, Janakarajan wrote:
> So, when the amd_pmu_init is called, a query to guest_cpuid_family() gives a
> value of -1.

And that is because...? And it can be fixed to give the proper guest family I
presume ...?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Natarajan, Janakarajan]

On 11/15/2017 1:07 PM, Borislav Petkov wrote:
> On Wed, Nov 15, 2017 at 01:04:03PM -0600, Natarajan, Janakarajan wrote:
>> So, when the amd_pmu_init is called, a query to guest_cpuid_family() gives a
>> value of -1.
> And that is because...? And it can be fixed to give the proper guest family I
> presume ...?

Qemu initially creates a vcpu with KVM_CREATE_VCPU which in kvm 
initializes the pmu. Later on
the cpuid is set using KVM_SET_CPUID2 which sets all the cpuid entries 
in kvm and the pmu refresh is called.

>

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Borislav Petkov]

On Thu, Nov 16, 2017 at 11:13:47AM -0600, Natarajan, Janakarajan wrote:
> On 11/15/2017 1:07 PM, Borislav Petkov wrote:
> > On Wed, Nov 15, 2017 at 01:04:03PM -0600, Natarajan, Janakarajan wrote:
> > > So, when the amd_pmu_init is called, a query to guest_cpuid_family() gives a
> > > value of -1.
> > And that is because...? And it can be fixed to give the proper guest family I
> > presume ...?
> 
> Qemu initially creates a vcpu with KVM_CREATE_VCPU which in kvm initializes
> the pmu. Later on
> the cpuid is set using KVM_SET_CPUID2 which sets all the cpuid entries in
> kvm and the pmu refresh is called.

That doesn't answer my questions above.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Natarajan, Janakarajan]

On 11/16/2017 11:25 AM, Borislav Petkov wrote:
> On Thu, Nov 16, 2017 at 11:13:47AM -0600, Natarajan, Janakarajan wrote:
>> On 11/15/2017 1:07 PM, Borislav Petkov wrote:
>>> On Wed, Nov 15, 2017 at 01:04:03PM -0600, Natarajan, Janakarajan wrote:
>>>> So, when the amd_pmu_init is called, a query to guest_cpuid_family() gives a
>>>> value of -1.
>>> And that is because...? And it can be fixed to give the proper guest family I
>>> presume ...?
>> Qemu initially creates a vcpu with KVM_CREATE_VCPU which in kvm initializes
>> the pmu. Later on
>> the cpuid is set using KVM_SET_CPUID2 which sets all the cpuid entries in
>> kvm and the pmu refresh is called.
> That doesn't answer my questions above.

Ah my apologies. So when the pmu is initialized the cpuid entries aren't 
available then. We'll have to wait until the pmu
refresh is called to get the family value. So there is no place to fix 
it. The setting of the nr_counters based
on the family will have to be done in the amd_pmu_refresh() and not in 
the amd_pmu_init().

>

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Borislav Petkov]

On Thu, Nov 16, 2017 at 12:00:11PM -0600, Natarajan, Janakarajan wrote:
> Ah my apologies. So when the pmu is initialized the cpuid entries
> aren't available then.

So let's see:

... kvm_arch_vcpu_create() ->
svm_create_vcpu() ->
kvm_vcpu_init() ->
kvm_arch_vcpu_init() ->

<--- HERE

kvm_pmu_init()

But at HERE in kvm_arch_vcpu_init() right before kvm_pmu_init() we do already query
cpuid:

	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);

so it's not like we don't know about cpuid leafs at that point. Which
would mean that the code can be made to set the CPU family earlier,
before kvm_pmu_init() runs so that you have the proper CPU family and
thus have this thing properly designed.

Maybe Paolo and Radim have a better suggestion here...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Natarajan, Janakarajan]

On 11/17/2017 5:44 AM, Borislav Petkov wrote:
> On Thu, Nov 16, 2017 at 12:00:11PM -0600, Natarajan, Janakarajan wrote:
>> Ah my apologies. So when the pmu is initialized the cpuid entries
>> aren't available then.
> So let's see:
>
> ... kvm_arch_vcpu_create() ->
> svm_create_vcpu() ->
> kvm_vcpu_init() ->
> kvm_arch_vcpu_init() ->
>
> <--- HERE
>
> kvm_pmu_init()
>
> But at HERE in kvm_arch_vcpu_init() right before kvm_pmu_init() we do already query
> cpuid:
>
> 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);

cpuid_query_maxphyaddr first checks for for the 0x80000008 cpuid entry, 
and if it can't find it
returns a default value of 36.

When invoked in the kvm_arch_vcpu_init(), the default is being returned. 
The vcpu->arch.maxphyaddr
is later updated in the kvm_update_cpuid(), which in my case was setting 
the maxphyaddr to 40.
I believe the cpuid entry is not available when invoked in the init call 
and a default value is
being used as a placeholder until the entries are updated by qemu.

> so it's not like we don't know about cpuid leafs at that point. Which
> would mean that the code can be made to set the CPU family earlier,
> before kvm_pmu_init() runs so that you have the proper CPU family and
> thus have this thing properly designed.
>
> Maybe Paolo and Radim have a better suggestion here...
>

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Natarajan, Janakarajan]

On 11/17/2017 5:44 AM, Borislav Petkov wrote:
> On Thu, Nov 16, 2017 at 12:00:11PM -0600, Natarajan, Janakarajan wrote:
>> Ah my apologies. So when the pmu is initialized the cpuid entries
>> aren't available then.
> So let's see:
>
> ... kvm_arch_vcpu_create() ->
> svm_create_vcpu() ->
> kvm_vcpu_init() ->
> kvm_arch_vcpu_init() ->
>
> <--- HERE
>
> kvm_pmu_init()
>
> But at HERE in kvm_arch_vcpu_init() right before kvm_pmu_init() we do already query
> cpuid:
>
> 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>
> so it's not like we don't know about cpuid leafs at that point. Which
> would mean that the code can be made to set the CPU family earlier,
> before kvm_pmu_init() runs so that you have the proper CPU family and
> thus have this thing properly designed.
>
> Maybe Paolo and Radim have a better suggestion here...

Paolo, Radim any suggestions about this. I feel that the number of 
counters initialized can be 6
and the subsequent code (kvm_pmu_refresh()) takes care of the number of 
counters to be used (4 or 6)
based on the vcpu family.

>

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Radim Krcmar]

2017-12-01 13:30-0600, Natarajan, Janakarajan:
> On 11/17/2017 5:44 AM, Borislav Petkov wrote:
> > On Thu, Nov 16, 2017 at 12:00:11PM -0600, Natarajan, Janakarajan wrote:
> > > Ah my apologies. So when the pmu is initialized the cpuid entries
> > > aren't available then.
> > So let's see:
> > 
> > ... kvm_arch_vcpu_create() ->
> > svm_create_vcpu() ->
> > kvm_vcpu_init() ->
> > kvm_arch_vcpu_init() ->
> > 
> > <--- HERE
> > 
> > kvm_pmu_init()
> > 
> > But at HERE in kvm_arch_vcpu_init() right before kvm_pmu_init() we do already query
> > cpuid:
> > 
> > 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> > 
> > so it's not like we don't know about cpuid leafs at that point. Which
> > would mean that the code can be made to set the CPU family earlier,
> > before kvm_pmu_init() runs so that you have the proper CPU family and
> > thus have this thing properly designed.
> > 
> > Maybe Paolo and Radim have a better suggestion here...
> 
> Paolo, Radim any suggestions about this. I feel that the number of counters
> initialized can be 6

This is the best solution with the current framework.

> and the subsequent code (kvm_pmu_refresh()) takes care of the number of
> counters to be used (4 or 6)
> based on the vcpu family.

Can't we look only at X86_FEATURE_PERFCTR_CORE and completely ignore the
AMD family?
Using the family would bring problems with compatiblity.

Thanks.

Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest

[Natarajan, Janakarajan]

On 12/5/2017 11:56 AM, Radim Krcmar wrote:
> 2017-12-01 13:30-0600, Natarajan, Janakarajan:
>> On 11/17/2017 5:44 AM, Borislav Petkov wrote:
>>> On Thu, Nov 16, 2017 at 12:00:11PM -0600, Natarajan, Janakarajan wrote:
>>>> Ah my apologies. So when the pmu is initialized the cpuid entries
>>>> aren't available then.
>>> So let's see:
>>>
>>> ... kvm_arch_vcpu_create() ->
>>> svm_create_vcpu() ->
>>> kvm_vcpu_init() ->
>>> kvm_arch_vcpu_init() ->
>>>
>>> <--- HERE
>>>
>>> kvm_pmu_init()
>>>
>>> But at HERE in kvm_arch_vcpu_init() right before kvm_pmu_init() we do already query
>>> cpuid:
>>>
>>> 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>>>
>>> so it's not like we don't know about cpuid leafs at that point. Which
>>> would mean that the code can be made to set the CPU family earlier,
>>> before kvm_pmu_init() runs so that you have the proper CPU family and
>>> thus have this thing properly designed.
>>>
>>> Maybe Paolo and Radim have a better suggestion here...
>> Paolo, Radim any suggestions about this. I feel that the number of counters
>> initialized can be 6
> This is the best solution with the current framework.

Okay.

>
>> and the subsequent code (kvm_pmu_refresh()) takes care of the number of
>> counters to be used (4 or 6)
>> based on the vcpu family.
> Can't we look only at X86_FEATURE_PERFCTR_CORE and completely ignore the
> AMD family?
> Using the family would bring problems with compatiblity.

Yeah. We can just use the X86_FEATURE_PERFCTR_CORE flag as a check. I'll 
send a v3
with the changes.

Thanks.

>
> Thanks.