Skip to contentSkip to conversation

Enable full width counting for KVM: x86/pmu

PATCH RESEND In review Like Xu 7 participants 16 messages May 29, 2020 – Mar 11, 2022

Subsystems (3)
x86 KVM KVM x86

v1 of 4 In review Latest is v4 →

lore
Share & tools
lore

Like Xu

author
#0 lore raw

PATCH RESEND 1/1 Enable full width counting for KVM: x86/pmu

Hi Paolo,

As you said, you will queue the v3 of KVM patch, but it looks like we are missing that part at the top of the kvm/queue tree.

For your convenience, let me resend v4 so that we can upstream this feature in the next merged window. Also this patch series includes patches for qemu and kvm-unit-tests. Please help review.

Previous:
https://lore.kernel.org/kvm/[email protected]/

Like Xu (1):
KVM: x86/pmu: Support full width counting [kvm-unit-tests] x86: pmu: Test full-width counter writes [Qemu-devel] target/i386: define a new MSR based feature
word - FEAT_PERF_CAPABILITIES

Wei Wang (1):
KVM: x86/pmu: Tweak kvm_pmu_get_msr to pass 'struct msr_data' in

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            |  2 +-
 arch/x86/kvm/pmu.c              |  4 +-
 arch/x86/kvm/pmu.h              |  4 +-
 arch/x86/kvm/svm/pmu.c          |  7 ++--
 arch/x86/kvm/vmx/capabilities.h | 11 +++++
 arch/x86/kvm/vmx/pmu_intel.c    | 71 +++++++++++++++++++++++++++------
 arch/x86/kvm/vmx/vmx.c          |  3 ++
 arch/x86/kvm/x86.c              |  6 ++-
 9 files changed, 87 insertions(+), 22 deletions(-)
Signature · 1 line
2.21.3
author
#1 lore raw

PATCH RESEND v4 1/2 KVM: x86/pmu: Tweak kvm_pmu_get_msr to pass 'struct msr_data' in

From: Wei Wang <[email protected]>

Change kvm_pmu_get_msr() to get the msr_data struct, as the host_initiated field from the struct could be used by get_msr. This also makes this API consistent with kvm_pmu_set_msr. No functional changes.

Signed-off-by Wei Wang <[email protected]>
---
 arch/x86/kvm/pmu.c           |  4 ++--
 arch/x86/kvm/pmu.h           |  4 ++--
 arch/x86/kvm/svm/pmu.c       |  7 ++++---
 arch/x86/kvm/vmx/pmu_intel.c | 19 +++++++++++--------
 arch/x86/kvm/x86.c           |  4 ++--
 5 files changed, 21 insertions(+), 17 deletions(-)
arch/x86/kvm/pmu.c +2 -2
index a5078841bdac..b86346903f2e 100644
@@ -397,9 +397,9 @@ static void kvm_pmu_mark_pmc_in_use(struct kvm_vcpu *vcpu, u32 msr)
 		__set_bit(pmc->idx, pmu->pmc_in_use);
 }
 
-int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
+int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
-	return kvm_x86_ops.pmu_ops->get_msr(vcpu, msr, data);
+	return kvm_x86_ops.pmu_ops->get_msr(vcpu, msr_info);
 }
 
 int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
arch/x86/kvm/pmu.h +2 -2
index a6c78a797cb1..ab85eed8a6cc 100644
@@ -32,7 +32,7 @@ struct kvm_pmu_ops {
 	struct kvm_pmc *(*msr_idx_to_pmc)(struct kvm_vcpu *vcpu, u32 msr);
 	int (*is_valid_rdpmc_ecx)(struct kvm_vcpu *vcpu, unsigned int idx);
 	bool (*is_valid_msr)(struct kvm_vcpu *vcpu, u32 msr);
-	int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
+	int (*get_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 	int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 	void (*refresh)(struct kvm_vcpu *vcpu);
 	void (*init)(struct kvm_vcpu *vcpu);

@@ -147,7 +147,7 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu);
 int kvm_pmu_rdpmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
 int kvm_pmu_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx);
 bool kvm_pmu_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr);
-int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
+int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 void kvm_pmu_refresh(struct kvm_vcpu *vcpu);
 void kvm_pmu_reset(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm/pmu.c +4 -3
index ce0b10fe5e2b..035da07500e8 100644
@@ -215,21 +215,22 @@ static struct kvm_pmc *amd_msr_idx_to_pmc(struct kvm_vcpu *vcpu, u32 msr)
 	return pmc;
 }
 
-static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
+static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	struct kvm_pmc *pmc;
+	u32 msr = msr_info->index;
 
 	/* MSR_PERFCTRn */
 	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_COUNTER);
 	if (pmc) {
-		*data = pmc_read_counter(pmc);
+		msr_info->data = pmc_read_counter(pmc);
 		return 0;
 	}
 	/* MSR_EVNTSELn */
 	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_EVNTSEL);
 	if (pmc) {
-		*data = pmc->eventsel;
+		msr_info->data = pmc->eventsel;
 		return 0;
 	}
arch/x86/kvm/vmx/pmu_intel.c +11 -8
index 7c857737b438..e1a303fefc16 100644
@@ -184,35 +184,38 @@ static struct kvm_pmc *intel_msr_idx_to_pmc(struct kvm_vcpu *vcpu, u32 msr)
 	return pmc;
 }
 
-static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
+static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	struct kvm_pmc *pmc;
+	u32 msr = msr_info->index;
 
 	switch (msr) {
 	case MSR_CORE_PERF_FIXED_CTR_CTRL:
-		*data = pmu->fixed_ctr_ctrl;
+		msr_info->data = pmu->fixed_ctr_ctrl;
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_STATUS:
-		*data = pmu->global_status;
+		msr_info->data = pmu->global_status;
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
-		*data = pmu->global_ctrl;
+		msr_info->data = pmu->global_ctrl;
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-		*data = pmu->global_ovf_ctrl;
+		msr_info->data = pmu->global_ovf_ctrl;
 		return 0;
 	default:
 		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0))) {
 			u64 val = pmc_read_counter(pmc);
-			*data = val & pmu->counter_bitmask[KVM_PMC_GP];
+			msr_info->data =
+				val & pmu->counter_bitmask[KVM_PMC_GP];
 			return 0;
 		} else if ((pmc = get_fixed_pmc(pmu, msr))) {
 			u64 val = pmc_read_counter(pmc);
-			*data = val & pmu->counter_bitmask[KVM_PMC_FIXED];
+			msr_info->data =
+				val & pmu->counter_bitmask[KVM_PMC_FIXED];
 			return 0;
 		} else if ((pmc = get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0))) {
-			*data = pmc->eventsel;
+			msr_info->data = pmc->eventsel;
 			return 0;
 		}
 	}
arch/x86/kvm/x86.c +2 -2
index d4aa7dc662d5..9f77776f1479 100644
@@ -3113,7 +3113,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
 	case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL1:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
-			return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
+			return kvm_pmu_get_msr(vcpu, msr_info);
 		msr_info->data = 0;
 		break;
 	case MSR_IA32_UCODE_REV:

@@ -3275,7 +3275,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	default:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
-			return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
+			return kvm_pmu_get_msr(vcpu, msr_info);
 		if (!ignore_msrs) {
 			vcpu_debug_ratelimited(vcpu, "unhandled rdmsr: 0x%x\n",
 					       msr_info->index);
Signature · 1 line
2.21.3
author
#2 lore raw

PATCH RESEND v4 2/2 KVM: x86/pmu: Support full width counting

In reply to Like Xu

Intel CPUs have a new alternative MSR range (starting from MSR_IA32_PMC0) for GP counters that allows writing the full counter width. Enable this range from a new capability bit (IA32_PERF_CAPABILITIES.FW_WRITE[bit 13]).

The guest would query CPUID to get the counter width, and sign extends the counter values as needed. The traditional MSRs always limit to 32bit, even though the counter internally is larger (48 or 57 bits).

When the new capability is set, use the alternative range which do not have these restrictions. This lowers the overhead of perf stat slightly because it has to do less interrupts to accumulate the counter value.

Signed-off-by Like Xu <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            |  2 +-
 arch/x86/kvm/vmx/capabilities.h | 11 +++++++
 arch/x86/kvm/vmx/pmu_intel.c    | 52 ++++++++++++++++++++++++++++++---
 arch/x86/kvm/vmx/vmx.c          |  3 ++
 arch/x86/kvm/x86.c              |  2 ++
 6 files changed, 66 insertions(+), 5 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 3485f8454088..452e1bfea4c1 100644
@@ -600,6 +600,7 @@ struct kvm_vcpu_arch {
 	u64 ia32_xss;
 	u64 microcode_version;
 	u64 arch_capabilities;
+	u64 perf_capabilities;
 
 	/*
 	 * Paging state of the vcpu
arch/x86/kvm/cpuid.c +1 -1
index cd708b0b460a..9d781308b872 100644
@@ -296,7 +296,7 @@ void kvm_set_cpu_caps(void)
 		F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
 		0 /* DS-CPL, VMX, SMX, EST */ |
 		0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
-		F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ |
+		F(FMA) | F(CX16) | 0 /* xTPR Update */ | F(PDCM) |
 		F(PCID) | 0 /* Reserved, DCA */ | F(XMM4_1) |
 		F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
 		0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE */ | F(AVX) |
arch/x86/kvm/vmx/capabilities.h +11 -0
index 8903475f751e..4bbd8b448d22 100644
@@ -18,6 +18,8 @@ extern int __read_mostly pt_mode;
 #define PT_MODE_SYSTEM		0
 #define PT_MODE_HOST_GUEST	1
 
+#define PMU_CAP_FW_WRITES	(1ULL << 13)
+
 struct nested_vmx_msrs {
 	/*
 	 * We only store the "true" versions of the VMX capability MSRs. We

@@ -367,4 +369,13 @@ static inline bool vmx_pt_mode_is_host_guest(void)
 	return pt_mode == PT_MODE_HOST_GUEST;
 }
 
+static inline u64 vmx_get_perf_capabilities(void)
+{
+	/*
+	 * Since counters are virtualized, KVM would support full
+	 * width counting unconditionally, even if the host lacks it.
+	 */
+	return PMU_CAP_FW_WRITES;
+}
+
 #endif /* __KVM_X86_VMX_CAPS_H */
arch/x86/kvm/vmx/pmu_intel.c +48 -4

99 lines not on this page · raw message

arch/x86/kvm/vmx/vmx.c +3 -0
index 7b55dc6230a9..d5b7c3b55ae4 100644
@@ -1788,6 +1788,9 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr)
 		if (!nested)
 			return 1;
 		return vmx_get_vmx_msr(&vmcs_config.nested, msr->index, &msr->data);
+	case MSR_IA32_PERF_CAPABILITIES:
+		msr->data = vmx_get_perf_capabilities();
+		return 0;
 	default:
 		return 1;
 	}
arch/x86/kvm/x86.c +2 -0
index 9f77776f1479..791d5955d1b8 100644
@@ -1253,6 +1253,7 @@ static const u32 emulated_msrs_all[] = {
 	MSR_IA32_TSC_ADJUST,
 	MSR_IA32_TSCDEADLINE,
 	MSR_IA32_ARCH_CAPABILITIES,
+	MSR_IA32_PERF_CAPABILITIES,
 	MSR_IA32_MISC_ENABLE,
 	MSR_IA32_MCG_STATUS,
 	MSR_IA32_MCG_CTL,

@@ -1319,6 +1320,7 @@ static const u32 msr_based_features_all[] = {
 	MSR_F10H_DECFG,
 	MSR_IA32_UCODE_REV,
 	MSR_IA32_ARCH_CAPABILITIES,
+	MSR_IA32_PERF_CAPABILITIES,
 };
 
 static u32 msr_based_features[ARRAY_SIZE(msr_based_features_all)];
Signature · 1 line
2.21.3
author
#3 lore raw

PATCH 1/1 x86: pmu: Test full-width counter writes support

In reply to Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>
---
 lib/x86/msr.h |   1 +
 x86/pmu.c     | 125 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 102 insertions(+), 24 deletions(-)
lib/x86/msr.h +1 -0
index 8dca964..6ef5502 100644
@@ -35,6 +35,7 @@
 #define MSR_IA32_SPEC_CTRL              0x00000048
 #define MSR_IA32_PRED_CMD               0x00000049
 
+#define MSR_IA32_PMC0                  0x000004c1
 #define MSR_IA32_PERFCTR0		0x000000c1
 #define MSR_IA32_PERFCTR1		0x000000c2
 #define MSR_FSB_FREQ			0x000000cd
x86/pmu.c +101 -24

221 lines not on this page · raw message

Signature · 1 line
2.21.3
#4 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

Quoted 8 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>
---
 lib/x86/msr.h |   1 +
 x86/pmu.c     | 125 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 102 insertions(+), 24 deletions(-)

[...]

Quoted 14 lines
x86/pmu.c +10 -0
@@ -452,6 +468,66 @@ static void check_running_counter_wrmsr(void)
 	report_prefix_pop();
 }
 
+static void check_counters(void)
+{
+	check_gp_counters();
+	check_fixed_counters();
+	check_rdpmc();
+	check_counters_many();
+	check_counter_overflow();
+	check_gp_counter_cmask();
+	check_running_counter_wrmsr();
+}

x86/pmu.c static void check_running_counter_wrmsr(void) · line 490

Quoted 10 lines
x86/pmu.c +10 -0
+
+static void do_unsupported_width_counter_write(void *index)
+{
+	wrmsr(MSR_IA32_PMC0 + *((int *) index), 0xffffff0123456789ull);
+}
+
+static void  check_gp_counters_write_width(void)
+{
+	u64 val_64 = 0xffffff0123456789ull;
+	u64 val_32 = val_64 & ((1ul << 32) - 1);

Hi,

this broke compilation on 32-bit hosts:

https://travis-ci.com/github/huth/kvm-unit-tests/jobs/349654654#L710

Fix should be easy, I guess - either use 1ull or specify the mask 0xffffffff directly.

Thomas

maintainer
#5 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

Quoted 36 lines · Thomas Huth
Quoted 8 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>
---
 lib/x86/msr.h |   1 +
 x86/pmu.c     | 125 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 102 insertions(+), 24 deletions(-)

[...]

Quoted 14 lines
x86/pmu.c +10 -0
@@ -452,6 +468,66 @@ static void check_running_counter_wrmsr(void)
 	report_prefix_pop();
 }
 
+static void check_counters(void)
+{
+	check_gp_counters();
+	check_fixed_counters();
+	check_rdpmc();
+	check_counters_many();
+	check_counter_overflow();
+	check_gp_counter_cmask();
+	check_running_counter_wrmsr();
+}

x86/pmu.c static void check_running_counter_wrmsr(void) · line 490

Quoted 10 lines
x86/pmu.c +10 -0
+
+static void do_unsupported_width_counter_write(void *index)
+{
+	wrmsr(MSR_IA32_PMC0 + *((int *) index), 0xffffff0123456789ull);
+}
+
+static void  check_gp_counters_write_width(void)
+{
+	u64 val_64 = 0xffffff0123456789ull;
+	u64 val_32 = val_64 & ((1ul << 32) - 1);

Hi,

Quoted 6 lines

this broke compilation on 32-bit hosts:

https://travis-ci.com/github/huth/kvm-unit-tests/jobs/349654654#L710

Fix should be easy, I guess - either use 1ull or specify the mask 0xffffffff directly.

Or

u64 val_32 = (u64)(u32) val_64;

I'll send a patch.

Paolo

#6 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

Quoted 43 lines · Paolo Bonzini
Quoted 40 lines · Thomas Huth
Quoted 8 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>

---
lib/x86/msr.h | 1 +
x86/pmu.c | 125 ++++++++++++++++++++++++++++++++++++++++----------

2 files changed, 102 insertions(+), 24 deletions(-)

[...]

Quoted 14 lines
x86/pmu.c +10 -0
@@ -452,6 +468,66 @@ static void check_running_counter_wrmsr(void)
 	report_prefix_pop();
 }
 
+static void check_counters(void)
+{
+	check_gp_counters();
+	check_fixed_counters();
+	check_rdpmc();
+	check_counters_many();
+	check_counter_overflow();
+	check_gp_counter_cmask();
+	check_running_counter_wrmsr();
+}

x86/pmu.c static void check_running_counter_wrmsr(void) · line 490

Quoted 10 lines
x86/pmu.c +10 -0
+
+static void do_unsupported_width_counter_write(void *index)
+{
+	wrmsr(MSR_IA32_PMC0 + *((int *) index), 0xffffff0123456789ull);
+}
+
+static void  check_gp_counters_write_width(void)
+{
+	u64 val_64 = 0xffffff0123456789ull;
+	u64 val_32 = val_64 & ((1ul << 32) - 1);

Hi,

this broke compilation on 32-bit hosts:

https://travis-ci.com/github/huth/kvm-unit-tests/jobs/349654654#L710

Quoted 8 lines
Quoted 2 lines

Fix should be easy, I guess - either use 1ull or specify the mask 0xffffffff directly.

Or

u64 val_32 = (u64)(u32) val_64;

I'll send a patch.

I missed this correspondence, but while running the tests on 32-bit I encountered two additional cases of wrong masks that caused failures.

I have sent a separate patch for your consideration.

#7 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

In reply to Like Xu

x86/pmu.c static void check_running_counter_wrmsr(void) · line 515

Quoted 11 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>

---

x86/pmu.c +4 -0
+       /*
+        * MSR_IA32_PMCn supports writing values ​​up to GP counter width,
+        * and only the lowest bits of GP counter width are valid.
+        */

Could you rewrite this comment in ASCII, please? I would do it, but I'm not sure what the correct translation is.

author
#8 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

Quoted 8 lines · Jim Mattson
Quoted 6 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>

---

Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       /*
+        * MSR_IA32_PMCn supports writing values ​​up to GP counter width,
+        * and only the lowest bits of GP counter width are valid.
+        */

Could you rewrite this comment in ASCII, please? I would do it, but I'm not sure what the correct translation is.

My first submitted patch says that
they are just Unicode "ZERO WIDTH SPACE".

https://lore.kernel.org/kvm/[email protected]/

Here you go:

---

From 1b058846aabcd7a85b5c5f41cb2b63b6a348bdc4 Mon Sep 17 00:00:00 2001
From: Like Xu <[email protected]>
Date: Wed, 12 May 2021 14:26:40 +0800
Subject: [PATCH] x86: pmu: Fix a comment about full-width counter writes
support

Remove two Unicode characters 'ZERO WIDTH SPACE' (U+200B).

Fixes 22f2901a0e ("x86: pmu: Test full-width counter writes support")
Reported-by Jim Mattson <[email protected]>
Signed-off-by Like Xu <[email protected]>
---
 x86/pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
x86/pmu.c +1 -1
index 5a3d55b..6cb3506 100644
@@ -510,7 +510,7 @@ static void  check_gp_counters_write_width(void)
        }
 
        /*
-        * MSR_IA32_PMCn supports writing values ​​up to GP counter width,
+        * MSR_IA32_PMCn supports writing values up to GP counter width,
         * and only the lowest bits of GP counter width are valid.
         */
        for (i = 0; i < num_counters; i++) {

--
2.31.1

#9 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

Quoted 57 lines · Like Xu
Quoted 8 lines · Jim Mattson
Quoted 6 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>

---

Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       /*
+        * MSR_IA32_PMCn supports writing values ​​up to GP counter width,
+        * and only the lowest bits of GP counter width are valid.
+        */

Could you rewrite this comment in ASCII, please? I would do it, but I'm not sure what the correct translation is.

My first submitted patch says that
they are just Unicode "ZERO WIDTH SPACE".

https://lore.kernel.org/kvm/[email protected]/

Here you go:

---

From 1b058846aabcd7a85b5c5f41cb2b63b6a348bdc4 Mon Sep 17 00:00:00 2001
From: Like Xu <[email protected]>
Date: Wed, 12 May 2021 14:26:40 +0800
Subject: [PATCH] x86: pmu: Fix a comment about full-width counter writes
support

Remove two Unicode characters 'ZERO WIDTH SPACE' (U+200B).

Fixes 22f2901a0e ("x86: pmu: Test full-width counter writes support")
Reported-by Jim Mattson <[email protected]>
Signed-off-by Like Xu <[email protected]>
---
  x86/pmu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
x86/pmu.c +1 -1
index 5a3d55b..6cb3506 100644
@@ -510,7 +510,7 @@ static void  check_gp_counters_write_width(void)
         }
 
         /*
-        * MSR_IA32_PMCn supports writing values ​​up to GP
 counter width,
+        * MSR_IA32_PMCn supports writing values up to GP counter width,
          * and only the lowest bits of GP counter width are valid.
          */

         for (i = 0; i < num_counters; i++) {
Quoted 2 lines

--
2.31.1

Paolo:

Did this patch get overlooked? I'm still seeing the unicode characters in this comment.

#10 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

Quoted 63 lines · Jim Mattson
Quoted 57 lines · Like Xu
Quoted 8 lines · Jim Mattson
Quoted 6 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>

---

Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       /*
+        * MSR_IA32_PMCn supports writing values ​​up to GP counter width,
+        * and only the lowest bits of GP counter width are valid.
+        */

Could you rewrite this comment in ASCII, please? I would do it, but I'm not sure what the correct translation is.

My first submitted patch says that
they are just Unicode "ZERO WIDTH SPACE".

https://lore.kernel.org/kvm/[email protected]/

Here you go:

---

From 1b058846aabcd7a85b5c5f41cb2b63b6a348bdc4 Mon Sep 17 00:00:00 2001
From: Like Xu <[email protected]>
Date: Wed, 12 May 2021 14:26:40 +0800
Subject: [PATCH] x86: pmu: Fix a comment about full-width counter writes
support

Remove two Unicode characters 'ZERO WIDTH SPACE' (U+200B).

Fixes 22f2901a0e ("x86: pmu: Test full-width counter writes support")
Reported-by Jim Mattson <[email protected]>
Signed-off-by Like Xu <[email protected]>
---
   x86/pmu.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
x86/pmu.c +1 -1
index 5a3d55b..6cb3506 100644
@@ -510,7 +510,7 @@ static void  check_gp_counters_write_width(void)
          }
 
          /*
-        * MSR_IA32_PMCn supports writing values ​​up to GP
 counter width,
+        * MSR_IA32_PMCn supports writing values up to GP counter width,
           * and only the lowest bits of GP counter width are valid.
           */

          for (i = 0; i < num_counters; i++) {
Quoted 2 lines

--
2.31.1

Paolo:

Quoted 3 lines

Did this patch get overlooked? I'm still seeing the unicode characters in this comment.

Hi Paolo, please help review this one as well:
https://lore.kernel.org/kvm/[email protected]/

#11 lore raw

Re: [kvm-unit-tests PATCH] x86: pmu: Test full-width counter writes support

In reply to Jim Mattson
Quoted 63 lines · Jim Mattson
Quoted 57 lines · Like Xu
Quoted 8 lines · Jim Mattson
Quoted 6 lines · Like Xu

When the full-width writes capability is set, use the alternative MSR range to write larger sign counter values (up to GP counter width).

Signed-off-by Like Xu <[email protected]>

---

Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       /*
+        * MSR_IA32_PMCn supports writing values ​​up to GP counter width,
+        * and only the lowest bits of GP counter width are valid.
+        */

Could you rewrite this comment in ASCII, please? I would do it, but I'm not sure what the correct translation is.

My first submitted patch says that
they are just Unicode "ZERO WIDTH SPACE".

https://lore.kernel.org/kvm/[email protected]/

Here you go:

---

From 1b058846aabcd7a85b5c5f41cb2b63b6a348bdc4 Mon Sep 17 00:00:00 2001
From: Like Xu <[email protected]>
Date: Wed, 12 May 2021 14:26:40 +0800
Subject: [PATCH] x86: pmu: Fix a comment about full-width counter writes
support

Remove two Unicode characters 'ZERO WIDTH SPACE' (U+200B).

Fixes 22f2901a0e ("x86: pmu: Test full-width counter writes support")
Reported-by Jim Mattson <[email protected]>
Signed-off-by Like Xu <[email protected]>
---
   x86/pmu.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
x86/pmu.c +1 -1
index 5a3d55b..6cb3506 100644
@@ -510,7 +510,7 @@ static void  check_gp_counters_write_width(void)
          }
 
          /*
-        * MSR_IA32_PMCn supports writing values ​​up to GP
 counter width,
+        * MSR_IA32_PMCn supports writing values up to GP counter width,
           * and only the lowest bits of GP counter width are valid.
           */

          for (i = 0; i < num_counters; i++) {
Quoted 2 lines

--
2.31.1

Paolo:

Quoted 2 lines

Did this patch get overlooked? I'm still seeing the unicode characters in this comment.

Yes, seems like it felt through the cracks. It's better to send patches as a new mail thread instead of posting them in a reply, otherwise they might be overlooked. Anyway, I've pushed this patch now to the repo.

Thomas

author
#12 lore raw

PATCH 1/1 target/i386: define a new MSR based feature word - FEAT_PERF_CAPABILITIES

In reply to Like Xu

The Perfmon and Debug Capability MSR named IA32_PERF_CAPABILITIES is a feature-enumerating MSR, which only enumerates the feature full-width write (via bit 13) by now which indicates the processor supports IA32_A_PMCx interface for updating bits 32 and above of IA32_PMCx.

The existence of MSR IA32_PERF_CAPABILITIES is enumerated by CPUID.1:ECX[15].

Cc Paolo Bonzini <[email protected]>
Cc Richard Henderson <[email protected]>
Cc Eduardo Habkost <[email protected]>
Cc Marcelo Tosatti <[email protected]>
Cc [email protected]
Signed-off-by Like Xu <[email protected]>
---
 target/i386/cpu.c | 29 +++++++++++++++++++++++++++++
 target/i386/cpu.h |  3 +++
 target/i386/kvm.c | 20 ++++++++++++++++++++
 3 files changed, 52 insertions(+)
target/i386/cpu.c +29 -0
index 3733d9a279..be56966bb0 100644
@@ -1139,6 +1139,22 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             .index = MSR_IA32_CORE_CAPABILITY,
         },
     },
+    [FEAT_PERF_CAPABILITIES] = {
+        .type = MSR_FEATURE_WORD,
+        .feat_names = {
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, "full-width-write", NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+        },
+        .msr = {
+            .index = MSR_IA32_PERF_CAPABILITIES,
+        },
+    },
 
     [FEAT_VMX_PROCBASED_CTLS] = {
         .type = MSR_FEATURE_WORD,

@@ -1316,6 +1332,10 @@ static FeatureDep feature_dependencies[] = {
         .from = { FEAT_7_0_EDX,             CPUID_7_0_EDX_CORE_CAPABILITY },
         .to = { FEAT_CORE_CAPABILITY,       ~0ull },
     },
+    {
+        .from = { FEAT_1_ECX,             CPUID_EXT_PDCM },
+        .to = { FEAT_PERF_CAPABILITIES,       ~0ull },
+    },
     {
         .from = { FEAT_1_ECX,               CPUID_EXT_VMX },
         .to = { FEAT_VMX_PROCBASED_CTLS,    ~0ull },

@@ -5488,6 +5508,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
             *edx |= CPUID_HT;
         }
+        if (!cpu->enable_pmu) {
+            *ecx &= ~CPUID_EXT_PDCM;
+        }
         break;
     case 2:
         /* cache info: needed for Pentium Pro compatibility */

@@ -6505,6 +6528,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    if (kvm_enabled() && cpu->enable_pmu &&
+        (kvm_arch_get_supported_cpuid(kvm_state, 1, 0, R_ECX) &
+         CPUID_EXT_PDCM)) {
+        env->features[FEAT_1_ECX] |= CPUID_EXT_PDCM;
+    }
+
     if (cpu->ucode_rev == 0) {
         /* The default is the same as KVM's.  */
         if (IS_AMD_CPU(env)) {
target/i386/cpu.h +3 -0
index 408392dbf6..fad2f874bd 100644
@@ -356,6 +356,8 @@ typedef enum X86Seg {
 #define MSR_IA32_ARCH_CAPABILITIES      0x10a
 #define ARCH_CAP_TSX_CTRL_MSR		(1<<7)
 
+#define MSR_IA32_PERF_CAPABILITIES      0x345
+
 #define MSR_IA32_TSX_CTRL		0x122
 #define MSR_IA32_TSCDEADLINE            0x6e0
 

@@ -529,6 +531,7 @@ typedef enum FeatureWord {
     FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
     FEAT_ARCH_CAPABILITIES,
     FEAT_CORE_CAPABILITY,
+    FEAT_PERF_CAPABILITIES,
     FEAT_VMX_PROCBASED_CTLS,
     FEAT_VMX_SECONDARY_CTLS,
     FEAT_VMX_PINBASED_CTLS,
target/i386/kvm.c +20 -0
index 34f838728d..9be6f76b2c 100644
@@ -106,6 +106,7 @@ static bool has_msr_core_capabs;
 static bool has_msr_vmx_vmfunc;
 static bool has_msr_ucode_rev;
 static bool has_msr_vmx_procbased_ctls2;
+static bool has_msr_perf_capabs;
 
 static uint32_t has_architectural_pmu_version;
 static uint32_t num_architectural_pmu_gp_counters;

@@ -2027,6 +2028,9 @@ static int kvm_get_supported_msrs(KVMState *s)
             case MSR_IA32_CORE_CAPABILITY:
                 has_msr_core_capabs = true;
                 break;
+            case MSR_IA32_PERF_CAPABILITIES:
+                has_msr_perf_capabs = true;
+                break;
             case MSR_IA32_VMX_VMFUNC:
                 has_msr_vmx_vmfunc = true;
                 break;

@@ -2643,6 +2647,18 @@ static void kvm_msr_entry_add_vmx(X86CPU *cpu, FeatureWordArray f)
                       VMCS12_MAX_FIELD_INDEX << 1);
 }
 
+static void kvm_msr_entry_add_perf(X86CPU *cpu, FeatureWordArray f)
+{
+    uint64_t kvm_perf_cap =
+        kvm_arch_get_supported_msr_feature(kvm_state,
+                                           MSR_IA32_PERF_CAPABILITIES);
+
+    if (kvm_perf_cap) {
+        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES,
+                        kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]);
+    }
+}
+
 static int kvm_buf_set_msrs(X86CPU *cpu)
 {
     int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);

@@ -2675,6 +2691,10 @@ static void kvm_init_msrs(X86CPU *cpu)
                           env->features[FEAT_CORE_CAPABILITY]);
     }
 
+    if (has_msr_perf_capabs && cpu->enable_pmu) {
+        kvm_msr_entry_add_perf(cpu, env->features);
+    }
+
     if (has_msr_ucode_rev) {
         kvm_msr_entry_add(cpu, MSR_IA32_UCODE_REV, cpu->ucode_rev);
     }
Signature · 1 line
2.21.3
maintainer
#13 lore raw

Re: [Qemu-devel PATCH] target/i386: define a new MSR based feature word - FEAT_PERF_CAPABILITIES

Quoted 7 lines · Like Xu
target/i386/cpu.c +3 -0
+        if (!cpu->enable_pmu) {
+            *ecx &= ~CPUID_EXT_PDCM;
+        }
         break;
     case 2:
         /* cache info: needed for Pentium Pro compatibility */

target/i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 6536

Quoted 9 lines
target/i386/cpu.c +6 -0
@@ -6505,6 +6528,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    if (kvm_enabled() && cpu->enable_pmu &&
+        (kvm_arch_get_supported_cpuid(kvm_state, 1, 0, R_ECX) &
+         CPUID_EXT_PDCM)) {
+        env->features[FEAT_1_ECX] |= CPUID_EXT_PDCM;
+    }
+

I'm dropping this hunk two hunks because it's going to break live migration with e.g. "-cpu IvyBridge,pmu=on". We will have to add PDCM by default only to future CPU models, but "-cpu host,pmu=on" will pick it up automatically.

Paolo

maintainer
#14 lore raw
In reply to Like Xu
Quoted 21 lines · Like Xu

Hi Paolo,

As you said, you will queue the v3 of KVM patch, but it looks like we are missing that part at the top of the kvm/queue tree.

For your convenience, let me resend v4 so that we can upstream this feature in the next merged window. Also this patch series includes patches for qemu and kvm-unit-tests. Please help review.

Previous:
https://lore.kernel.org/kvm/[email protected]/

Like Xu (1):
KVM: x86/pmu: Support full width counting [kvm-unit-tests] x86: pmu: Test full-width counter writes [Qemu-devel] target/i386: define a new MSR based feature
word - FEAT_PERF_CAPABILITIES

Wei Wang (1):
KVM: x86/pmu: Tweak kvm_pmu_get_msr to pass 'struct msr_data' in

Quoted 11 lines
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            |  2 +-
 arch/x86/kvm/pmu.c              |  4 +-
 arch/x86/kvm/pmu.h              |  4 +-
 arch/x86/kvm/svm/pmu.c          |  7 ++--
 arch/x86/kvm/vmx/capabilities.h | 11 +++++
 arch/x86/kvm/vmx/pmu_intel.c    | 71 +++++++++++++++++++++++++++------
 arch/x86/kvm/vmx/vmx.c          |  3 ++
 arch/x86/kvm/x86.c              |  6 ++-
 9 files changed, 87 insertions(+), 22 deletions(-)

Thanks, I was busy with AMD stuff as you saw. :) I've queued it now.

Paolo

#15 lore raw
Quoted 33 lines · Paolo Bonzini
Quoted 21 lines · Like Xu

Hi Paolo,

As you said, you will queue the v3 of KVM patch, but it looks like we are missing that part at the top of the kvm/queue tree.

For your convenience, let me resend v4 so that we can upstream this feature in the next merged window. Also this patch series includes patches for qemu and kvm-unit-tests. Please help review.

Previous:
https://lore.kernel.org/kvm/[email protected]/

Like Xu (1):
KVM: x86/pmu: Support full width counting [kvm-unit-tests] x86: pmu: Test full-width counter writes [Qemu-devel] target/i386: define a new MSR based feature
word - FEAT_PERF_CAPABILITIES

Wei Wang (1):
KVM: x86/pmu: Tweak kvm_pmu_get_msr to pass 'struct msr_data' in

Quoted 11 lines
  arch/x86/include/asm/kvm_host.h |  1 +
  arch/x86/kvm/cpuid.c            |  2 +-
  arch/x86/kvm/pmu.c              |  4 +-
  arch/x86/kvm/pmu.h              |  4 +-
  arch/x86/kvm/svm/pmu.c          |  7 ++--
  arch/x86/kvm/vmx/capabilities.h | 11 +++++
  arch/x86/kvm/vmx/pmu_intel.c    | 71 +++++++++++++++++++++++++++------
  arch/x86/kvm/vmx/vmx.c          |  3 ++
  arch/x86/kvm/x86.c              |  6 ++-
  9 files changed, 87 insertions(+), 22 deletions(-)
Quoted 1 line

Thanks, I was busy with AMD stuff as you saw. :) I've queued it now.

Yes, we all know you're busy.

I will be very grateful if you could comment the KVM part for the LBR feature. :D

https://lore.kernel.org/kvm/[email protected]/

Thanks,
Like Xu

Quoted 3 lines

Paolo