[PATCH v3 0/8] KVM: x86: Add AMD Guest PerfMonV2 PMU support

[PATCH v3 0/8] KVM: x86: Add AMD Guest PerfMonV2 PMU support

[Like Xu]

Starting with Zen4, core PMU on AMD platforms such as Genoa and
Ryzen-7000 will support PerfMonV2, and it is also compatible with
legacy PERFCTR_CORE behavior and msr addresses.

If you don't have access to the hardware specification, the commits
d6d0c7f681fd..7685665c390d for host perf can also bring a quick
overview. Its main change is the addition of three msr's equivalent
to Intel V2, namely global_ctrl, global_status, global_status_clear.

It is worth noting that this feature is very attractive for reducing the
overhead of PMU virtualization, since multiple msr accesses to multiple
counters will be replaced by a single access to the global register,
plus more accuracy gain when multiple guest counters are used.

All related testcases are passed on a Genoa box.
Please feel free to run more tests, add more or share comments.

Previous:
https://lore.kernel.org/kvm/20220919093453.71737-1-likexu@tencent.com/

V2 -> V3 Changelog:
- Renme pmc_is_enabled(); (Jim)
- Move the reprogram_counters() changes as a separate patch; (Jim)
- Refactoring to align with other set_msr() helper; (Sean)
- Fix the issue if userspace wants to expose v1 for whatever reason; (Sean)
- Add the feature flag X86_FEATURE_AMD_PMU_V2; (Sean)
- Check enable_pmu for intel 0xa as well; (Sean)
- Limit AMD pmu's KVM support to version 2 as well;
- Other nit changes around C code taste; (Sean)

Like Xu (7):
  KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled()
  KVM: x86/pmu: Rewrite reprogram_counters() to improve performance
  KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
  KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry
  KVM: x86/svm/pmu: Add AMD PerfMonV2 support
  KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
  KVM: x86/cpuid: Use fast return for cpuid "0xa" leaf when !enable_pmu

Sean Christopherson (1):
  KVM: VMX: Refactor intel_pmu_set_msr() to align with other set_msr()
    helpers

 arch/x86/include/asm/kvm-x86-pmu-ops.h |   1 -
 arch/x86/include/asm/kvm_host.h        |   1 +
 arch/x86/kvm/cpuid.c                   |  32 ++++++-
 arch/x86/kvm/pmu.c                     |  65 +++++++++++++--
 arch/x86/kvm/pmu.h                     |  28 ++++++-
 arch/x86/kvm/reverse_cpuid.h           |  10 +++
 arch/x86/kvm/svm/pmu.c                 |  73 +++++++++++-----
 arch/x86/kvm/svm/svm.c                 |  11 ++-
 arch/x86/kvm/vmx/pmu_intel.c           | 111 +++++++------------------
 arch/x86/kvm/x86.c                     |  14 +++-
 10 files changed, 228 insertions(+), 118 deletions(-)

-- 
2.38.1

[PATCH v3 1/8] KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled()

[Like Xu]

From: Like Xu <likexu@tencent.com>

The name of function pmc_is_enabled() is a bit misleading. A PMC can
be disabled either by PERF_CLOBAL_CTRL or by its corresponding EVTSEL.
Add the global semantic to its name.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/include/asm/kvm-x86-pmu-ops.h | 2 +-
 arch/x86/kvm/pmu.c                     | 6 +++---
 arch/x86/kvm/pmu.h                     | 2 +-
 arch/x86/kvm/svm/pmu.c                 | 2 +-
 arch/x86/kvm/vmx/pmu_intel.c           | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-pmu-ops.h b/arch/x86/include/asm/kvm-x86-pmu-ops.h
index c17e3e96fc1d..86a3fb01e103 100644
--- a/arch/x86/include/asm/kvm-x86-pmu-ops.h
+++ b/arch/x86/include/asm/kvm-x86-pmu-ops.h
@@ -13,7 +13,7 @@ BUILD_BUG_ON(1)
  * at the call sites.
  */
 KVM_X86_PMU_OP(hw_event_available)
-KVM_X86_PMU_OP(pmc_is_enabled)
+KVM_X86_PMU_OP(pmc_is_globally_enabled)
 KVM_X86_PMU_OP(pmc_idx_to_pmc)
 KVM_X86_PMU_OP(rdpmc_ecx_to_pmc)
 KVM_X86_PMU_OP(msr_idx_to_pmc)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 684393c22105..e57f707fb940 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -83,7 +83,7 @@ void kvm_pmu_ops_update(const struct kvm_pmu_ops *pmu_ops)
 #undef __KVM_X86_PMU_OP
 }
 
-static inline bool pmc_is_enabled(struct kvm_pmc *pmc)
+static inline bool pmc_is_globally_enabled(struct kvm_pmc *pmc)
 {
 	return static_call(kvm_x86_pmu_pmc_is_enabled)(pmc);
 }
@@ -306,7 +306,7 @@ static void reprogram_counter(struct kvm_pmc *pmc)
 
 	pmc_pause_counter(pmc);
 
-	if (!pmc_speculative_in_use(pmc) || !pmc_is_enabled(pmc))
+	if (!pmc_speculative_in_use(pmc) || !pmc_is_globally_enabled(pmc))
 		goto reprogram_complete;
 
 	if (!check_pmu_event_filter(pmc))
@@ -581,7 +581,7 @@ void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, u64 perf_hw_id)
 	for_each_set_bit(i, pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX) {
 		pmc = static_call(kvm_x86_pmu_pmc_idx_to_pmc)(pmu, i);
 
-		if (!pmc || !pmc_is_enabled(pmc) || !pmc_speculative_in_use(pmc))
+		if (!pmc || !pmc_is_globally_enabled(pmc) || !pmc_speculative_in_use(pmc))
 			continue;
 
 		/* Ignore checks for edge detect, pin control, invert and CMASK bits */
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 85ff3c0588ba..2b5376ba66ea 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -26,7 +26,7 @@ struct kvm_event_hw_type_mapping {
 
 struct kvm_pmu_ops {
 	bool (*hw_event_available)(struct kvm_pmc *pmc);
-	bool (*pmc_is_enabled)(struct kvm_pmc *pmc);
+	bool (*pmc_is_globally_enabled)(struct kvm_pmc *pmc);
 	struct kvm_pmc *(*pmc_idx_to_pmc)(struct kvm_pmu *pmu, int pmc_idx);
 	struct kvm_pmc *(*rdpmc_ecx_to_pmc)(struct kvm_vcpu *vcpu,
 		unsigned int idx, u64 *mask);
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 0e313fbae055..7958a983b760 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -218,7 +218,7 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu)
 
 struct kvm_pmu_ops amd_pmu_ops __initdata = {
 	.hw_event_available = amd_hw_event_available,
-	.pmc_is_enabled = amd_pmc_is_enabled,
+	.pmc_is_globally_enabled = amd_pmc_is_enabled,
 	.pmc_idx_to_pmc = amd_pmc_idx_to_pmc,
 	.rdpmc_ecx_to_pmc = amd_rdpmc_ecx_to_pmc,
 	.msr_idx_to_pmc = amd_msr_idx_to_pmc,
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index e5cec07ca8d9..f81cf54a245f 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -797,7 +797,7 @@ void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu)
 
 struct kvm_pmu_ops intel_pmu_ops __initdata = {
 	.hw_event_available = intel_hw_event_available,
-	.pmc_is_enabled = intel_pmc_is_enabled,
+	.pmc_is_globally_enabled = intel_pmc_is_enabled,
 	.pmc_idx_to_pmc = intel_pmc_idx_to_pmc,
 	.rdpmc_ecx_to_pmc = intel_rdpmc_ecx_to_pmc,
 	.msr_idx_to_pmc = intel_msr_idx_to_pmc,
-- 
2.38.1

Re: [PATCH v3 1/8] KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled()

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> The name of function pmc_is_enabled() is a bit misleading. A PMC can
> be disabled either by PERF_CLOBAL_CTRL or by its corresponding EVTSEL.
> Add the global semantic to its name.
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---

...

> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 684393c22105..e57f707fb940 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -83,7 +83,7 @@ void kvm_pmu_ops_update(const struct kvm_pmu_ops *pmu_ops)
>  #undef __KVM_X86_PMU_OP
>  }
>  
> -static inline bool pmc_is_enabled(struct kvm_pmc *pmc)
> +static inline bool pmc_is_globally_enabled(struct kvm_pmc *pmc)
>  {
>  	return static_call(kvm_x86_pmu_pmc_is_enabled)(pmc);

This doesn't compile.  v3, and I'm getting pings, and the very first patch doesn't
compile.

Re: [PATCH v3 1/8] KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled()

[Like Xu]

On 27/1/2023 10:03 am, Sean Christopherson wrote:
> On Fri, Nov 11, 2022, Like Xu wrote:
>> From: Like Xu<likexu@tencent.com>
>>
>> The name of function pmc_is_enabled() is a bit misleading. A PMC can
>> be disabled either by PERF_CLOBAL_CTRL or by its corresponding EVTSEL.
>> Add the global semantic to its name.
>>
>> Suggested-by: Jim Mattson<jmattson@google.com>
>> Signed-off-by: Like Xu<likexu@tencent.com>
>> ---
> ...
> 
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index 684393c22105..e57f707fb940 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -83,7 +83,7 @@ void kvm_pmu_ops_update(const struct kvm_pmu_ops *pmu_ops)
>>   #undef __KVM_X86_PMU_OP
>>   }
>>   
>> -static inline bool pmc_is_enabled(struct kvm_pmc *pmc)
>> +static inline bool pmc_is_globally_enabled(struct kvm_pmc *pmc)
>>   {
>>   	return static_call(kvm_x86_pmu_pmc_is_enabled)(pmc);
> This doesn't compile.  v3, and I'm getting pings, and the very first patch doesn't
> compile.
> 

Oops, very sorry for this breaking the git-bisect attribute, it's my fault to 
split the code diff incorrectly
(weakly it compiles fine w/ 4th patch), I will enhance the process before 
sending any patches to you.
Thank you for taking time to review the rest of patches in detail as you always 
do. The new version is
under construction, apologies again.

[PATCH v3 2/8] KVM: VMX: Refactor intel_pmu_set_msr() to align with other set_msr() helpers

[Like Xu]

From: Sean Christopherson <seanjc@google.com>

Invert the flows in intel_pmu_set_msr()'s case statements so that they
follow the kernel's preferred style of:

	if (<not valid>)
		return <error>

	<commit change>
	return <success>

which is also the style used by every other set_msr() helper (except
AMD's PMU variant, which doesn't use a switch statement).

Opportunstically move the "val == current" checks below the validity
checks.  Except for the one-off case for MSR_P6_EVNTSEL2, the reserved
bit checks are extremely cheap, and the guest is unlikely to frequently
write the current value, i.e. avoiding the reserved bit checks doesn't
add much (any?) value.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 81 +++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index f81cf54a245f..2f7cd388859c 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -397,44 +397,43 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 	switch (msr) {
 	case MSR_CORE_PERF_FIXED_CTR_CTRL:
-		if (pmu->fixed_ctr_ctrl == data)
-			return 0;
-		if (!(data & pmu->fixed_ctr_ctrl_mask)) {
+		if (data & pmu->fixed_ctr_ctrl_mask)
+			return 1;
+
+		if (pmu->fixed_ctr_ctrl != data)
 			reprogram_fixed_counters(pmu, data);
-			return 0;
-		}
 		break;
 	case MSR_CORE_PERF_GLOBAL_STATUS:
-		if (msr_info->host_initiated) {
-			pmu->global_status = data;
-			return 0;
-		}
-		break; /* RO MSR */
+		if (!msr_info->host_initiated)
+			return 1; /* RO MSR */
+
+		pmu->global_status = data;
+		break;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
-		if (pmu->global_ctrl == data)
-			return 0;
-		if (kvm_valid_perf_global_ctrl(pmu, data)) {
+		if (!kvm_valid_perf_global_ctrl(pmu, data))
+			return 1;
+
+		if (pmu->global_ctrl != data) {
 			diff = pmu->global_ctrl ^ data;
 			pmu->global_ctrl = data;
 			reprogram_counters(pmu, diff);
-			return 0;
 		}
 		break;
 	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-		if (!(data & pmu->global_ovf_ctrl_mask)) {
-			if (!msr_info->host_initiated)
-				pmu->global_status &= ~data;
-			return 0;
-		}
+		if (data & pmu->global_ovf_ctrl_mask)
+			return 1;
+
+		if (!msr_info->host_initiated)
+			pmu->global_status &= ~data;
 		break;
 	case MSR_IA32_PEBS_ENABLE:
-		if (pmu->pebs_enable == data)
-			return 0;
-		if (!(data & pmu->pebs_enable_mask)) {
+		if (data & pmu->pebs_enable_mask)
+			return 1;
+
+		if (pmu->pebs_enable != data) {
 			diff = pmu->pebs_enable ^ data;
 			pmu->pebs_enable = data;
 			reprogram_counters(pmu, diff);
-			return 0;
 		}
 		break;
 	case MSR_IA32_DS_AREA:
@@ -442,15 +441,14 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 		if (is_noncanonical_address(data, vcpu))
 			return 1;
+
 		pmu->ds_area = data;
-		return 0;
+		break;
 	case MSR_PEBS_DATA_CFG:
-		if (pmu->pebs_data_cfg == data)
-			return 0;
-		if (!(data & pmu->pebs_data_cfg_mask)) {
-			pmu->pebs_data_cfg = data;
-			return 0;
-		}
+		if (data & pmu->pebs_data_cfg_mask)
+			return 1;
+
+		pmu->pebs_data_cfg = data;
 		break;
 	default:
 		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
@@ -458,33 +456,38 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			if ((msr & MSR_PMC_FULL_WIDTH_BIT) &&
 			    (data & ~pmu->counter_bitmask[KVM_PMC_GP]))
 				return 1;
+
 			if (!msr_info->host_initiated &&
 			    !(msr & MSR_PMC_FULL_WIDTH_BIT))
 				data = (s64)(s32)data;
 			pmc->counter += data - pmc_read_counter(pmc);
 			pmc_update_sample_period(pmc);
-			return 0;
+			break;
 		} else if ((pmc = get_fixed_pmc(pmu, msr))) {
 			pmc->counter += data - pmc_read_counter(pmc);
 			pmc_update_sample_period(pmc);
-			return 0;
+			break;
 		} else if ((pmc = get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0))) {
-			if (data == pmc->eventsel)
-				return 0;
 			reserved_bits = pmu->reserved_bits;
 			if ((pmc->idx == 2) &&
 			    (pmu->raw_event_mask & HSW_IN_TX_CHECKPOINTED))
 				reserved_bits ^= HSW_IN_TX_CHECKPOINTED;
-			if (!(data & reserved_bits)) {
+			if (data & reserved_bits)
+				return 1;
+
+			if (data != pmc->eventsel) {
 				pmc->eventsel = data;
 				kvm_pmu_request_counter_reprogam(pmc);
-				return 0;
 			}
-		} else if (intel_pmu_handle_lbr_msrs_access(vcpu, msr_info, false))
-			return 0;
+			break;
+		} else if (intel_pmu_handle_lbr_msrs_access(vcpu, msr_info, false)) {
+			break;
+		}
+		/* Not a known PMU MSR. */
+		return 1;
 	}
 
-	return 1;
+	return 0;
 }
 
 static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
-- 
2.38.1

Re: [PATCH v3 2/8] KVM: VMX: Refactor intel_pmu_set_msr() to align with other set_msr() helpers

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Invert the flows in intel_pmu_set_msr()'s case statements so that they
> follow the kernel's preferred style of:
> 
> 	if (<not valid>)
> 		return <error>
> 
> 	<commit change>
> 	return <success>
> 
> which is also the style used by every other set_msr() helper (except
> AMD's PMU variant, which doesn't use a switch statement).
> 
> Opportunstically move the "val == current" checks below the validity
> checks.  Except for the one-off case for MSR_P6_EVNTSEL2, the reserved
> bit checks are extremely cheap, and the guest is unlikely to frequently
> write the current value, i.e. avoiding the reserved bit checks doesn't
> add much (any?) value.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

FYI, I'm going to post this separately and extend it to give the get_msr() flow
the same treatment.  I'll plan on getting it queued sooner than later so that
this series can use it as a base.

[PATCH v3 3/8] KVM: x86/pmu: Rewrite reprogram_counters() to improve performance

[Like Xu]

From: Like Xu <likexu@tencent.com>

A valid pmc is always tested before using pmu->reprogram_pmi. Eliminate
this part of the redundancy by setting the counter's bitmask directly,
and in addition, trigger KVM_REQ_PMU only once to save more cpu cycles.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/pmu.h           | 11 +++++++++++
 arch/x86/kvm/vmx/pmu_intel.c | 12 ------------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 2b5376ba66ea..be552c8217a0 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -189,6 +189,17 @@ static inline void kvm_pmu_request_counter_reprogam(struct kvm_pmc *pmc)
 	kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
 }
 
+static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
+{
+	int bit;
+
+	if (diff) {
+		for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX)
+			__set_bit(bit, pmu->reprogram_pmi);
+		kvm_make_request(KVM_REQ_PMU, pmu_to_vcpu(pmu));
+	}
+}
+
 void kvm_pmu_deliver_pmi(struct kvm_vcpu *vcpu);
 void kvm_pmu_handle_event(struct kvm_vcpu *vcpu);
 int kvm_pmu_rdpmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 2f7cd388859c..db704eea2d7c 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -68,18 +68,6 @@ static struct kvm_pmc *intel_pmc_idx_to_pmc(struct kvm_pmu *pmu, int pmc_idx)
 	}
 }
 
-static void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
-{
-	int bit;
-	struct kvm_pmc *pmc;
-
-	for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX) {
-		pmc = intel_pmc_idx_to_pmc(pmu, bit);
-		if (pmc)
-			kvm_pmu_request_counter_reprogam(pmc);
-	}
-}
-
 static bool intel_hw_event_available(struct kvm_pmc *pmc)
 {
 	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
-- 
2.38.1

Re: [PATCH v3 3/8] KVM: x86/pmu: Rewrite reprogram_counters() to improve performance

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> A valid pmc is always tested before using pmu->reprogram_pmi. Eliminate
> this part of the redundancy by setting the counter's bitmask directly,
> and in addition, trigger KVM_REQ_PMU only once to save more cpu cycles.
> 
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/pmu.h           | 11 +++++++++++
>  arch/x86/kvm/vmx/pmu_intel.c | 12 ------------
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 2b5376ba66ea..be552c8217a0 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -189,6 +189,17 @@ static inline void kvm_pmu_request_counter_reprogam(struct kvm_pmc *pmc)
>  	kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>  }
>  
> +static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
> +{
> +	int bit;
> +
> +	if (diff) {

Prefer this, it's slightly cleaner:

	if (!diff)
		return;

> +		for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX)
> +			__set_bit(bit, pmu->reprogram_pmi);

Needs to use the atomic set_bit().

Re: [PATCH v3 3/8] KVM: x86/pmu: Rewrite reprogram_counters() to improve performance

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> A valid pmc is always tested before using pmu->reprogram_pmi. Eliminate
> this part of the redundancy by setting the counter's bitmask directly,
> and in addition, trigger KVM_REQ_PMU only once to save more cpu cycles.

It's a little silly, but can you split this into two patches?  First optimize the
helper, then expose it in pmu.h.  The optimization stands on its own, whereas the
code movement is justified only by the incoming AMD PMU v2 support.

> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/pmu.h           | 11 +++++++++++
>  arch/x86/kvm/vmx/pmu_intel.c | 12 ------------
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 2b5376ba66ea..be552c8217a0 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -189,6 +189,17 @@ static inline void kvm_pmu_request_counter_reprogam(struct kvm_pmc *pmc)
>  	kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>  }
>  
> +static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
> +{
> +	int bit;
> +
> +	if (diff) {
> +		for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX)
> +			__set_bit(bit, pmu->reprogram_pmi);
> +		kvm_make_request(KVM_REQ_PMU, pmu_to_vcpu(pmu));
> +	}
> +}
> +
>  void kvm_pmu_deliver_pmi(struct kvm_vcpu *vcpu);
>  void kvm_pmu_handle_event(struct kvm_vcpu *vcpu);
>  int kvm_pmu_rdpmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index 2f7cd388859c..db704eea2d7c 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -68,18 +68,6 @@ static struct kvm_pmc *intel_pmc_idx_to_pmc(struct kvm_pmu *pmu, int pmc_idx)
>  	}
>  }
>  
> -static void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
> -{
> -	int bit;
> -	struct kvm_pmc *pmc;
> -
> -	for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX) {
> -		pmc = intel_pmc_idx_to_pmc(pmu, bit);
> -		if (pmc)
> -			kvm_pmu_request_counter_reprogam(pmc);
> -	}
> -}
> -
>  static bool intel_hw_event_available(struct kvm_pmc *pmc)
>  {
>  	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
> -- 
> 2.38.1
> 

[PATCH v3 4/8] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic

[Like Xu]

From: Like Xu <likexu@tencent.com>

The AMD PerfMonV2 defines three registers similar to part of the Intel
v2 PMU registers, including the GLOBAL_CTRL, GLOBAL_STATUS and
GLOBAL_OVF_CTRL MSRs. For better code reuse, this specific part of
the handling can be extracted to make it generic for X86 as a straight
code movement.

Specifically, move the kvm_pmu_set/get_msr() hanlders of GLOBAL_STATUS,
GLOBAL_CTRL, GLOBAL_OVF_CTRL defined by intel to generic pmu.c and
remove the callback function .pmc_is_globally_enabled, which is very
helpful to introduce the AMD PerfMonV2 code later.

The new non-prefix pmc_is_globally_enabled() works well as legacy AMD
vPMU version is indexed as 1. Note that the specific *_is_valid_msr will
continue to be used to avoid cross-vendor msr access.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/include/asm/kvm-x86-pmu-ops.h |  1 -
 arch/x86/kvm/pmu.c                     | 55 +++++++++++++++++++++++---
 arch/x86/kvm/pmu.h                     | 17 +++++++-
 arch/x86/kvm/svm/pmu.c                 |  9 -----
 arch/x86/kvm/vmx/pmu_intel.c           | 46 +--------------------
 5 files changed, 67 insertions(+), 61 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-pmu-ops.h b/arch/x86/include/asm/kvm-x86-pmu-ops.h
index 86a3fb01e103..6c98f4bb4228 100644
--- a/arch/x86/include/asm/kvm-x86-pmu-ops.h
+++ b/arch/x86/include/asm/kvm-x86-pmu-ops.h
@@ -13,7 +13,6 @@ BUILD_BUG_ON(1)
  * at the call sites.
  */
 KVM_X86_PMU_OP(hw_event_available)
-KVM_X86_PMU_OP(pmc_is_globally_enabled)
 KVM_X86_PMU_OP(pmc_idx_to_pmc)
 KVM_X86_PMU_OP(rdpmc_ecx_to_pmc)
 KVM_X86_PMU_OP(msr_idx_to_pmc)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index e57f707fb940..a3726af5416d 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -83,11 +83,6 @@ void kvm_pmu_ops_update(const struct kvm_pmu_ops *pmu_ops)
 #undef __KVM_X86_PMU_OP
 }
 
-static inline bool pmc_is_globally_enabled(struct kvm_pmc *pmc)
-{
-	return static_call(kvm_x86_pmu_pmc_is_enabled)(pmc);
-}
-
 static void kvm_pmi_trigger_fn(struct irq_work *irq_work)
 {
 	struct kvm_pmu *pmu = container_of(irq_work, struct kvm_pmu, irq_work);
@@ -471,11 +466,61 @@ static void kvm_pmu_mark_pmc_in_use(struct kvm_vcpu *vcpu, u32 msr)
 
 int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
+	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+	u32 msr = msr_info->index;
+
+	switch (msr) {
+	case MSR_CORE_PERF_GLOBAL_STATUS:
+		msr_info->data = pmu->global_status;
+		return 0;
+	case MSR_CORE_PERF_GLOBAL_CTRL:
+		msr_info->data = pmu->global_ctrl;
+		return 0;
+	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
+		msr_info->data = 0;
+		return 0;
+	default:
+		break;
+	}
+
 	return static_call(kvm_x86_pmu_get_msr)(vcpu, msr_info);
 }
 
 int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
+	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+	u32 msr = msr_info->index;
+	u64 data = msr_info->data;
+	u64 diff;
+
+	switch (msr) {
+	case MSR_CORE_PERF_GLOBAL_STATUS:
+		if (!msr_info->host_initiated)
+			return 1; /* RO MSR */
+
+		pmu->global_status = data;
+		return 0;
+	case MSR_CORE_PERF_GLOBAL_CTRL:
+		if (!kvm_valid_perf_global_ctrl(pmu, data))
+			return 1;
+
+		if (pmu->global_ctrl != data) {
+			diff = pmu->global_ctrl ^ data;
+			pmu->global_ctrl = data;
+			reprogram_counters(pmu, diff);
+		}
+		return 0;
+	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
+		if (data & pmu->global_ovf_ctrl_mask)
+			return 1;
+
+		if (!msr_info->host_initiated)
+			pmu->global_status &= ~data;
+		return 0;
+	default:
+		break;
+	}
+
 	kvm_pmu_mark_pmc_in_use(vcpu, msr_info->index);
 	return static_call(kvm_x86_pmu_set_msr)(vcpu, msr_info);
 }
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index be552c8217a0..8739e5ea2835 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -26,7 +26,6 @@ struct kvm_event_hw_type_mapping {
 
 struct kvm_pmu_ops {
 	bool (*hw_event_available)(struct kvm_pmc *pmc);
-	bool (*pmc_is_globally_enabled)(struct kvm_pmc *pmc);
 	struct kvm_pmc *(*pmc_idx_to_pmc)(struct kvm_pmu *pmu, int pmc_idx);
 	struct kvm_pmc *(*rdpmc_ecx_to_pmc)(struct kvm_vcpu *vcpu,
 		unsigned int idx, u64 *mask);
@@ -200,6 +199,22 @@ static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
 	}
 }
 
+/*
+ * Check if a PMC is enabled by comparing it against global_ctrl bits.
+ *
+ * If the current version of vPMU doesn't have global_ctrl MSR,
+ * all vPMCs are enabled (return TRUE).
+ */
+static inline bool pmc_is_globally_enabled(struct kvm_pmc *pmc)
+{
+	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
+
+	if (pmu->version < 2)
+		return true;
+
+	return test_bit(pmc->idx, (unsigned long *)&pmu->global_ctrl);
+}
+
 void kvm_pmu_deliver_pmi(struct kvm_vcpu *vcpu);
 void kvm_pmu_handle_event(struct kvm_vcpu *vcpu);
 int kvm_pmu_rdpmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 7958a983b760..4e7d7e6cccec 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -76,14 +76,6 @@ static bool amd_hw_event_available(struct kvm_pmc *pmc)
 	return true;
 }
 
-/* check if a PMC is enabled by comparing it against global_ctrl bits. Because
- * AMD CPU doesn't have global_ctrl MSR, all PMCs are enabled (return TRUE).
- */
-static bool amd_pmc_is_enabled(struct kvm_pmc *pmc)
-{
-	return true;
-}
-
 static bool amd_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
@@ -218,7 +210,6 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu)
 
 struct kvm_pmu_ops amd_pmu_ops __initdata = {
 	.hw_event_available = amd_hw_event_available,
-	.pmc_is_globally_enabled = amd_pmc_is_enabled,
 	.pmc_idx_to_pmc = amd_pmc_idx_to_pmc,
 	.rdpmc_ecx_to_pmc = amd_rdpmc_ecx_to_pmc,
 	.msr_idx_to_pmc = amd_msr_idx_to_pmc,
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index db704eea2d7c..f95f8d1db2cf 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -90,17 +90,6 @@ static bool intel_hw_event_available(struct kvm_pmc *pmc)
 	return true;
 }
 
-/* check if a PMC is enabled by comparing it with globl_ctrl bits. */
-static bool intel_pmc_is_enabled(struct kvm_pmc *pmc)
-{
-	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
-
-	if (!intel_pmu_has_perf_global_ctrl(pmu))
-		return true;
-
-	return test_bit(pmc->idx, (unsigned long *)&pmu->global_ctrl);
-}
-
 static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
@@ -335,15 +324,6 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_CORE_PERF_FIXED_CTR_CTRL:
 		msr_info->data = pmu->fixed_ctr_ctrl;
 		return 0;
-	case MSR_CORE_PERF_GLOBAL_STATUS:
-		msr_info->data = pmu->global_status;
-		return 0;
-	case MSR_CORE_PERF_GLOBAL_CTRL:
-		msr_info->data = pmu->global_ctrl;
-		return 0;
-	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-		msr_info->data = 0;
-		return 0;
 	case MSR_IA32_PEBS_ENABLE:
 		msr_info->data = pmu->pebs_enable;
 		return 0;
@@ -391,29 +371,6 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (pmu->fixed_ctr_ctrl != data)
 			reprogram_fixed_counters(pmu, data);
 		break;
-	case MSR_CORE_PERF_GLOBAL_STATUS:
-		if (!msr_info->host_initiated)
-			return 1; /* RO MSR */
-
-		pmu->global_status = data;
-		break;
-	case MSR_CORE_PERF_GLOBAL_CTRL:
-		if (!kvm_valid_perf_global_ctrl(pmu, data))
-			return 1;
-
-		if (pmu->global_ctrl != data) {
-			diff = pmu->global_ctrl ^ data;
-			pmu->global_ctrl = data;
-			reprogram_counters(pmu, diff);
-		}
-		break;
-	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-		if (data & pmu->global_ovf_ctrl_mask)
-			return 1;
-
-		if (!msr_info->host_initiated)
-			pmu->global_status &= ~data;
-		break;
 	case MSR_IA32_PEBS_ENABLE:
 		if (data & pmu->pebs_enable_mask)
 			return 1;
@@ -773,7 +730,7 @@ void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu)
 		pmc = intel_pmc_idx_to_pmc(pmu, bit);
 
 		if (!pmc || !pmc_speculative_in_use(pmc) ||
-		    !intel_pmc_is_enabled(pmc) || !pmc->perf_event)
+		    !pmc_is_globally_enabled(pmc) || !pmc->perf_event)
 			continue;
 
 		/*
@@ -788,7 +745,6 @@ void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu)
 
 struct kvm_pmu_ops intel_pmu_ops __initdata = {
 	.hw_event_available = intel_hw_event_available,
-	.pmc_is_globally_enabled = intel_pmc_is_enabled,
 	.pmc_idx_to_pmc = intel_pmc_idx_to_pmc,
 	.rdpmc_ecx_to_pmc = intel_rdpmc_ecx_to_pmc,
 	.msr_idx_to_pmc = intel_msr_idx_to_pmc,
-- 
2.38.1

[PATCH v3 5/8] KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry

[Like Xu]

From: Like Xu <likexu@tencent.com>

Alias X86_FEATURE_AMD_PMU_V2 for feature AMD_PMU_V2 in KVM-only leafs that
aren't scattered by cpufeatures.h so that it can be used in KVM, e.g. to
query guest CPUID.  As a bonus, no translation is needed for these features
in __feature_translate().

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/reverse_cpuid.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
index a19d473d0184..7cfedb3e47c0 100644
--- a/arch/x86/kvm/reverse_cpuid.h
+++ b/arch/x86/kvm/reverse_cpuid.h
@@ -13,6 +13,7 @@
  */
 enum kvm_only_cpuid_leafs {
 	CPUID_12_EAX	 = NCAPINTS,
+	CPUID_8000_0022_EAX,
 	NR_KVM_CPU_CAPS,
 
 	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
@@ -23,7 +24,15 @@ enum kvm_only_cpuid_leafs {
 /* Intel-defined SGX sub-features, CPUID level 0x12 (EAX). */
 #define KVM_X86_FEATURE_SGX1		KVM_X86_FEATURE(CPUID_12_EAX, 0)
 #define KVM_X86_FEATURE_SGX2		KVM_X86_FEATURE(CPUID_12_EAX, 1)
+#define KVM_X86_FEATURE_AMD_PMU_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
 
+/*
+ * Alias X86_FEATURE_* to the KVM variant for features in KVM-only leafs that
+ * aren't scattered by cpufeatures.h so that X86_FEATURE_* can be used in KVM,
+ * e.g. to query guest CPUID.  As a bonus, no translation is needed for these
+ * features in __feature_translate().
+ */
+#define X86_FEATURE_AMD_PMU_V2      KVM_X86_FEATURE_AMD_PMU_V2
 struct cpuid_reg {
 	u32 function;
 	u32 index;
@@ -48,6 +57,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
 	[CPUID_7_1_EAX]       = {         7, 1, CPUID_EAX},
 	[CPUID_12_EAX]        = {0x00000012, 0, CPUID_EAX},
 	[CPUID_8000_001F_EAX] = {0x8000001f, 0, CPUID_EAX},
+	[CPUID_8000_0022_EAX] = {0x80000022, 0, CPUID_EAX},
 };
 
 /*
-- 
2.38.1

Re: [PATCH v3 5/8] KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> Alias X86_FEATURE_AMD_PMU_V2 for feature AMD_PMU_V2 in KVM-only leafs that
> aren't scattered by cpufeatures.h so that it can be used in KVM, e.g. to
> query guest CPUID.  As a bonus, no translation is needed for these features
> in __feature_translate().
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/reverse_cpuid.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
> index a19d473d0184..7cfedb3e47c0 100644
> --- a/arch/x86/kvm/reverse_cpuid.h
> +++ b/arch/x86/kvm/reverse_cpuid.h
> @@ -13,6 +13,7 @@
>   */
>  enum kvm_only_cpuid_leafs {
>  	CPUID_12_EAX	 = NCAPINTS,
> +	CPUID_8000_0022_EAX,
>  	NR_KVM_CPU_CAPS,
>  
>  	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
> @@ -23,7 +24,15 @@ enum kvm_only_cpuid_leafs {
>  /* Intel-defined SGX sub-features, CPUID level 0x12 (EAX). */
>  #define KVM_X86_FEATURE_SGX1		KVM_X86_FEATURE(CPUID_12_EAX, 0)
>  #define KVM_X86_FEATURE_SGX2		KVM_X86_FEATURE(CPUID_12_EAX, 1)
> +#define KVM_X86_FEATURE_AMD_PMU_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
>  
> +/*
> + * Alias X86_FEATURE_* to the KVM variant for features in KVM-only leafs that
> + * aren't scattered by cpufeatures.h so that X86_FEATURE_* can be used in KVM,
> + * e.g. to query guest CPUID.  As a bonus, no translation is needed for these
> + * features in __feature_translate().
> + */
> +#define X86_FEATURE_AMD_PMU_V2      KVM_X86_FEATURE_AMD_PMU_V2

I gave you bad input earlier, for purely KVM-defined flags there's no need for an
intermediate KVM_X86_FEATURE_AMD_PMU_V2, this could simply be:

  #define X86_FEATURE_AMD_PMU_V2         KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)

That's a moot point though because, after much searching because I had a very hard
time believing the kernel wouldn't want to know about this flag, I found commit

  d6d0c7f681fd ("x86/cpufeatures: Add PerfMonV2 feature bit")

from nearly a year ago.  I.e. to avoid confusiong, this needs to be a scattered
flag, not a purely KVM flag.

---
 arch/x86/kvm/reverse_cpuid.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
index 4945456fd646..333e28b0a13c 100644
--- a/arch/x86/kvm/reverse_cpuid.h
+++ b/arch/x86/kvm/reverse_cpuid.h
@@ -15,6 +15,7 @@ enum kvm_only_cpuid_leafs {
 	CPUID_12_EAX	 = NCAPINTS,
 	CPUID_7_1_EDX,
 	CPUID_8000_0007_EDX,
+	CPUID_8000_0022_EAX,
 	NR_KVM_CPU_CAPS,
 
 	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
@@ -47,6 +48,9 @@ enum kvm_only_cpuid_leafs {
 /* CPUID level 0x80000007 (EDX). */
 #define KVM_X86_FEATURE_CONSTANT_TSC	KVM_X86_FEATURE(CPUID_8000_0007_EDX, 8)
 
+/* CPUID level 0x80000022 (EAX) */
+#define KVM_X86_FEATURE_PERFMON_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
+
 struct cpuid_reg {
 	u32 function;
 	u32 index;
@@ -73,6 +77,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
 	[CPUID_8000_001F_EAX] = {0x8000001f, 0, CPUID_EAX},
 	[CPUID_7_1_EDX]       = {         7, 1, CPUID_EDX},
 	[CPUID_8000_0007_EDX] = {0x80000007, 0, CPUID_EDX},
+	[CPUID_8000_0022_EAX] = {0x80000022, 0, CPUID_EAX},
 };
 
 /*
@@ -107,6 +112,8 @@ static __always_inline u32 __feature_translate(int x86_feature)
 		return KVM_X86_FEATURE_SGX_EDECCSSA;
 	else if (x86_feature == X86_FEATURE_CONSTANT_TSC)
 		return KVM_X86_FEATURE_CONSTANT_TSC;
+	else if (x86_feature == X86_FEATURE_PERFMON_V2)
+		return KVM_X86_FEATURE_PERFMON_V2;
 
 	return x86_feature;
 }

base-commit: 5f3f3cc1279cd5cd52d301b97844bd3ce40c8020
-- 

Re: [PATCH v3 5/8] KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry

[Like Xu]

On 25/1/2023 3:47 am, Sean Christopherson wrote:
> On Fri, Nov 11, 2022, Like Xu wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> Alias X86_FEATURE_AMD_PMU_V2 for feature AMD_PMU_V2 in KVM-only leafs that
>> aren't scattered by cpufeatures.h so that it can be used in KVM, e.g. to
>> query guest CPUID.  As a bonus, no translation is needed for these features
>> in __feature_translate().
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Like Xu <likexu@tencent.com>
>> ---
>>   arch/x86/kvm/reverse_cpuid.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
>> index a19d473d0184..7cfedb3e47c0 100644
>> --- a/arch/x86/kvm/reverse_cpuid.h
>> +++ b/arch/x86/kvm/reverse_cpuid.h
>> @@ -13,6 +13,7 @@
>>    */
>>   enum kvm_only_cpuid_leafs {
>>   	CPUID_12_EAX	 = NCAPINTS,
>> +	CPUID_8000_0022_EAX,
>>   	NR_KVM_CPU_CAPS,
>>   
>>   	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
>> @@ -23,7 +24,15 @@ enum kvm_only_cpuid_leafs {
>>   /* Intel-defined SGX sub-features, CPUID level 0x12 (EAX). */
>>   #define KVM_X86_FEATURE_SGX1		KVM_X86_FEATURE(CPUID_12_EAX, 0)
>>   #define KVM_X86_FEATURE_SGX2		KVM_X86_FEATURE(CPUID_12_EAX, 1)
>> +#define KVM_X86_FEATURE_AMD_PMU_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
>>   
>> +/*
>> + * Alias X86_FEATURE_* to the KVM variant for features in KVM-only leafs that
>> + * aren't scattered by cpufeatures.h so that X86_FEATURE_* can be used in KVM,
>> + * e.g. to query guest CPUID.  As a bonus, no translation is needed for these
>> + * features in __feature_translate().
>> + */
>> +#define X86_FEATURE_AMD_PMU_V2      KVM_X86_FEATURE_AMD_PMU_V2
> 
> I gave you bad input earlier, for purely KVM-defined flags there's no need for an
> intermediate KVM_X86_FEATURE_AMD_PMU_V2, this could simply be:
> 
>    #define X86_FEATURE_AMD_PMU_V2         KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
> 
> That's a moot point though because, after much searching because I had a very hard
> time believing the kernel wouldn't want to know about this flag, I found commit
> 
>    d6d0c7f681fd ("x86/cpufeatures: Add PerfMonV2 feature bit")
> 
> from nearly a year ago.  I.e. to avoid confusiong, this needs to be a scattered
> flag, not a purely KVM flag.
> 
> ---
>   arch/x86/kvm/reverse_cpuid.h | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
> index 4945456fd646..333e28b0a13c 100644
> --- a/arch/x86/kvm/reverse_cpuid.h
> +++ b/arch/x86/kvm/reverse_cpuid.h
> @@ -15,6 +15,7 @@ enum kvm_only_cpuid_leafs {
>   	CPUID_12_EAX	 = NCAPINTS,
>   	CPUID_7_1_EDX,
>   	CPUID_8000_0007_EDX,
> +	CPUID_8000_0022_EAX,
>   	NR_KVM_CPU_CAPS,
>   
>   	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
> @@ -47,6 +48,9 @@ enum kvm_only_cpuid_leafs {
>   /* CPUID level 0x80000007 (EDX). */
>   #define KVM_X86_FEATURE_CONSTANT_TSC	KVM_X86_FEATURE(CPUID_8000_0007_EDX, 8)
>   
> +/* CPUID level 0x80000022 (EAX) */
> +#define KVM_X86_FEATURE_PERFMON_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)

I'm very confused and is this the usage you want to see:

	kvm_cpu_cap_set(KVM_X86_FEATURE_PERFMON_V2)
	kvm_cpu_cap_has(KVM_X86_FEATURE_PERFMON_V2)
	guest_cpuid_has(vcpu, X86_FEATURE_PERFMON_V2)

? then what about X86_FEATURE_PERFMON_V2 ?

> +
>   struct cpuid_reg {
>   	u32 function;
>   	u32 index;
> @@ -73,6 +77,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
>   	[CPUID_8000_001F_EAX] = {0x8000001f, 0, CPUID_EAX},
>   	[CPUID_7_1_EDX]       = {         7, 1, CPUID_EDX},
>   	[CPUID_8000_0007_EDX] = {0x80000007, 0, CPUID_EDX},
> +	[CPUID_8000_0022_EAX] = {0x80000022, 0, CPUID_EAX},
>   };
>   
>   /*
> @@ -107,6 +112,8 @@ static __always_inline u32 __feature_translate(int x86_feature)
>   		return KVM_X86_FEATURE_SGX_EDECCSSA;
>   	else if (x86_feature == X86_FEATURE_CONSTANT_TSC)
>   		return KVM_X86_FEATURE_CONSTANT_TSC;
> +	else if (x86_feature == X86_FEATURE_PERFMON_V2)
> +		return KVM_X86_FEATURE_PERFMON_V2;
>   
>   	return x86_feature;
>   }
> 
> base-commit: 5f3f3cc1279cd5cd52d301b97844bd3ce40c8020

Re: [PATCH v3 5/8] KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry

[Sean Christopherson]

On Mon, Feb 06, 2023, Like Xu wrote:
> On 25/1/2023 3:47 am, Sean Christopherson wrote:
> > On Fri, Nov 11, 2022, Like Xu wrote:
> > > From: Like Xu <likexu@tencent.com>
> > > 
> > > Alias X86_FEATURE_AMD_PMU_V2 for feature AMD_PMU_V2 in KVM-only leafs that
> > > aren't scattered by cpufeatures.h so that it can be used in KVM, e.g. to
> > > query guest CPUID.  As a bonus, no translation is needed for these features
> > > in __feature_translate().
> > > 
> > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: Like Xu <likexu@tencent.com>
> > > ---
> > >   arch/x86/kvm/reverse_cpuid.h | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
> > > index a19d473d0184..7cfedb3e47c0 100644
> > > --- a/arch/x86/kvm/reverse_cpuid.h
> > > +++ b/arch/x86/kvm/reverse_cpuid.h
> > > @@ -13,6 +13,7 @@
> > >    */
> > >   enum kvm_only_cpuid_leafs {
> > >   	CPUID_12_EAX	 = NCAPINTS,
> > > +	CPUID_8000_0022_EAX,
> > >   	NR_KVM_CPU_CAPS,
> > >   	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
> > > @@ -23,7 +24,15 @@ enum kvm_only_cpuid_leafs {
> > >   /* Intel-defined SGX sub-features, CPUID level 0x12 (EAX). */
> > >   #define KVM_X86_FEATURE_SGX1		KVM_X86_FEATURE(CPUID_12_EAX, 0)
> > >   #define KVM_X86_FEATURE_SGX2		KVM_X86_FEATURE(CPUID_12_EAX, 1)
> > > +#define KVM_X86_FEATURE_AMD_PMU_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
> > > +/*
> > > + * Alias X86_FEATURE_* to the KVM variant for features in KVM-only leafs that
> > > + * aren't scattered by cpufeatures.h so that X86_FEATURE_* can be used in KVM,
> > > + * e.g. to query guest CPUID.  As a bonus, no translation is needed for these
> > > + * features in __feature_translate().
> > > + */
> > > +#define X86_FEATURE_AMD_PMU_V2      KVM_X86_FEATURE_AMD_PMU_V2
> > 
> > I gave you bad input earlier, for purely KVM-defined flags there's no need for an
> > intermediate KVM_X86_FEATURE_AMD_PMU_V2, this could simply be:
> > 
> >    #define X86_FEATURE_AMD_PMU_V2         KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
> > 
> > That's a moot point though because, after much searching because I had a very hard
> > time believing the kernel wouldn't want to know about this flag, I found commit
> > 
> >    d6d0c7f681fd ("x86/cpufeatures: Add PerfMonV2 feature bit")
> > 
> > from nearly a year ago.  I.e. to avoid confusiong, this needs to be a scattered
> > flag, not a purely KVM flag.
> > 
> > ---
> >   arch/x86/kvm/reverse_cpuid.h | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
> > index 4945456fd646..333e28b0a13c 100644
> > --- a/arch/x86/kvm/reverse_cpuid.h
> > +++ b/arch/x86/kvm/reverse_cpuid.h
> > @@ -15,6 +15,7 @@ enum kvm_only_cpuid_leafs {
> >   	CPUID_12_EAX	 = NCAPINTS,
> >   	CPUID_7_1_EDX,
> >   	CPUID_8000_0007_EDX,
> > +	CPUID_8000_0022_EAX,
> >   	NR_KVM_CPU_CAPS,
> >   	NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
> > @@ -47,6 +48,9 @@ enum kvm_only_cpuid_leafs {
> >   /* CPUID level 0x80000007 (EDX). */
> >   #define KVM_X86_FEATURE_CONSTANT_TSC	KVM_X86_FEATURE(CPUID_8000_0007_EDX, 8)
> > +/* CPUID level 0x80000022 (EAX) */
> > +#define KVM_X86_FEATURE_PERFMON_V2	KVM_X86_FEATURE(CPUID_8000_0022_EAX, 0)
> 
> I'm very confused and is this the usage you want to see:
> 
> 	kvm_cpu_cap_set(KVM_X86_FEATURE_PERFMON_V2)
> 	kvm_cpu_cap_has(KVM_X86_FEATURE_PERFMON_V2)
> 	guest_cpuid_has(vcpu, X86_FEATURE_PERFMON_V2)
> 
> ?

No, all of those should just use X86_FEATURE_PERFMON_V2, i.e. the existing flag
from cpufeatures.h.

> then what about X86_FEATURE_PERFMON_V2 ?

Not sure I follow.  As above, it's scattered, thus KVM needs to redirect it to
the hardware-defined bit position, which is the role of __feature_translate()
and KVM_X86_FEATURE_PERFMON_V2.

[PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support

[Like Xu]

From: Like Xu <likexu@tencent.com>

If AMD Performance Monitoring Version 2 (PerfMonV2) is detected by
the guest, it can use a new scheme to manage the Core PMCs using the
new global control and status registers.

In addition to benefiting from the PerfMonV2 functionality in the same
way as the host (higher precision), the guest also can reduce the number
of vm-exits by lowering the total number of MSRs accesses.

In terms of implementation details, amd_is_valid_msr() is resurrected
since three newly added MSRs could not be mapped to one vPMC.
The possibility of emulating PerfMonV2 on the mainframe has also
been eliminated for reasons of precision.

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/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/pmu.c              |  6 ++++
 arch/x86/kvm/svm/pmu.c          | 64 +++++++++++++++++++++++++++------
 arch/x86/kvm/x86.c              | 14 ++++++--
 4 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 81114a376c4e..d02990fcd46f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -512,6 +512,7 @@ struct kvm_pmc {
 #define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
 #define KVM_PMC_MAX_FIXED	3
 #define KVM_AMD_PMC_MAX_GENERIC	6
+#define MSR_F15H_PERF_MSR_MAX	(MSR_F15H_PERF_CTR0 + 2 * (KVM_AMD_PMC_MAX_GENERIC - 1))
 struct kvm_pmu {
 	unsigned nr_arch_gp_counters;
 	unsigned nr_arch_fixed_counters;
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index a3726af5416d..c70ff57ee44c 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -471,12 +471,15 @@ int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 	switch (msr) {
 	case MSR_CORE_PERF_GLOBAL_STATUS:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
 		msr_info->data = pmu->global_status;
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
 		msr_info->data = pmu->global_ctrl;
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
 		msr_info->data = 0;
 		return 0;
 	default:
@@ -495,12 +498,14 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 	switch (msr) {
 	case MSR_CORE_PERF_GLOBAL_STATUS:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
 		if (!msr_info->host_initiated)
 			return 1; /* RO MSR */
 
 		pmu->global_status = data;
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
 		if (!kvm_valid_perf_global_ctrl(pmu, data))
 			return 1;
 
@@ -511,6 +516,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		}
 		return 0;
 	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
 		if (data & pmu->global_ovf_ctrl_mask)
 			return 1;
 
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 4e7d7e6cccec..e58f39f8f10b 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -92,12 +92,6 @@ static struct kvm_pmc *amd_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
 	return amd_pmc_idx_to_pmc(vcpu_to_pmu(vcpu), idx & ~(3u << 30));
 }
 
-static bool amd_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
-{
-	/* All MSRs refer to exactly one PMC, so msr_idx_to_pmc is enough.  */
-	return false;
-}
-
 static struct kvm_pmc *amd_msr_idx_to_pmc(struct kvm_vcpu *vcpu, u32 msr)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
@@ -109,6 +103,29 @@ static struct kvm_pmc *amd_msr_idx_to_pmc(struct kvm_vcpu *vcpu, u32 msr)
 	return pmc;
 }
 
+static bool amd_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
+{
+	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+
+	switch (msr) {
+	case MSR_K7_EVNTSEL0 ... MSR_K7_PERFCTR3:
+		return pmu->version > 0;
+	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5:
+		return guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE);
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
+		return pmu->version > 1;
+	default:
+		if (msr > MSR_F15H_PERF_CTR5 &&
+		    msr < MSR_F15H_PERF_CTL0 + 2 * pmu->nr_arch_gp_counters)
+			return pmu->version > 1;
+		break;
+	}
+
+	return amd_msr_idx_to_pmc(vcpu, msr);
+}
+
 static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
@@ -162,20 +179,42 @@ 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);
+	struct kvm_cpuid_entry2 *entry;
+	union cpuid_0x80000022_ebx ebx;
 
-	if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
-		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
+	pmu->version = 1;
+	if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2) &&
+	    guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2)) {
+		pmu->version = 2;
+		entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
+		ebx.full = entry->ebx;
+		pmu->nr_arch_gp_counters = min3((unsigned int)ebx.split.num_core_pmc,
+						(unsigned int)kvm_pmu_cap.num_counters_gp,
+						(unsigned int)KVM_AMD_PMC_MAX_GENERIC);
+	}
+
+	/* Commitment to minimal PMCs, regardless of CPUID.80000022 */
+	if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE) &&
+	    guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
+		pmu->nr_arch_gp_counters = max_t(unsigned int,
+						 pmu->nr_arch_gp_counters,
+						 AMD64_NUM_COUNTERS_CORE);
 	else
-		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
+		pmu->nr_arch_gp_counters = max_t(unsigned int,
+						 pmu->nr_arch_gp_counters,
+						 AMD64_NUM_COUNTERS);
+
+	if (pmu->version > 1) {
+		pmu->global_ctrl_mask = ~((1ull << pmu->nr_arch_gp_counters) - 1);
+		pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask;
+	}
 
 	pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << 48) - 1;
 	pmu->reserved_bits = 0xfffffff000280000ull;
 	pmu->raw_event_mask = AMD64_RAW_EVENT_MASK;
-	pmu->version = 1;
 	/* not applicable to AMD; but clean them to prevent any fall out */
 	pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
 	pmu->nr_arch_fixed_counters = 0;
-	pmu->global_status = 0;
 	bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
 }
 
@@ -186,6 +225,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
 
 	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > AMD64_NUM_COUNTERS_CORE);
 	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > INTEL_PMC_MAX_GENERIC);
+	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC < 1);
 
 	for (i = 0; i < KVM_AMD_PMC_MAX_GENERIC ; i++) {
 		pmu->gp_counters[i].type = KVM_PMC_GP;
@@ -206,6 +246,8 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu)
 		pmc_stop_counter(pmc);
 		pmc->counter = pmc->prev_counter = pmc->eventsel = 0;
 	}
+
+	pmu->global_ctrl = pmu->global_status = 0;
 }
 
 struct kvm_pmu_ops amd_pmu_ops __initdata = {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e46e458c5b08..99bc47f1a40e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1458,6 +1458,10 @@ static const u32 msrs_to_save_all[] = {
 	MSR_F15H_PERF_CTR0, MSR_F15H_PERF_CTR1, MSR_F15H_PERF_CTR2,
 	MSR_F15H_PERF_CTR3, MSR_F15H_PERF_CTR4, MSR_F15H_PERF_CTR5,
 
+	MSR_AMD64_PERF_CNTR_GLOBAL_CTL,
+	MSR_AMD64_PERF_CNTR_GLOBAL_STATUS,
+	MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR,
+
 	MSR_IA32_XFD, MSR_IA32_XFD_ERR,
 };
 
@@ -3859,7 +3863,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_PEBS_ENABLE:
 	case MSR_IA32_DS_AREA:
 	case MSR_PEBS_DATA_CFG:
-	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5:
+	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_MSR_MAX:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
 		if (kvm_pmu_is_valid_msr(vcpu, msr))
 			return kvm_pmu_set_msr(vcpu, msr_info);
 		/*
@@ -3962,7 +3969,10 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_PEBS_ENABLE:
 	case MSR_IA32_DS_AREA:
 	case MSR_PEBS_DATA_CFG:
-	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5:
+	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_MSR_MAX:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
+	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
 			return kvm_pmu_get_msr(vcpu, msr_info);
 		/*
-- 
2.38.1

Re: [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
On Fri, Nov 11, 2022, Like Xu wrote:
> @@ -162,20 +179,42 @@ 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);
> +     struct kvm_cpuid_entry2 *entry;
> +     union cpuid_0x80000022_ebx ebx;
>
> -     if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
> -             pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
> +     pmu->version = 1;
> +     if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2) &&

Why check kvm_cpu_cap support?  I.e. what will go wrong if userspace enumerates
PMU v2 to the guest without proper hardware/KVM support.

If this is _necessary_ to protect the host kernel, then we should probably have
a helper to query PMU features, e.g.

static __always_inline bool guest_pmu_has(struct kvm_vcpu *vcpu,
                                          unsigned int x86_feature)
{
        return kvm_cpu_cap_has(x86_feature) &&
               guest_cpuid_has(vcpu, x86_feature);
}



> +         guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2)) {
> +             pmu->version = 2;
> +             entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
> +             ebx.full = entry->ebx;
> +             pmu->nr_arch_gp_counters = min3((unsigned int)ebx.split.num_core_pmc,
> +                                             (unsigned int)kvm_pmu_cap.num_counters_gp,
> +                                             (unsigned int)KVM_AMD_PMC_MAX_GENERIC);

Blech.  This really shouldn't be necessary, KVM should tweak kvm_pmu_cap.num_counters_gp
as needed during initialization to ensure num_counters_gp doesn't exceed KVM's
internal limits.

Posted a patch[*], please take a look.  As mentioned in that thread, I'll somewhat
speculatively apply that series sooner than later so that you can use it a base
for this series (assuming the patch isn't busted).

[*] https://lore.kernel.org/all/20230124234905.3774678-2-seanjc@google.com

> +     }
> +
> +     /* Commitment to minimal PMCs, regardless of CPUID.80000022 */

Please expand this comment.  I'm still not entirely sure I've interpreted it correctly,
and I'm not sure that I agree with the code.

> +     if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE) &&

AFAICT, checking kvm_cpu_cap_has() is an unrelated change.  Either it's a bug fix
and belongs in a separate patch, or it's unnecessary and should be dropped.

> +         guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
> +             pmu->nr_arch_gp_counters = max_t(unsigned int,
> +                                              pmu->nr_arch_gp_counters,
> +                                              AMD64_NUM_COUNTERS_CORE);
>       else
> -             pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
> +             pmu->nr_arch_gp_counters = max_t(unsigned int,
> +                                              pmu->nr_arch_gp_counters,
> +                                              AMD64_NUM_COUNTERS);

Using max() doesn't look right.  E.g. if KVM ends up running on some odd setup
where ebx.split.num_core_pmc/kvm_pmu_cap.num_counters_gp is less than
AMD64_NUM_COUNTERS_CORE or AMD64_NUM_COUNTERS.

Or more likely, if userspace says "only expose N counters to this guest".

Shouldn't this be something like?

	if (guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2))
		pmu->nr_arch_gp_counters = min(ebx.split.num_core_pmc,
					       kvm_pmu_cap.num_counters_gp);
	else if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
	else
		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERSE;

Re: [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support

[Like Xu]

On 25/1/2023 8:10 am, Sean Christopherson wrote:
> On Fri, Nov 11, 2022, Like Xu wrote:
> On Fri, Nov 11, 2022, Like Xu wrote:
>> @@ -162,20 +179,42 @@ 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);
>> +     struct kvm_cpuid_entry2 *entry;
>> +     union cpuid_0x80000022_ebx ebx;
>>
>> -     if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
>> -             pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
>> +     pmu->version = 1;
>> +     if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2) &&
> 
> Why check kvm_cpu_cap support?  I.e. what will go wrong if userspace enumerates
> PMU v2 to the guest without proper hardware/KVM support.
> 
> If this is _necessary_ to protect the host kernel, then we should probably have
> a helper to query PMU features, e.g.
> 
> static __always_inline bool guest_pmu_has(struct kvm_vcpu *vcpu,
>                                            unsigned int x86_feature)
> {
>          return kvm_cpu_cap_has(x86_feature) &&
>                 guest_cpuid_has(vcpu, x86_feature);
> }
> 
> 
> 
>> +         guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2)) {
>> +             pmu->version = 2;
>> +             entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
>> +             ebx.full = entry->ebx;
>> +             pmu->nr_arch_gp_counters = min3((unsigned int)ebx.split.num_core_pmc,
>> +                                             (unsigned int)kvm_pmu_cap.num_counters_gp,
>> +                                             (unsigned int)KVM_AMD_PMC_MAX_GENERIC);
> 
> Blech.  This really shouldn't be necessary, KVM should tweak kvm_pmu_cap.num_counters_gp
> as needed during initialization to ensure num_counters_gp doesn't exceed KVM's
> internal limits.
> 
> Posted a patch[*], please take a look.  As mentioned in that thread, I'll somewhat
> speculatively apply that series sooner than later so that you can use it a base
> for this series (assuming the patch isn't busted).
> 
> [*] https://lore.kernel.org/all/20230124234905.3774678-2-seanjc@google.com
> 
>> +     }
>> +
>> +     /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> 
> Please expand this comment.  I'm still not entirely sure I've interpreted it correctly,
> and I'm not sure that I agree with the code.

In the first version [1], I used almost the same if-elif-else sequence
but the concerns from JimM[2] has changed my mind:

"Nonetheless, for compatibility with old software, Fn8000_0022_EBX can never
report less than four counters (or six, if Fn8000_0001_ECX[PerfCtrExtCore] is set)."

Both in amd_pmu_refresh() and in __do_cpuid_func(), KVM implements
this using the override approach of first applying the semantics of
AMD_PMU_V2 and then implementing a minimum number of counters
supported based on whether or not guest have  PERFCTR_CORE,
the proposed if-elif-else does not fulfill this need.

[1] 20220905123946.95223-4-likexu@tencent.com/
[2] CALMp9eQObuiJGV=YrAU9Fw+KoXfJtZMJ-KUs-qCOVd+R9zGBpw@mail.gmail.com

> 
>> +     if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE) &&
> 
> AFAICT, checking kvm_cpu_cap_has() is an unrelated change.  Either it's a bug fix
> and belongs in a separate patch, or it's unnecessary and should be dropped.
> 
>> +         guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
>> +             pmu->nr_arch_gp_counters = max_t(unsigned int,
>> +                                              pmu->nr_arch_gp_counters,
>> +                                              AMD64_NUM_COUNTERS_CORE);
>>        else
>> -             pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
>> +             pmu->nr_arch_gp_counters = max_t(unsigned int,
>> +                                              pmu->nr_arch_gp_counters,
>> +                                              AMD64_NUM_COUNTERS);
> 
> Using max() doesn't look right.  E.g. if KVM ends up running on some odd setup
> where ebx.split.num_core_pmc/kvm_pmu_cap.num_counters_gp is less than
> AMD64_NUM_COUNTERS_CORE or AMD64_NUM_COUNTERS.
> 
> Or more likely, if userspace says "only expose N counters to this guest".
> 
> Shouldn't this be something like?
> 
> 	if (guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2))
> 		pmu->nr_arch_gp_counters = min(ebx.split.num_core_pmc,
> 					       kvm_pmu_cap.num_counters_gp);
> 	else if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
> 		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
> 	else
> 		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERSE;
> 

Re: [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support

[Sean Christopherson]

On Mon, Feb 06, 2023, Like Xu wrote:
> On 25/1/2023 8:10 am, Sean Christopherson wrote:
> > > +     }
> > > +
> > > +     /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> > 
> > Please expand this comment.  I'm still not entirely sure I've interpreted it correctly,
> > and I'm not sure that I agree with the code.
> 
> In the first version [1], I used almost the same if-elif-else sequence
> but the concerns from JimM[2] has changed my mind:
> 
> "Nonetheless, for compatibility with old software, Fn8000_0022_EBX can never
> report less than four counters (or six, if Fn8000_0001_ECX[PerfCtrExtCore] is set)."
> 
> Both in amd_pmu_refresh() and in __do_cpuid_func(), KVM implements
> this using the override approach of first applying the semantics of
> AMD_PMU_V2 and then implementing a minimum number of counters
> supported based on whether or not guest have  PERFCTR_CORE,
> the proposed if-elif-else does not fulfill this need.

Jim's comments were in the context of __do_cpuid_func(), i.e. KVM_GET_SUPPORTED_CPUID.
As far as guest CPUID is concerned, that's userspace's responsibility to get correct.

And for KVM_GET_SUPPORTED_CPUID, overriding kvm_pmu_cap.num_counters_gp is not
the correct approach.  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 6 counters and perf says there are 4, then something is wrong and enumerating
6 to the guest is only going to cause more problems.

> [1] 20220905123946.95223-4-likexu@tencent.com/
> [2] CALMp9eQObuiJGV=YrAU9Fw+KoXfJtZMJ-KUs-qCOVd+R9zGBpw@mail.gmail.com

[PATCH v3 7/8] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022

[Like Xu]

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   | 30 +++++++++++++++++++++++++++++-
 arch/x86/kvm/svm/svm.c | 11 ++++++++---
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 6b5912578edd..df551fa66ccc 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1113,7 +1113,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
@@ -1229,6 +1229,34 @@ 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_AMD_PMU_V2)) {
+			entry->eax = entry->ebx;
+			break;
+		}
+
+		cpuid_entry_override(entry, CPUID_8000_0022_EAX);
+
+		if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2))
+			ebx.split.num_core_pmc = min(kvm_pmu_cap.num_counters_gp,
+						     KVM_AMD_PMC_MAX_GENERIC);
+
+		if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
+			ebx.split.num_core_pmc = max_t(unsigned int,
+						       ebx.split.num_core_pmc,
+						       AMD64_NUM_COUNTERS_CORE);
+		else
+			ebx.split.num_core_pmc = max_t(unsigned int,
+						       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 527f18d8cc44..127983ab8307 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4914,9 +4914,14 @@ static __init void svm_set_cpu_caps(void)
 	    boot_cpu_has(X86_FEATURE_AMD_SSBD))
 		kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
 
-	/* AMD PMU PERFCTR_CORE CPUID */
-	if (enable_pmu && boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
-		kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);
+	if (enable_pmu) {
+		/* AMD PMU PERFCTR_CORE CPUID */
+		if (boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
+			kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);
+		/* KVM only support AMD PerfMon V2 */
+		if (kvm_pmu_cap.version > 1)
+			kvm_cpu_cap_set(X86_FEATURE_AMD_PMU_V2);
+	}
 
 	/* CPUID 0x8000001F (SME/SEV features) */
 	sev_set_cpu_caps();
-- 
2.38.1

Re: [PATCH v3 7/8] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> 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   | 30 +++++++++++++++++++++++++++++-
>  arch/x86/kvm/svm/svm.c | 11 ++++++++---
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 6b5912578edd..df551fa66ccc 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1113,7 +1113,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
> @@ -1229,6 +1229,34 @@ 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_AMD_PMU_V2)) {
> +			entry->eax = entry->ebx;
> +			break;
> +		}
> +
> +		cpuid_entry_override(entry, CPUID_8000_0022_EAX);
> +
> +		if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2))
> +			ebx.split.num_core_pmc = min(kvm_pmu_cap.num_counters_gp,
> +						     KVM_AMD_PMC_MAX_GENERIC);

Similar to the previous patch, sanitizing kvm_pmu_cap.num_counters_gp should be
handled during PMU initialization.

> +
> +		if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
> +			ebx.split.num_core_pmc = max_t(unsigned int,
> +						       ebx.split.num_core_pmc,
> +						       AMD64_NUM_COUNTERS_CORE);
> +		else
> +			ebx.split.num_core_pmc = max_t(unsigned int,
> +						       ebx.split.num_core_pmc,
> +						       AMD64_NUM_COUNTERS);

Again like the previous patch, I would expect this to be an if-elif-else sequence.

> +
> +		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 527f18d8cc44..127983ab8307 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4914,9 +4914,14 @@ static __init void svm_set_cpu_caps(void)
>  	    boot_cpu_has(X86_FEATURE_AMD_SSBD))
>  		kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
>  
> -	/* AMD PMU PERFCTR_CORE CPUID */
> -	if (enable_pmu && boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
> -		kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);

Another existing wart.  This existing code can be:

	if (enable_pmu)
		kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE)

Can you add a patch to do that minor cleanup?  Then this patch can yield:

	if (enable_pmu) {
		kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
		kvm_cpu_cap_check_and_set(X86_FEATURE_PERFMON_V2);
	}

KVM's reverse-CPUID magic should Just Work for PERFMON_V2 even though it's scattered.
		

> +	if (enable_pmu) {
> +		/* AMD PMU PERFCTR_CORE CPUID */
> +		if (boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
> +			kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);
> +		/* KVM only support AMD PerfMon V2 */
> +		if (kvm_pmu_cap.version > 1)
> +			kvm_cpu_cap_set(X86_FEATURE_AMD_PMU_V2);
> +	}
>  
>  	/* CPUID 0x8000001F (SME/SEV features) */
>  	sev_set_cpu_caps();
> -- 
> 2.38.1
> 

[PATCH v3 8/8] KVM: x86/cpuid: Use fast return for cpuid "0xa" leaf when !enable_pmu

[Like Xu]

From: Like Xu <likexu@tencent.com>

Although when !enable_pmu, KVM will have zero-padded kvm_pmu_cap to do
subsequent cpuid leaf assignments, one extra branch instruction saves
a few subsequent zero-assignment instructions, speeding things up a bit.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index df551fa66ccc..719290ff6d77 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -922,7 +922,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		union cpuid10_eax eax;
 		union cpuid10_edx edx;
 
-		if (!static_cpu_has(X86_FEATURE_ARCH_PERFMON)) {
+		if (!enable_pmu || !static_cpu_has(X86_FEATURE_ARCH_PERFMON)) {
 			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
 			break;
 		}
-- 
2.38.1

Re: [PATCH v3 8/8] KVM: x86/cpuid: Use fast return for cpuid "0xa" leaf when !enable_pmu

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> Although when !enable_pmu, KVM will have zero-padded kvm_pmu_cap to do
> subsequent cpuid leaf assignments, one extra branch instruction saves
> a few subsequent zero-assignment instructions, speeding things up a bit.

I doubt my motivation was performance, I'm guessing I suggested adding the explicit
!enable_pmu check because relying on kvm_pmu_cap to be zeroed isn't obvious.

Re: [PATCH v3 0/8] KVM: x86: Add AMD Guest PerfMonV2 PMU support

[Like Xu]

Any comments to move it forward ?

On 11/11/2022 6:26 pm, Like Xu wrote:
> Starting with Zen4, core PMU on AMD platforms such as Genoa and
> Ryzen-7000 will support PerfMonV2, and it is also compatible with
> legacy PERFCTR_CORE behavior and msr addresses.
> 
> If you don't have access to the hardware specification, the commits
> d6d0c7f681fd..7685665c390d for host perf can also bring a quick
> overview. Its main change is the addition of three msr's equivalent
> to Intel V2, namely global_ctrl, global_status, global_status_clear.
> 
> It is worth noting that this feature is very attractive for reducing the
> overhead of PMU virtualization, since multiple msr accesses to multiple
> counters will be replaced by a single access to the global register,
> plus more accuracy gain when multiple guest counters are used.
> 
> All related testcases are passed on a Genoa box.
> Please feel free to run more tests, add more or share comments.
> 
> Previous:
> https://lore.kernel.org/kvm/20220919093453.71737-1-likexu@tencent.com/
> 
> V2 -> V3 Changelog:
> - Renme pmc_is_enabled(); (Jim)
> - Move the reprogram_counters() changes as a separate patch; (Jim)
> - Refactoring to align with other set_msr() helper; (Sean)
> - Fix the issue if userspace wants to expose v1 for whatever reason; (Sean)
> - Add the feature flag X86_FEATURE_AMD_PMU_V2; (Sean)
> - Check enable_pmu for intel 0xa as well; (Sean)
> - Limit AMD pmu's KVM support to version 2 as well;
> - Other nit changes around C code taste; (Sean)
> 
> Like Xu (7):
>    KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled()
>    KVM: x86/pmu: Rewrite reprogram_counters() to improve performance
>    KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
>    KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry
>    KVM: x86/svm/pmu: Add AMD PerfMonV2 support
>    KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
>    KVM: x86/cpuid: Use fast return for cpuid "0xa" leaf when !enable_pmu
> 
> Sean Christopherson (1):
>    KVM: VMX: Refactor intel_pmu_set_msr() to align with other set_msr()
>      helpers
> 
>   arch/x86/include/asm/kvm-x86-pmu-ops.h |   1 -
>   arch/x86/include/asm/kvm_host.h        |   1 +
>   arch/x86/kvm/cpuid.c                   |  32 ++++++-
>   arch/x86/kvm/pmu.c                     |  65 +++++++++++++--
>   arch/x86/kvm/pmu.h                     |  28 ++++++-
>   arch/x86/kvm/reverse_cpuid.h           |  10 +++
>   arch/x86/kvm/svm/pmu.c                 |  73 +++++++++++-----
>   arch/x86/kvm/svm/svm.c                 |  11 ++-
>   arch/x86/kvm/vmx/pmu_intel.c           | 111 +++++++------------------
>   arch/x86/kvm/x86.c                     |  14 +++-
>   10 files changed, 228 insertions(+), 118 deletions(-)
> 

Re: [PATCH v3 0/8] KVM: x86: Add AMD Guest PerfMonV2 PMU support

[Sean Christopherson]

On Fri, Nov 11, 2022, Like Xu wrote:
> Starting with Zen4, core PMU on AMD platforms such as Genoa and
> Ryzen-7000 will support PerfMonV2, and it is also compatible with
> legacy PERFCTR_CORE behavior and msr addresses.
> 
> If you don't have access to the hardware specification, the commits
> d6d0c7f681fd..7685665c390d for host perf can also bring a quick
> overview. Its main change is the addition of three msr's equivalent
> to Intel V2, namely global_ctrl, global_status, global_status_clear.
> 
> It is worth noting that this feature is very attractive for reducing the
> overhead of PMU virtualization, since multiple msr accesses to multiple
> counters will be replaced by a single access to the global register,
> plus more accuracy gain when multiple guest counters are used.

Some minor nits, though I haven't looked at the meat of the series yet.  I'll
give this a thorough review early next week (unless I'm extra ambitious tomorrow).