* [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-19 9:34 [PATCH v2 0/3] KVM: x86: Add AMD Guest PerfMonV2 PMU support Like Xu
@ 2022-09-19 9:34 ` Like Xu
2022-09-22 0:20 ` Jim Mattson
2022-10-27 22:10 ` Sean Christopherson
2022-09-19 9:34 ` [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
2022-09-19 9:34 ` [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
2 siblings, 2 replies; 18+ messages in thread
From: Like Xu @ 2022-09-19 9:34 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini; +Cc: Jim Mattson, kvm, linux-kernel
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.
The new non-prefix pmc_is_enabled() works well as legacy AMD vPMU
version is indexeqd 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 | 30 ++++++++++++-
arch/x86/kvm/svm/pmu.c | 9 ----
arch/x86/kvm/vmx/pmu_intel.c | 58 +-------------------------
5 files changed, 80 insertions(+), 73 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..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_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 fabbc43031b3..2643a3994624 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_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);
@@ -455,11 +450,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) {
+ pmu->global_status = data;
+ return 0;
+ }
+ break; /* RO MSR */
+ case MSR_CORE_PERF_GLOBAL_CTRL:
+ if (pmu->global_ctrl == data)
+ return 0;
+ if (kvm_valid_perf_global_ctrl(pmu, 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;
+ }
+ break;
+ 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 847e7112a5d3..0275f1bff5ea 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_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);
@@ -189,6 +188,35 @@ static inline void kvm_pmu_request_counter_reprogam(struct kvm_pmc *pmc)
kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
}
+/*
+ * 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_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);
+}
+
+static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
+{
+ int bit;
+
+ if (!diff)
+ return;
+
+ 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/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index f99f2c869664..3a20972e9f1a 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_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 612c89ef5398..12eb2edfe9bc 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);
@@ -102,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);
@@ -347,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;
@@ -404,29 +372,6 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 0;
}
break;
- case MSR_CORE_PERF_GLOBAL_STATUS:
- if (msr_info->host_initiated) {
- pmu->global_status = data;
- return 0;
- }
- break; /* RO MSR */
- case MSR_CORE_PERF_GLOBAL_CTRL:
- if (pmu->global_ctrl == data)
- return 0;
- if (kvm_valid_perf_global_ctrl(pmu, 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;
- }
- break;
case MSR_IA32_PEBS_ENABLE:
if (pmu->pebs_enable == data)
return 0;
@@ -783,7 +728,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_enabled(pmc) || !pmc->perf_event)
continue;
hw_idx = pmc->perf_event->hw.idx;
@@ -795,7 +740,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_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.37.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-19 9:34 ` [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
@ 2022-09-22 0:20 ` Jim Mattson
2022-09-22 5:47 ` Like Xu
2022-10-27 22:10 ` Sean Christopherson
1 sibling, 1 reply; 18+ messages in thread
From: Jim Mattson @ 2022-09-22 0:20 UTC (permalink / raw)
To: Like Xu; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On Mon, Sep 19, 2022 at 2:35 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> 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.
>
> The new non-prefix pmc_is_enabled() works well as legacy AMD vPMU
> version is indexeqd as 1. Note that the specific *_is_valid_msr will
Nit: indexed
> 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 | 30 ++++++++++++-
> arch/x86/kvm/svm/pmu.c | 9 ----
> arch/x86/kvm/vmx/pmu_intel.c | 58 +-------------------------
> 5 files changed, 80 insertions(+), 73 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..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_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 fabbc43031b3..2643a3994624 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_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);
> @@ -455,11 +450,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) {
> + pmu->global_status = data;
> + return 0;
> + }
> + break; /* RO MSR */
Perhaps 'return 1'?
> + case MSR_CORE_PERF_GLOBAL_CTRL:
> + if (pmu->global_ctrl == data)
> + return 0;
> + if (kvm_valid_perf_global_ctrl(pmu, data)) {
> + diff = pmu->global_ctrl ^ data;
> + pmu->global_ctrl = data;
> + reprogram_counters(pmu, diff);
> + return 0;
> + }
> + break;
Perhaps 'return 1'?
> + 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;
> + }
> + break;
Perhaps 'return 1'?
> + 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 847e7112a5d3..0275f1bff5ea 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_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);
> @@ -189,6 +188,35 @@ static inline void kvm_pmu_request_counter_reprogam(struct kvm_pmc *pmc)
> kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
> }
>
> +/*
> + * 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).
The name of this function is a bit misleading. A PMC can be disabled
either by PERF_CLOBAL_CTRL or by its corresponding EVTSEL.
> + */
> +static inline bool pmc_is_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);
> +}
> +
> +static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
> +{
> + int bit;
> +
> + if (!diff)
> + return;
> +
> + for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX)
> + __set_bit(bit, pmu->reprogram_pmi);
I see that you've dropped the index to pmc conversion and the test for
a valid pmc. Maybe this is okay, but I'm not sure what the caller
looks like, since this is all in global_ctrl_changed() upstream.
What's your diff base?
> +
> + kvm_make_request(KVM_REQ_PMU, pmu_to_vcpu(pmu));
Why this new request? It's not in the Intel-specific version of these
function that you elide below.
Perhaps you could split up the semantic changes from the simple renamings?
> +}
> +
> 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 f99f2c869664..3a20972e9f1a 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_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 612c89ef5398..12eb2edfe9bc 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);
> @@ -102,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);
> @@ -347,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;
> @@ -404,29 +372,6 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> return 0;
> }
> break;
> - case MSR_CORE_PERF_GLOBAL_STATUS:
> - if (msr_info->host_initiated) {
> - pmu->global_status = data;
> - return 0;
> - }
> - break; /* RO MSR */
> - case MSR_CORE_PERF_GLOBAL_CTRL:
> - if (pmu->global_ctrl == data)
> - return 0;
> - if (kvm_valid_perf_global_ctrl(pmu, 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;
> - }
> - break;
> case MSR_IA32_PEBS_ENABLE:
> if (pmu->pebs_enable == data)
> return 0;
> @@ -783,7 +728,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_enabled(pmc) || !pmc->perf_event)
> continue;
>
> hw_idx = pmc->perf_event->hw.idx;
> @@ -795,7 +740,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_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.37.3
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-22 0:20 ` Jim Mattson
@ 2022-09-22 5:47 ` Like Xu
2022-09-22 6:20 ` Like Xu
2022-10-07 22:19 ` Sean Christopherson
0 siblings, 2 replies; 18+ messages in thread
From: Like Xu @ 2022-09-22 5:47 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On 22/9/2022 8:20 am, Jim Mattson wrote:
> On Mon, Sep 19, 2022 at 2:35 AM Like Xu <like.xu.linux@gmail.com> wrote:
>>
>> 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.
>>
>> The new non-prefix pmc_is_enabled() works well as legacy AMD vPMU
>> version is indexeqd as 1. Note that the specific *_is_valid_msr will
> Nit: indexed
>> 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 | 30 ++++++++++++-
>> arch/x86/kvm/svm/pmu.c | 9 ----
>> arch/x86/kvm/vmx/pmu_intel.c | 58 +-------------------------
>> 5 files changed, 80 insertions(+), 73 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..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_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 fabbc43031b3..2643a3994624 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_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);
>> @@ -455,11 +450,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) {
>> + pmu->global_status = data;
>> + return 0;
>> + }
>> + break; /* RO MSR */
> Perhaps 'return 1'?
>> + case MSR_CORE_PERF_GLOBAL_CTRL:
>> + if (pmu->global_ctrl == data)
>> + return 0;
>> + if (kvm_valid_perf_global_ctrl(pmu, data)) {
>> + diff = pmu->global_ctrl ^ data;
>> + pmu->global_ctrl = data;
>> + reprogram_counters(pmu, diff);
>> + return 0;
>> + }
>> + break;
> Perhaps 'return 1'?
>> + 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;
>> + }
>> + break;
> Perhaps 'return 1'?
All above applied.
>> + 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 847e7112a5d3..0275f1bff5ea 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_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);
>> @@ -189,6 +188,35 @@ static inline void kvm_pmu_request_counter_reprogam(struct kvm_pmc *pmc)
>> kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>> }
>>
>> +/*
>> + * 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).
>
> The name of this function is a bit misleading. A PMC can be disabled
> either by PERF_CLOBAL_CTRL or by its corresponding EVTSEL.
The name of this function is indeed very legacy.
How about rename it to pmc_is_enabled_globally() in a separate patch?
>
>> + */
>> +static inline bool pmc_is_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);
>> +}
>> +
>> +static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
>> +{
>> + int bit;
>> +
>> + if (!diff)
>> + return;
>> +
>> + for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX)
>> + __set_bit(bit, pmu->reprogram_pmi);
>
> I see that you've dropped the index to pmc conversion and the test for
> a valid pmc. Maybe this is okay, but I'm not sure what the caller
> looks like, since this is all in global_ctrl_changed() upstream.
> What's your diff base?
As the cover letter says, the diff base is:
https://lore.kernel.org/kvm/20220831085328.45489-1-likexu@tencent.com/.
Based on that, the reprogram_counters() can be reused
when either global_ctrl or pebs_enable is changed.
>
>> +
>> + kvm_make_request(KVM_REQ_PMU, pmu_to_vcpu(pmu));
>
> Why this new request? It's not in the Intel-specific version of these
> function that you elide below.
>
> Perhaps you could split up the semantic changes from the simple renamings?
The creation of perf_event is delayed to the last step,
https://lore.kernel.org/kvm/20220831085328.45489-5-likexu@tencent.com/
Based on the new code base, no semantic changes I assume.
>
>> +}
>> +
>> 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 f99f2c869664..3a20972e9f1a 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_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 612c89ef5398..12eb2edfe9bc 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);
>> @@ -102,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);
>> @@ -347,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;
>> @@ -404,29 +372,6 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> return 0;
>> }
>> break;
>> - case MSR_CORE_PERF_GLOBAL_STATUS:
>> - if (msr_info->host_initiated) {
>> - pmu->global_status = data;
>> - return 0;
>> - }
>> - break; /* RO MSR */
>> - case MSR_CORE_PERF_GLOBAL_CTRL:
>> - if (pmu->global_ctrl == data)
>> - return 0;
>> - if (kvm_valid_perf_global_ctrl(pmu, 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;
>> - }
>> - break;
>> case MSR_IA32_PEBS_ENABLE:
>> if (pmu->pebs_enable == data)
>> return 0;
>> @@ -783,7 +728,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_enabled(pmc) || !pmc->perf_event)
>> continue;
>>
>> hw_idx = pmc->perf_event->hw.idx;
>> @@ -795,7 +740,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_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.37.3
>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-22 5:47 ` Like Xu
@ 2022-09-22 6:20 ` Like Xu
2022-10-27 22:14 ` Sean Christopherson
2022-10-07 22:19 ` Sean Christopherson
1 sibling, 1 reply; 18+ messages in thread
From: Like Xu @ 2022-09-22 6:20 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On 22/9/2022 1:47 pm, Like Xu wrote:
>> Why this new request? It's not in the Intel-specific version of these
>> function that you elide below.
>>
>> Perhaps you could split up the semantic changes from the simple renamings?
>
> The creation of perf_event is delayed to the last step,
> https://lore.kernel.org/kvm/20220831085328.45489-5-likexu@tencent.com/
>
> Based on the new code base, no semantic changes I assume.
Sorry, I think we do need one more clear commit like this:
---
From e08b2d03a652e5ec226d8907c7648bff57f31d3b Mon Sep 17 00:00:00 2001
From: Like Xu <likexu@tencent.com>
Date: Thu, 22 Sep 2022 14:15:18 +0800
Subject: [PATCH] KVM: x86/pmu: Rewrite reprogram_counters() to improve
performance
Before using pmu->reprogram_pmi, the test for a valid pmc is always
applied. This part of the redundancy could be removed by setting the
counters' bitmask directly, and furthermore triggering KVM_REQ_PMU
only once to save more cycles.
Signed-off-by: Like Xu <likexu@tencent.com>
---
arch/x86/kvm/pmu.h | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index fb8040854f4d..9b8e74ccd18a 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -204,15 +204,11 @@ static inline bool pmc_is_enabled_globally(struct kvm_pmc
*pmc)
return test_bit(pmc->idx, (unsigned long *)&pmu->global_ctrl);
}
-static void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
+static inline 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);
+ if (diff) {
+ pmu->reprogram_pmi |= diff;
+ kvm_make_request(KVM_REQ_PMU, pmu_to_vcpu(pmu));
}
}
--
2.37.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-22 6:20 ` Like Xu
@ 2022-10-27 22:14 ` Sean Christopherson
0 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2022-10-27 22:14 UTC (permalink / raw)
To: Like Xu; +Cc: Jim Mattson, Paolo Bonzini, kvm, linux-kernel
On Thu, Sep 22, 2022, Like Xu wrote:
> Subject: [PATCH] KVM: x86/pmu: Rewrite reprogram_counters() to improve
> performance
>
> Before using pmu->reprogram_pmi, the test for a valid pmc is always
> applied. This part of the redundancy could be removed by setting the
> counters' bitmask directly, and furthermore triggering KVM_REQ_PMU
> only once to save more cycles.
Assuming you plan on posting this, please explicitly state what the patch does.
"This part of the redundancy could" only says what _could_ be done, not what is
actually done. Oftentimes, a changelog will say something "could" be done when
talking about alternative solutions. So when I see "could", I think "this is
what the patch _isn't_ doing".
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-22 5:47 ` Like Xu
2022-09-22 6:20 ` Like Xu
@ 2022-10-07 22:19 ` Sean Christopherson
1 sibling, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2022-10-07 22:19 UTC (permalink / raw)
To: Like Xu; +Cc: Jim Mattson, Paolo Bonzini, kvm, linux-kernel
On Thu, Sep 22, 2022, Like Xu wrote:
> On 22/9/2022 8:20 am, Jim Mattson wrote:
> > > 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) {
> > > + pmu->global_status = data;
> > > + return 0;
> > > + }
> > > + break; /* RO MSR */
> > Perhaps 'return 1'?
> > > + case MSR_CORE_PERF_GLOBAL_CTRL:
> > > + if (pmu->global_ctrl == data)
> > > + return 0;
> > > + if (kvm_valid_perf_global_ctrl(pmu, data)) {
> > > + diff = pmu->global_ctrl ^ data;
> > > + pmu->global_ctrl = data;
> > > + reprogram_counters(pmu, diff);
> > > + return 0;
> > > + }
> > > + break;
> > Perhaps 'return 1'?
> > > + case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> > > + if (!(data & pmu->global_ovf_ctrl_mask)) {
> > > + if (!msr_info->host_initiated)
> > > + pmu->global_status &= ~data;
Pre-existing code question: why are writes from host userspace dropped? Is the
intent to avoid clearing the status register when the VM is migrated?
> > > + return 0;
> > > + }
> > > + break;
> > Perhaps 'return 1'?
Assuming the code is inverted, I'd prefer to keep the "break" to be consistent
with the other set_msr() helpers.
> All above applied.
I realize the code is pre-existing, but as opportunistic refactoring this should
be cleaned up to align with pretty much every other set_msr() helper, and with
respect to how the kernel typically handles errors. The preferred pattern is to do:
if (xyz)
return <error>
<commit change>
return <success>
I.e. intel_pmu_set_msr() is backwards, and having "default" statement silently
fallthrough is a bit nasty.
Cases like MSR_PEBS_DATA_CFG have also needlessly copied the "do check iff the
value is changing".
Can you add fold in the below (lightly tested) prep patch to fix intel_pmu_set_msr()?
Then this patch becomes a straight code movement.
--
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 7 Oct 2022 14:40:49 -0700
Subject: [PATCH] KVM: VMX: Refactor intel_pmu_set_msr() to align with other
set_msr() helpers
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 | 78 ++++++++++++++++++------------------
1 file changed, 39 insertions(+), 39 deletions(-)
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 25b70a85bef5..3031baa6742b 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;
+
+ 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:
@@ -443,14 +442,11 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
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)) ||
@@ -463,28 +459,32 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
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;
reprogram_counter(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)
base-commit: e18d6152ff0f41b7f01f9817372022df04e0d354
--
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic
2022-09-19 9:34 ` [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
2022-09-22 0:20 ` Jim Mattson
@ 2022-10-27 22:10 ` Sean Christopherson
1 sibling, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2022-10-27 22:10 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, Jim Mattson, kvm, linux-kernel
On Mon, Sep 19, 2022, Like Xu wrote:
> 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.
>
> The new non-prefix pmc_is_enabled() works well as legacy AMD vPMU
> version is indexeqd as 1. Note that the specific *_is_valid_msr will
> continue to be used to avoid cross-vendor msr access.
Please state what the patch is doing and why. The above provides a small part of
the "why", and alludes to the "what", but never actually states what is being done.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
2022-09-19 9:34 [PATCH v2 0/3] KVM: x86: Add AMD Guest PerfMonV2 PMU support Like Xu
2022-09-19 9:34 ` [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
@ 2022-09-19 9:34 ` Like Xu
2022-09-21 0:06 ` Jim Mattson
2022-10-27 22:47 ` Sean Christopherson
2022-09-19 9:34 ` [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
2 siblings, 2 replies; 18+ messages in thread
From: Like Xu @ 2022-09-19 9:34 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Jim Mattson, kvm, linux-kernel, Sandipan Das
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 | 67 +++++++++++++++++++++++++++------
arch/x86/kvm/x86.c | 14 ++++++-
4 files changed, 74 insertions(+), 14 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 323f2fa46bb4..925d07431155 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -509,6 +509,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 AMD64_NUM_COUNTERS_CORE
+#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 2643a3994624..8348c9ba5b37 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -455,12 +455,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:
@@ -479,12 +482,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) {
pmu->global_status = data;
return 0;
}
break; /* RO MSR */
case MSR_CORE_PERF_GLOBAL_CTRL:
+ case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
if (pmu->global_ctrl == data)
return 0;
if (kvm_valid_perf_global_ctrl(pmu, data)) {
@@ -495,6 +500,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
}
break;
case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
+ case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
if (!(data & pmu->global_ovf_ctrl_mask)) {
if (!msr_info->host_initiated)
pmu->global_status &= ~data;
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 3a20972e9f1a..8a39e9e33b06 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,43 @@ 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;
- else
- pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
+ pmu->version = 1;
+ if (kvm_pmu_cap.version > 1) {
+ pmu->version = min_t(unsigned int, 2, kvm_pmu_cap.version);
+ entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
+ if (entry) {
+ 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 (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 = 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 +226,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
BUILD_BUG_ON(AMD64_NUM_COUNTERS_CORE > KVM_AMD_PMC_MAX_GENERIC);
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 +247,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 1d28d147fc34..34ceae4e2354 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1445,6 +1445,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,
};
@@ -3848,7 +3852,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);
/*
@@ -3951,7 +3958,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.37.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
2022-09-19 9:34 ` [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
@ 2022-09-21 0:06 ` Jim Mattson
2022-10-27 22:47 ` Sean Christopherson
1 sibling, 0 replies; 18+ messages in thread
From: Jim Mattson @ 2022-09-21 0:06 UTC (permalink / raw)
To: Like Xu
Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel, Sandipan Das
On Mon, Sep 19, 2022 at 2:35 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> 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>
Reviewed-by: Jim Mattson <jmattson@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
2022-09-19 9:34 ` [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
2022-09-21 0:06 ` Jim Mattson
@ 2022-10-27 22:47 ` Sean Christopherson
2022-11-09 9:54 ` Like Xu
1 sibling, 1 reply; 18+ messages in thread
From: Sean Christopherson @ 2022-10-27 22:47 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, Jim Mattson, kvm, linux-kernel, Sandipan Das
On Mon, Sep 19, 2022, Like Xu wrote:
> @@ -162,20 +179,43 @@ 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;
> - else
> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
> + pmu->version = 1;
> + if (kvm_pmu_cap.version > 1) {
> + pmu->version = min_t(unsigned int, 2, kvm_pmu_cap.version);
This is wrong, it forces the guest PMU verson to match the max version supported
by KVM. E.g. if userspace wants to expose v1 for whatever reason, pmu->version
will still end up 2+.
> + entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
> + if (entry) {
> + 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);
This is technically wrong, the number of counters is supposed to be valid if and
only if v2 is supported. On a related topic, does KVM explode if userspace
specifies a bogus PMU version on Intel? I don't see any sanity checks there...
With a proper feature flag
pmu->version = 1;
if (kvm_cpu_has(X86_FEATURE_AMD_PMU_V2) &&
guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)) {
pmu->version = 2;
entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
if (entry) {
...
Though technically the "if (entry)" check is unnecesary.
> + }
> + }
> +
> + /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> + if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE)) {
Unnecessary braces.
> + pmu->nr_arch_gp_counters = max_t(unsigned int,
> + pmu->nr_arch_gp_counters,
> + AMD64_NUM_COUNTERS_CORE);
What happens if userspace sets X86_FEATURE_PERFCTR_CORE when its not supported?
E.g. will KVM be coerced into taking a #GP on a non-existent counter?
> + } else {
> + pmu->nr_arch_gp_counters = max_t(unsigned int,
> + pmu->nr_arch_gp_counters,
> + AMD64_NUM_COUNTERS);
> + }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
2022-10-27 22:47 ` Sean Christopherson
@ 2022-11-09 9:54 ` Like Xu
2022-11-09 14:51 ` Sean Christopherson
0 siblings, 1 reply; 18+ messages in thread
From: Like Xu @ 2022-11-09 9:54 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel
On 28/10/2022 6:47 am, Sean Christopherson wrote:
> What happens if userspace sets X86_FEATURE_PERFCTR_CORE when its not supported?
> E.g. will KVM be coerced into taking a #GP on a non-existent counter?
I'm getting a bit tired of this generic issue, what does KVM need to do when the
KVM user space
sets a capability that KVM doesn't support (like cpuid). Should it change the
guest cpuid audibly
or silently ? Should it report an error when the guest uses this unsupported
capability at run time,
or should it just let the KVM report an error when user space setting the cpuid ?
For vPMU, I may prefer to report an error when setting the vpmu capability (via
cpuid or perf_cap).
Please let me know what you think and make it law as an example to others.
Thanks,
Like Xu
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
2022-11-09 9:54 ` Like Xu
@ 2022-11-09 14:51 ` Sean Christopherson
0 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2022-11-09 14:51 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, kvm, linux-kernel
On Wed, Nov 09, 2022, Like Xu wrote:
> On 28/10/2022 6:47 am, Sean Christopherson wrote:
> > What happens if userspace sets X86_FEATURE_PERFCTR_CORE when its not supported?
> > E.g. will KVM be coerced into taking a #GP on a non-existent counter?
>
> I'm getting a bit tired of this generic issue, what does KVM need to do when
> the KVM user space sets a capability that KVM doesn't support (like cpuid).
> Should it change the guest cpuid audibly or silently ? Should it report an
> error when the guest uses this unsupported capability at run time, or should
> it just let the KVM report an error when user space setting the cpuid ?
KVM should do nothing unless the bogus CPUID can be used to trigger unexpected
and/or unwanted behavior in the kernel, which is why I asked if KVM might end up
taking a #GP.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
2022-09-19 9:34 [PATCH v2 0/3] KVM: x86: Add AMD Guest PerfMonV2 PMU support Like Xu
2022-09-19 9:34 ` [PATCH v2 1/3] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
2022-09-19 9:34 ` [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
@ 2022-09-19 9:34 ` Like Xu
2022-09-21 0:02 ` Jim Mattson
2022-10-27 22:37 ` Sean Christopherson
2 siblings, 2 replies; 18+ messages in thread
From: Like Xu @ 2022-09-19 9:34 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Jim Mattson, kvm, linux-kernel, Sandipan Das
From: Sandipan Das <sandipan.das@amd.com>
From: Sandipan Das <sandipan.das@amd.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: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Sandipan Das <sandipan.das@amd.com>
---
arch/x86/include/asm/perf_event.h | 8 ++++++++
arch/x86/kvm/cpuid.c | 32 ++++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index f6fc8dd51ef4..c848f504e467 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -214,6 +214,14 @@ union cpuid_0x80000022_ebx {
unsigned int full;
};
+union cpuid_0x80000022_eax {
+ struct {
+ /* Performance Monitoring Version 2 Supported */
+ unsigned int perfmon_v2:1;
+ } split;
+ unsigned int full;
+};
+
struct x86_pmu_capability {
int version;
int num_counters_gp;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 75dcf7a72605..34ba845c91b7 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1094,7 +1094,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
@@ -1203,6 +1203,36 @@ 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_eax eax;
+ union cpuid_0x80000022_ebx ebx;
+
+ entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+ if (!enable_pmu)
+ break;
+
+ if (kvm_pmu_cap.version > 1) {
+ /* AMD PerfMon is only supported up to V2 in the KVM. */
+ eax.split.perfmon_v2 = 1;
+ 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->eax = eax.full;
+ entry->ebx = ebx.full;
+ break;
+ }
/*Add support for Centaur's CPUID instruction*/
case 0xC0000000:
/*Just support up to 0xC0000004 now*/
--
2.37.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
2022-09-19 9:34 ` [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
@ 2022-09-21 0:02 ` Jim Mattson
2022-10-27 22:37 ` Sean Christopherson
1 sibling, 0 replies; 18+ messages in thread
From: Jim Mattson @ 2022-09-21 0:02 UTC (permalink / raw)
To: Like Xu
Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel, Sandipan Das
On Mon, Sep 19, 2022 at 2:35 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> From: Sandipan Das <sandipan.das@amd.com>
>
> From: Sandipan Das <sandipan.das@amd.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: Like Xu <likexu@tencent.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
2022-09-19 9:34 ` [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
2022-09-21 0:02 ` Jim Mattson
@ 2022-10-27 22:37 ` Sean Christopherson
2022-11-10 9:26 ` Like Xu
1 sibling, 1 reply; 18+ messages in thread
From: Sean Christopherson @ 2022-10-27 22:37 UTC (permalink / raw)
To: Like Xu, g; +Cc: Paolo Bonzini, Jim Mattson, kvm, linux-kernel, Sandipan Das
On Mon, Sep 19, 2022, Like Xu wrote:
> From: Sandipan Das <sandipan.das@amd.com>
>
> From: Sandipan Das <sandipan.das@amd.com>
Duplicate "From:"s.
> CPUID leaf 0x80000022 i.e. ExtPerfMonAndDbg advertises some
> new performance monitoring features for AMD processors.
Wrap changelogs closer to ~75 chars.
> 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: Like Xu <likexu@tencent.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> ---
> arch/x86/include/asm/perf_event.h | 8 ++++++++
> arch/x86/kvm/cpuid.c | 32 ++++++++++++++++++++++++++++++-
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index f6fc8dd51ef4..c848f504e467 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -214,6 +214,14 @@ union cpuid_0x80000022_ebx {
> unsigned int full;
> };
>
> +union cpuid_0x80000022_eax {
> + struct {
> + /* Performance Monitoring Version 2 Supported */
> + unsigned int perfmon_v2:1;
> + } split;
> + unsigned int full;
> +};
I'm not a fan of perf's unions, but I at least understand the value added for
CPUID entries that are a bunch of multi-bit values. However, this leaf appears
to be a pure features leaf. In which case a union just makes life painful.
Please add a CPUID_8000_0022_EAX kvm_only_cpuid_leafs entry (details in link[*]
below) so that KVM can write sane code like
guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)
and cpuid_entry_override() instead of manually filling in information.
where appropriate.
[*] https://lore.kernel.org/all/Y1AQX3RfM+awULlE@google.com
> struct x86_pmu_capability {
> int version;
> int num_counters_gp;
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 75dcf7a72605..34ba845c91b7 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1094,7 +1094,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
> @@ -1203,6 +1203,36 @@ 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_eax eax;
> + union cpuid_0x80000022_ebx ebx;
> +
> + entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> + if (!enable_pmu)
Shouldn't
case 0xa: { /* Architectural Performance Monitoring */
also check enable_pmu instead of X86_FEATURE_ARCH_PERFMON?
> + break;
> +
> + if (kvm_pmu_cap.version > 1) {
> + /* AMD PerfMon is only supported up to V2 in the KVM. */
> + eax.split.perfmon_v2 = 1;
With a proper CPUID_8000_0022_EAX, this becomes:
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);
...
entry->ebx = ebx.full;
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
2022-10-27 22:37 ` Sean Christopherson
@ 2022-11-10 9:26 ` Like Xu
2022-11-10 17:34 ` Sean Christopherson
0 siblings, 1 reply; 18+ messages in thread
From: Like Xu @ 2022-11-10 9:26 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Jim Mattson, kvm, linux-kernel, Sandipan Das
On 28/10/2022 6:37 am, Sean Christopherson wrote:
> On Mon, Sep 19, 2022, Like Xu wrote:
>> From: Sandipan Das <sandipan.das@amd.com>
>>
>> From: Sandipan Das <sandipan.das@amd.com>
>
> Duplicate "From:"s.
>
>> CPUID leaf 0x80000022 i.e. ExtPerfMonAndDbg advertises some
>> new performance monitoring features for AMD processors.
>
> Wrap changelogs closer to ~75 chars.
>
>> 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: Like Xu <likexu@tencent.com>
>> Signed-off-by: Like Xu <likexu@tencent.com>
>> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
>> ---
>> arch/x86/include/asm/perf_event.h | 8 ++++++++
>> arch/x86/kvm/cpuid.c | 32 ++++++++++++++++++++++++++++++-
>> 2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
>> index f6fc8dd51ef4..c848f504e467 100644
>> --- a/arch/x86/include/asm/perf_event.h
>> +++ b/arch/x86/include/asm/perf_event.h
>> @@ -214,6 +214,14 @@ union cpuid_0x80000022_ebx {
>> unsigned int full;
>> };
>>
>> +union cpuid_0x80000022_eax {
>> + struct {
>> + /* Performance Monitoring Version 2 Supported */
>> + unsigned int perfmon_v2:1;
>> + } split;
>> + unsigned int full;
>> +};
>
> I'm not a fan of perf's unions, but I at least understand the value added for
> CPUID entries that are a bunch of multi-bit values. However, this leaf appears
> to be a pure features leaf. In which case a union just makes life painful.
>
> Please add a CPUID_8000_0022_EAX kvm_only_cpuid_leafs entry (details in link[*]
> below) so that KVM can write sane code like
>
> guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)
>
> and cpuid_entry_override() instead of manually filling in information.
>
> where appropriate.
>
> [*] https://lore.kernel.org/all/Y1AQX3RfM+awULlE@google.com
When someone is selling syntactic sugar in the kernel space, extra attention
needs to be paid to runtime performance (union) and memory footprint
(reverse_cpuid).
Applied for this case, while the cpuid_0x80000022_eax will be used again
in the perf core since the other new AMD PMU features are pacing at the door.
>
>> struct x86_pmu_capability {
>> int version;
>> int num_counters_gp;
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 75dcf7a72605..34ba845c91b7 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1094,7 +1094,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
>> @@ -1203,6 +1203,36 @@ 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_eax eax;
>> + union cpuid_0x80000022_ebx ebx;
>> +
>> + entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>> + if (!enable_pmu)
>
> Shouldn't
>
> case 0xa: { /* Architectural Performance Monitoring */
>
> also check enable_pmu instead of X86_FEATURE_ARCH_PERFMON?
Applied as a separate patch, though KVM will have zero-padded kvm_pmu_cap to do
subsequent assignments when !enable_pmu but that doesn't hurt.
>
>> + break;
>> +
>> + if (kvm_pmu_cap.version > 1) {
>> + /* AMD PerfMon is only supported up to V2 in the KVM. */
>> + eax.split.perfmon_v2 = 1;
>
> With a proper CPUID_8000_0022_EAX, this becomes:
>
> 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);
>
> ...
Then in this code block, we will have:
/* AMD PerfMon is only supported up to V2 in the KVM. */
entry->eax |= BIT(0);
to cover AMD Perfmon V3+, any better move ?
>
> entry->ebx = ebx.full;
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
2022-11-10 9:26 ` Like Xu
@ 2022-11-10 17:34 ` Sean Christopherson
0 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2022-11-10 17:34 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, Jim Mattson, kvm, linux-kernel, Sandipan Das
On Thu, Nov 10, 2022, Like Xu wrote:
> On 28/10/2022 6:37 am, Sean Christopherson wrote:
> > I'm not a fan of perf's unions, but I at least understand the value added for
> > CPUID entries that are a bunch of multi-bit values. However, this leaf appears
> > to be a pure features leaf. In which case a union just makes life painful.
> >
> > Please add a CPUID_8000_0022_EAX kvm_only_cpuid_leafs entry (details in link[*]
> > below) so that KVM can write sane code like
> >
> > guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)
> >
> > and cpuid_entry_override() instead of manually filling in information.
> >
> > where appropriate.
> >
> > [*] https://lore.kernel.org/all/Y1AQX3RfM+awULlE@google.com
>
> When someone is selling syntactic sugar in the kernel space, extra attention
> needs to be paid to runtime performance (union) and memory footprint
> (reverse_cpuid).
No. Just no.
First off, this is more than syntactic sugar. KVM has had multiple bugs in the
past due to manually querying/filling CPUID entries. The reverse-CPUID infrastructure
guards against some of those bugs by limiting potential bugs to the leaf definition
and the feature definition. I.e. we only need to get the cpuid_leafs+X86_FEATURE_*
definitions correct.
Second, this code is not remotely performance sensitive, and the memory footprint
of the reverse_cpuid table is laughably small. It's literally 8 bytes per entry
FOR THE ENTIRE KERNEL. And that's ignoring the fact that the table might even be
optimized away entirely since it's really just a switch statement that doesn't
use a helper function.
> > With a proper CPUID_8000_0022_EAX, this becomes:
> >
> > 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);
> >
> > ...
>
> Then in this code block, we will have:
>
> /* AMD PerfMon is only supported up to V2 in the KVM. */
> entry->eax |= BIT(0);
I can't tell exactly what you're suggesting, but if you're implying that you don't
want to add CPUID_8000_0022_EAX, then NAK. Open coding CPUID feature bit
manipulations in KVM is not acceptable.
If I'm misunderstanding and there's something that isn't handled by
cpuid_entry_override(), then the correct way to force a CPUID feature bit is:
cpuid_entry_set(entry, X86_FEATURE_AMD_PMU_V2);
> to cover AMD Perfmon V3+, any better move ?
Huh? If/when V3+ comes along, the above
cpuid_entry_override(entry, CPUID_8000_0022_EAX);
will continue to do the right thing because KVM will (a) advertise V2 if it's
supported in hardware and (b) NOT advertise V3+ because the relevant CPUID bit(s)
will not be set in kvm_cpu_caps until KVM gains the necessary support.
^ permalink raw reply [flat|nested] 18+ messages in thread