* [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
@ 2023-03-21 11:27 Like Xu
2023-03-24 22:46 ` Sean Christopherson
2023-03-24 23:19 ` Jim Mattson
0 siblings, 2 replies; 7+ messages in thread
From: Like Xu @ 2023-03-21 11:27 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel
From: Like Xu <likexu@tencent.com>
Per Intel SDM, fixed-function performance counter 'i' is supported if:
FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
which means that the KVM user space can use EDX to limit the number of
fixed counters and at the same time, using ECX to enable part of other
KVM supported fixed counters.
Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
of the guest available fixed counters and perform the semantic checks.
Signed-off-by: Like Xu <likexu@tencent.com>
---
arch/x86/include/asm/kvm_host.h | 2 ++
arch/x86/kvm/pmu.h | 8 +++++
arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
3 files changed, 44 insertions(+), 19 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a45de1118a42..14689e583127 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -565,6 +565,8 @@ struct kvm_pmu {
*/
bool need_cleanup;
+ DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
+
/*
* The total number of programmed perf_events and it helps to avoid
* redundant check before cleanup if guest don't use vPMU at all.
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index be62c16f2265..9f4504e5e9d5 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -111,6 +111,11 @@ static inline struct kvm_pmc *get_gp_pmc(struct kvm_pmu *pmu, u32 msr,
return NULL;
}
+static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
+{
+ return test_bit(idx, pmu->supported_fixed_pmc_idx);
+}
+
/* returns fixed PMC with the specified MSR */
static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
{
@@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
u32 index = array_index_nospec(msr - base,
pmu->nr_arch_fixed_counters);
+ if (!fixed_ctr_is_supported(pmu, index))
+ return NULL;
+
return &pmu->fixed_counters[index];
}
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index e8a3be0b9df9..12f4b2fe7756 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
{
struct kvm_pmc *pmc;
- u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
+ u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
int i;
pmu->fixed_ctr_ctrl = data;
for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
- u8 new_ctrl = fixed_ctrl_field(data, i);
- u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
+ if (!fixed_ctr_is_supported(pmu, i))
+ continue;
+
+ new_ctrl = fixed_ctrl_field(data, i);
+ old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
if (old_ctrl == new_ctrl)
continue;
@@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
idx &= ~(3u << 30);
+ if (fixed && !fixed_ctr_is_supported(pmu, idx))
+ return false;
+
return fixed ? idx < pmu->nr_arch_fixed_counters
: idx < pmu->nr_arch_gp_counters;
}
@@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
counters = pmu->gp_counters;
num_counters = pmu->nr_arch_gp_counters;
}
- if (idx >= num_counters)
+ if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
return NULL;
*mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
return &counters[array_index_nospec(idx, num_counters)];
@@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
int i;
for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
+ if (!fixed_ctr_is_supported(pmu, i))
+ continue;
+
pmc = &pmu->fixed_counters[i];
event = fixed_pmc_events[array_index_nospec(i, size)];
pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
@@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
pmu->nr_arch_gp_counters = 0;
pmu->nr_arch_fixed_counters = 0;
+ bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
pmu->counter_bitmask[KVM_PMC_GP] = 0;
pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
pmu->version = 0;
@@ -551,13 +561,24 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
pmu->available_event_types = ~entry->ebx &
((1ull << eax.split.mask_length) - 1);
- if (pmu->version == 1) {
- pmu->nr_arch_fixed_counters = 0;
- } else {
+ counter_mask = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
+ bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
+
+ if (pmu->version > 1) {
pmu->nr_arch_fixed_counters =
- min3(ARRAY_SIZE(fixed_pmc_events),
- (size_t) edx.split.num_counters_fixed,
- (size_t)kvm_pmu_cap.num_counters_fixed);
+ min_t(int, ARRAY_SIZE(fixed_pmc_events),
+ kvm_pmu_cap.num_counters_fixed);
+ for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
+ /* FxCtr[i]_is_supported := CPUID.0xA.ECX[i] || (EDX[4:0] > i) */
+ if (!(entry->ecx & BIT_ULL(i) ||
+ edx.split.num_counters_fixed > i))
+ continue;
+
+ set_bit(i, pmu->supported_fixed_pmc_idx);
+ set_bit(INTEL_PMC_MAX_GENERIC + i, pmu->all_valid_pmc_idx);
+ pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
+ counter_mask &= ~BIT_ULL(INTEL_PMC_MAX_GENERIC + i);
+ }
edx.split.bit_width_fixed = min_t(int, edx.split.bit_width_fixed,
kvm_pmu_cap.bit_width_fixed);
pmu->counter_bitmask[KVM_PMC_FIXED] =
@@ -565,10 +586,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
setup_fixed_pmc_eventsel(pmu);
}
- for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
- pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
- counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
- (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
pmu->global_ctrl_mask = counter_mask;
pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask
& ~(MSR_CORE_PERF_GLOBAL_OVF_CTRL_OVF_BUF |
@@ -585,11 +602,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
pmu->raw_event_mask |= (HSW_IN_TX|HSW_IN_TX_CHECKPOINTED);
}
- bitmap_set(pmu->all_valid_pmc_idx,
- 0, pmu->nr_arch_gp_counters);
- bitmap_set(pmu->all_valid_pmc_idx,
- INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
-
perf_capabilities = vcpu_get_perf_capabilities(vcpu);
if (cpuid_model_is_consistent(vcpu) &&
(perf_capabilities & PMU_CAP_LBR_FMT))
@@ -605,6 +617,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
pmu->pebs_enable_mask = counter_mask;
pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
+ if (!fixed_ctr_is_supported(pmu, i))
+ continue;
+
pmu->fixed_ctr_ctrl_mask &=
~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
}
base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
--
2.40.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
2023-03-21 11:27 [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask Like Xu
@ 2023-03-24 22:46 ` Sean Christopherson
2023-03-24 23:19 ` Jim Mattson
1 sibling, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2023-03-24 22:46 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, kvm, linux-kernel
On Tue, Mar 21, 2023, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
>
> Per Intel SDM, fixed-function performance counter 'i' is supported if:
>
> FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
>
> which means that the KVM user space can use EDX to limit the number of
> fixed counters and at the same time, using ECX to enable part of other
> KVM supported fixed counters.
>
> Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
> of the guest available fixed counters and perform the semantic checks.
>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
> arch/x86/include/asm/kvm_host.h | 2 ++
> arch/x86/kvm/pmu.h | 8 +++++
> arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
> 3 files changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a45de1118a42..14689e583127 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -565,6 +565,8 @@ struct kvm_pmu {
> */
> bool need_cleanup;
>
> + DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
Why add a new bitmap? all_valid_pmc_idx is a strict superset, no?
> +static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
> +{
> + return test_bit(idx, pmu->supported_fixed_pmc_idx);
> +}
> +
> /* returns fixed PMC with the specified MSR */
> static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> {
> @@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> u32 index = array_index_nospec(msr - base,
> pmu->nr_arch_fixed_counters);
>
> + if (!fixed_ctr_is_supported(pmu, index))
> + return NULL;
> +
> return &pmu->fixed_counters[index];
> }
>
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index e8a3be0b9df9..12f4b2fe7756 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
> static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
> {
> struct kvm_pmc *pmc;
> - u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> + u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> int i;
>
> pmu->fixed_ctr_ctrl = data;
> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> - u8 new_ctrl = fixed_ctrl_field(data, i);
> - u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
Please keep the variable declarations in the loop (I've had to eat my words about
variable shadowing), e.g. to prevent accessing new_ctrl or old_ctrl after the loop.
> + if (!fixed_ctr_is_supported(pmu, i))
> + continue;
> +
> + new_ctrl = fixed_ctrl_field(data, i);
> + old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>
> if (old_ctrl == new_ctrl)
> continue;
> @@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
>
> idx &= ~(3u << 30);
>
> + if (fixed && !fixed_ctr_is_supported(pmu, idx))
> + return false;
> +
> return fixed ? idx < pmu->nr_arch_fixed_counters
> : idx < pmu->nr_arch_gp_counters;
> }
> @@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
> counters = pmu->gp_counters;
> num_counters = pmu->nr_arch_gp_counters;
> }
> - if (idx >= num_counters)
> + if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
> return NULL;
> *mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
> return &counters[array_index_nospec(idx, num_counters)];
> @@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> int i;
>
> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> + if (!fixed_ctr_is_supported(pmu, i))
> + continue;
> +
> pmc = &pmu->fixed_counters[i];
> event = fixed_pmc_events[array_index_nospec(i, size)];
> pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
> @@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>
> pmu->nr_arch_gp_counters = 0;
> pmu->nr_arch_fixed_counters = 0;
> + bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
Side topic, isn't KVM missing a bitmap_zero() on all_valid_pmc_idx?
> pmu->counter_bitmask[KVM_PMC_GP] = 0;
> pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
> pmu->version = 0;
...
> + if (pmu->version > 1) {
> pmu->nr_arch_fixed_counters =
> - min3(ARRAY_SIZE(fixed_pmc_events),
> - (size_t) edx.split.num_counters_fixed,
> - (size_t)kvm_pmu_cap.num_counters_fixed);
> + min_t(int, ARRAY_SIZE(fixed_pmc_events),
> + kvm_pmu_cap.num_counters_fixed);
Leaving nr_arch_fixed_counters set to kvm_pmu_cap.num_counters_fixed seems odd.
E.g. if userspace reduces the number of counters to 1, shouldn't that be reflected
in the PMU metadata?
Ah, you did that so the iteration works. That's super confusing. And silly,
because in the end, pmu->nr_arch_fixed_counters is just kvm_pmu_cap.num_counters_fixed.
THis holds true:
BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);
and KVM does this
kvm_pmu_cap.num_counters_fixed = min(kvm_pmu_cap.num_counters_fixed,
KVM_PMC_MAX_FIXED);
ergo the above min_t() is a nop.
One option would be to just use kvm_pmu_cap.num_counters_fixed, but I think it
makes more sense to use for_each_set_bit(), or perhaps for_each_set_bitrange_from()
if all_valid_pmc_idx is used.
And then end up with something like so:
static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
bool fixed = idx & (1u << 30);
idx &= ~(3u << 30);
return fixed ? fixed_ctr_is_supported(pmu, idx)
: idx < pmu->nr_arch_gp_counters;
}
static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
unsigned int idx, u64 *mask)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
bool fixed = idx & (1u << 30);
struct kvm_pmc *counters;
unsigned int num_counters;
if (!intel_is_valid_rdpmc_ecx(vcpu, idx))
return NULL;
idx &= ~(3u << 30);
if (fixed) {
counters = pmu->fixed_counters;
num_counters = kvm_pmu_cap.num_counters_fixed;
} else {
counters = pmu->gp_counters;
num_counters = pmu->nr_arch_gp_counters;
}
*mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
return &counters[array_index_nospec(idx, num_counters)];
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
2023-03-21 11:27 [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask Like Xu
2023-03-24 22:46 ` Sean Christopherson
@ 2023-03-24 23:19 ` Jim Mattson
2023-03-27 7:47 ` Like Xu
1 sibling, 1 reply; 7+ messages in thread
From: Jim Mattson @ 2023-03-24 23:19 UTC (permalink / raw)
To: Like Xu; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On Tue, Mar 21, 2023 at 4:28 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> From: Like Xu <likexu@tencent.com>
>
> Per Intel SDM, fixed-function performance counter 'i' is supported if:
>
> FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
>
> which means that the KVM user space can use EDX to limit the number of
> fixed counters and at the same time, using ECX to enable part of other
> KVM supported fixed counters.
>
> Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
> of the guest available fixed counters and perform the semantic checks.
>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
> arch/x86/include/asm/kvm_host.h | 2 ++
> arch/x86/kvm/pmu.h | 8 +++++
> arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
> 3 files changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a45de1118a42..14689e583127 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -565,6 +565,8 @@ struct kvm_pmu {
> */
> bool need_cleanup;
>
> + DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
> +
> /*
> * The total number of programmed perf_events and it helps to avoid
> * redundant check before cleanup if guest don't use vPMU at all.
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index be62c16f2265..9f4504e5e9d5 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -111,6 +111,11 @@ static inline struct kvm_pmc *get_gp_pmc(struct kvm_pmu *pmu, u32 msr,
> return NULL;
> }
>
> +static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
> +{
> + return test_bit(idx, pmu->supported_fixed_pmc_idx);
> +}
> +
> /* returns fixed PMC with the specified MSR */
> static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> {
> @@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> u32 index = array_index_nospec(msr - base,
> pmu->nr_arch_fixed_counters);
>
> + if (!fixed_ctr_is_supported(pmu, index))
> + return NULL;
> +
> return &pmu->fixed_counters[index];
> }
>
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index e8a3be0b9df9..12f4b2fe7756 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
> static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
> {
> struct kvm_pmc *pmc;
> - u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> + u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> int i;
>
> pmu->fixed_ctr_ctrl = data;
> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> - u8 new_ctrl = fixed_ctrl_field(data, i);
> - u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
> + if (!fixed_ctr_is_supported(pmu, i))
> + continue;
> +
> + new_ctrl = fixed_ctrl_field(data, i);
> + old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>
> if (old_ctrl == new_ctrl)
> continue;
> @@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
>
> idx &= ~(3u << 30);
>
> + if (fixed && !fixed_ctr_is_supported(pmu, idx))
> + return false;
> +
> return fixed ? idx < pmu->nr_arch_fixed_counters
> : idx < pmu->nr_arch_gp_counters;
> }
> @@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
> counters = pmu->gp_counters;
> num_counters = pmu->nr_arch_gp_counters;
> }
> - if (idx >= num_counters)
> + if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
> return NULL;
> *mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
> return &counters[array_index_nospec(idx, num_counters)];
> @@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> int i;
>
> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> + if (!fixed_ctr_is_supported(pmu, i))
> + continue;
> +
> pmc = &pmu->fixed_counters[i];
> event = fixed_pmc_events[array_index_nospec(i, size)];
> pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
> @@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>
> pmu->nr_arch_gp_counters = 0;
> pmu->nr_arch_fixed_counters = 0;
> + bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
> pmu->counter_bitmask[KVM_PMC_GP] = 0;
> pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
> pmu->version = 0;
> @@ -551,13 +561,24 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> pmu->available_event_types = ~entry->ebx &
> ((1ull << eax.split.mask_length) - 1);
>
> - if (pmu->version == 1) {
> - pmu->nr_arch_fixed_counters = 0;
> - } else {
> + counter_mask = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
> + bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
> +
> + if (pmu->version > 1) {
> pmu->nr_arch_fixed_counters =
> - min3(ARRAY_SIZE(fixed_pmc_events),
> - (size_t) edx.split.num_counters_fixed,
> - (size_t)kvm_pmu_cap.num_counters_fixed);
> + min_t(int, ARRAY_SIZE(fixed_pmc_events),
> + kvm_pmu_cap.num_counters_fixed);
> + for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> + /* FxCtr[i]_is_supported := CPUID.0xA.ECX[i] || (EDX[4:0] > i) */
This is true only when pmu->version >= 5.
From the SDM, volume 3, section 20.2.5 Architectural Performance
Monitoring Version 5:
With Architectural Performance Monitoring Version 5, register
CPUID.0AH.ECX indicates Fixed Counter enumeration. It is a bit mask
which enumerates the supported Fixed Counters in a processor. If bit
'i' is set, it implies that Fixed Counter 'i' is supported. Software
is recommended to use the following logic to check if a Fixed Counter
is supported on a given processor: FxCtr[i]_is_supported := ECX[i] ||
(EDX[4:0] > i);
Prior to PMU version 5, all fixed counters from 0 through <number of
fixed counters - 1> are supported.
> + if (!(entry->ecx & BIT_ULL(i) ||
> + edx.split.num_counters_fixed > i))
> + continue;
> +
> + set_bit(i, pmu->supported_fixed_pmc_idx);
> + set_bit(INTEL_PMC_MAX_GENERIC + i, pmu->all_valid_pmc_idx);
> + pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
> + counter_mask &= ~BIT_ULL(INTEL_PMC_MAX_GENERIC + i);
> + }
> edx.split.bit_width_fixed = min_t(int, edx.split.bit_width_fixed,
> kvm_pmu_cap.bit_width_fixed);
> pmu->counter_bitmask[KVM_PMC_FIXED] =
> @@ -565,10 +586,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> setup_fixed_pmc_eventsel(pmu);
> }
>
> - for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
> - pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
> - counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
> - (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
> pmu->global_ctrl_mask = counter_mask;
> pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask
> & ~(MSR_CORE_PERF_GLOBAL_OVF_CTRL_OVF_BUF |
> @@ -585,11 +602,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> pmu->raw_event_mask |= (HSW_IN_TX|HSW_IN_TX_CHECKPOINTED);
> }
>
> - bitmap_set(pmu->all_valid_pmc_idx,
> - 0, pmu->nr_arch_gp_counters);
> - bitmap_set(pmu->all_valid_pmc_idx,
> - INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
> -
> perf_capabilities = vcpu_get_perf_capabilities(vcpu);
> if (cpuid_model_is_consistent(vcpu) &&
> (perf_capabilities & PMU_CAP_LBR_FMT))
> @@ -605,6 +617,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> pmu->pebs_enable_mask = counter_mask;
> pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> + if (!fixed_ctr_is_supported(pmu, i))
> + continue;
> +
> pmu->fixed_ctr_ctrl_mask &=
> ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
> }
>
> base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
> --
> 2.40.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
2023-03-24 23:19 ` Jim Mattson
@ 2023-03-27 7:47 ` Like Xu
2023-03-27 14:58 ` Jim Mattson
0 siblings, 1 reply; 7+ messages in thread
From: Like Xu @ 2023-03-27 7:47 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On 25/3/2023 7:19 am, Jim Mattson wrote:
> On Tue, Mar 21, 2023 at 4:28 AM Like Xu <like.xu.linux@gmail.com> wrote:
>>
>> From: Like Xu <likexu@tencent.com>
>>
>> Per Intel SDM, fixed-function performance counter 'i' is supported if:
>>
>> FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
>>
>> which means that the KVM user space can use EDX to limit the number of
>> fixed counters and at the same time, using ECX to enable part of other
>> KVM supported fixed counters.
>>
>> Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
>> of the guest available fixed counters and perform the semantic checks.
>>
>> Signed-off-by: Like Xu <likexu@tencent.com>
>> ---
>> arch/x86/include/asm/kvm_host.h | 2 ++
>> arch/x86/kvm/pmu.h | 8 +++++
>> arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
>> 3 files changed, 44 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index a45de1118a42..14689e583127 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -565,6 +565,8 @@ struct kvm_pmu {
>> */
>> bool need_cleanup;
>>
>> + DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
>> +
>> /*
>> * The total number of programmed perf_events and it helps to avoid
>> * redundant check before cleanup if guest don't use vPMU at all.
>> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
>> index be62c16f2265..9f4504e5e9d5 100644
>> --- a/arch/x86/kvm/pmu.h
>> +++ b/arch/x86/kvm/pmu.h
>> @@ -111,6 +111,11 @@ static inline struct kvm_pmc *get_gp_pmc(struct kvm_pmu *pmu, u32 msr,
>> return NULL;
>> }
>>
>> +static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
>> +{
>> + return test_bit(idx, pmu->supported_fixed_pmc_idx);
>> +}
>> +
>> /* returns fixed PMC with the specified MSR */
>> static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>> {
>> @@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>> u32 index = array_index_nospec(msr - base,
>> pmu->nr_arch_fixed_counters);
>>
>> + if (!fixed_ctr_is_supported(pmu, index))
>> + return NULL;
>> +
>> return &pmu->fixed_counters[index];
>> }
>>
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index e8a3be0b9df9..12f4b2fe7756 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
>> static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>> {
>> struct kvm_pmc *pmc;
>> - u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>> + u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>> int i;
>>
>> pmu->fixed_ctr_ctrl = data;
>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>> - u8 new_ctrl = fixed_ctrl_field(data, i);
>> - u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>> + if (!fixed_ctr_is_supported(pmu, i))
>> + continue;
>> +
>> + new_ctrl = fixed_ctrl_field(data, i);
>> + old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>>
>> if (old_ctrl == new_ctrl)
>> continue;
>> @@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
>>
>> idx &= ~(3u << 30);
>>
>> + if (fixed && !fixed_ctr_is_supported(pmu, idx))
>> + return false;
>> +
>> return fixed ? idx < pmu->nr_arch_fixed_counters
>> : idx < pmu->nr_arch_gp_counters;
>> }
>> @@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
>> counters = pmu->gp_counters;
>> num_counters = pmu->nr_arch_gp_counters;
>> }
>> - if (idx >= num_counters)
>> + if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
>> return NULL;
>> *mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
>> return &counters[array_index_nospec(idx, num_counters)];
>> @@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
>> int i;
>>
>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>> + if (!fixed_ctr_is_supported(pmu, i))
>> + continue;
>> +
>> pmc = &pmu->fixed_counters[i];
>> event = fixed_pmc_events[array_index_nospec(i, size)];
>> pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
>> @@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>
>> pmu->nr_arch_gp_counters = 0;
>> pmu->nr_arch_fixed_counters = 0;
>> + bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
>> pmu->counter_bitmask[KVM_PMC_GP] = 0;
>> pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
>> pmu->version = 0;
>> @@ -551,13 +561,24 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>> pmu->available_event_types = ~entry->ebx &
>> ((1ull << eax.split.mask_length) - 1);
>>
>> - if (pmu->version == 1) {
>> - pmu->nr_arch_fixed_counters = 0;
>> - } else {
>> + counter_mask = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
>> + bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
>> +
>> + if (pmu->version > 1) {
>> pmu->nr_arch_fixed_counters =
>> - min3(ARRAY_SIZE(fixed_pmc_events),
>> - (size_t) edx.split.num_counters_fixed,
>> - (size_t)kvm_pmu_cap.num_counters_fixed);
>> + min_t(int, ARRAY_SIZE(fixed_pmc_events),
>> + kvm_pmu_cap.num_counters_fixed);
>> + for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>> + /* FxCtr[i]_is_supported := CPUID.0xA.ECX[i] || (EDX[4:0] > i) */
>
> This is true only when pmu->version >= 5.
This is true in for "Version 5" section, but not mentioned in the CPUID.0xA section.
I would argue that this is a deliberate omission for the instruction implementation,
as it does use the word "version>1" in the near CPUID.0xA.EDX section.
For virtualised use, this feature offers a kind of flexibility as users can
enable part of
the fixed counters, don't you think? Or maybe you're more looking forward to the
patch set that raises the vPMU version number from 2 to 5, that part of the code
was already in my tree some years ago.
>
> From the SDM, volume 3, section 20.2.5 Architectural Performance
> Monitoring Version 5:
>
> With Architectural Performance Monitoring Version 5, register
> CPUID.0AH.ECX indicates Fixed Counter enumeration. It is a bit mask
> which enumerates the supported Fixed Counters in a processor. If bit
> 'i' is set, it implies that Fixed Counter 'i' is supported. Software
> is recommended to use the following logic to check if a Fixed Counter
> is supported on a given processor: FxCtr[i]_is_supported := ECX[i] ||
> (EDX[4:0] > i);
>
> Prior to PMU version 5, all fixed counters from 0 through <number of
> fixed counters - 1> are supported.
>
>> + if (!(entry->ecx & BIT_ULL(i) ||
>> + edx.split.num_counters_fixed > i))
>> + continue;
>> +
>> + set_bit(i, pmu->supported_fixed_pmc_idx);
>> + set_bit(INTEL_PMC_MAX_GENERIC + i, pmu->all_valid_pmc_idx);
>> + pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
>> + counter_mask &= ~BIT_ULL(INTEL_PMC_MAX_GENERIC + i);
>> + }
>> edx.split.bit_width_fixed = min_t(int, edx.split.bit_width_fixed,
>> kvm_pmu_cap.bit_width_fixed);
>> pmu->counter_bitmask[KVM_PMC_FIXED] =
>> @@ -565,10 +586,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>> setup_fixed_pmc_eventsel(pmu);
>> }
>>
>> - for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
>> - pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
>> - counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
>> - (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
>> pmu->global_ctrl_mask = counter_mask;
>> pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask
>> & ~(MSR_CORE_PERF_GLOBAL_OVF_CTRL_OVF_BUF |
>> @@ -585,11 +602,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>> pmu->raw_event_mask |= (HSW_IN_TX|HSW_IN_TX_CHECKPOINTED);
>> }
>>
>> - bitmap_set(pmu->all_valid_pmc_idx,
>> - 0, pmu->nr_arch_gp_counters);
>> - bitmap_set(pmu->all_valid_pmc_idx,
>> - INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
>> -
>> perf_capabilities = vcpu_get_perf_capabilities(vcpu);
>> if (cpuid_model_is_consistent(vcpu) &&
>> (perf_capabilities & PMU_CAP_LBR_FMT))
>> @@ -605,6 +617,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>> pmu->pebs_enable_mask = counter_mask;
>> pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>> + if (!fixed_ctr_is_supported(pmu, i))
>> + continue;
>> +
>> pmu->fixed_ctr_ctrl_mask &=
>> ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
>> }
>>
>> base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
>> --
>> 2.40.0
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
2023-03-27 7:47 ` Like Xu
@ 2023-03-27 14:58 ` Jim Mattson
2023-03-28 10:27 ` Like Xu
0 siblings, 1 reply; 7+ messages in thread
From: Jim Mattson @ 2023-03-27 14:58 UTC (permalink / raw)
To: Like Xu; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On Mon, Mar 27, 2023 at 12:47 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> On 25/3/2023 7:19 am, Jim Mattson wrote:
> > On Tue, Mar 21, 2023 at 4:28 AM Like Xu <like.xu.linux@gmail.com> wrote:
> >>
> >> From: Like Xu <likexu@tencent.com>
> >>
> >> Per Intel SDM, fixed-function performance counter 'i' is supported if:
> >>
> >> FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
> >>
> >> which means that the KVM user space can use EDX to limit the number of
> >> fixed counters and at the same time, using ECX to enable part of other
> >> KVM supported fixed counters.
> >>
> >> Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
> >> of the guest available fixed counters and perform the semantic checks.
> >>
> >> Signed-off-by: Like Xu <likexu@tencent.com>
> >> ---
> >> arch/x86/include/asm/kvm_host.h | 2 ++
> >> arch/x86/kvm/pmu.h | 8 +++++
> >> arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
> >> 3 files changed, 44 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >> index a45de1118a42..14689e583127 100644
> >> --- a/arch/x86/include/asm/kvm_host.h
> >> +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -565,6 +565,8 @@ struct kvm_pmu {
> >> */
> >> bool need_cleanup;
> >>
> >> + DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
> >> +
> >> /*
> >> * The total number of programmed perf_events and it helps to avoid
> >> * redundant check before cleanup if guest don't use vPMU at all.
> >> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> >> index be62c16f2265..9f4504e5e9d5 100644
> >> --- a/arch/x86/kvm/pmu.h
> >> +++ b/arch/x86/kvm/pmu.h
> >> @@ -111,6 +111,11 @@ static inline struct kvm_pmc *get_gp_pmc(struct kvm_pmu *pmu, u32 msr,
> >> return NULL;
> >> }
> >>
> >> +static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
> >> +{
> >> + return test_bit(idx, pmu->supported_fixed_pmc_idx);
> >> +}
> >> +
> >> /* returns fixed PMC with the specified MSR */
> >> static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> >> {
> >> @@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> >> u32 index = array_index_nospec(msr - base,
> >> pmu->nr_arch_fixed_counters);
> >>
> >> + if (!fixed_ctr_is_supported(pmu, index))
> >> + return NULL;
> >> +
> >> return &pmu->fixed_counters[index];
> >> }
> >>
> >> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> >> index e8a3be0b9df9..12f4b2fe7756 100644
> >> --- a/arch/x86/kvm/vmx/pmu_intel.c
> >> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> >> @@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
> >> static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
> >> {
> >> struct kvm_pmc *pmc;
> >> - u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> >> + u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> >> int i;
> >>
> >> pmu->fixed_ctr_ctrl = data;
> >> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >> - u8 new_ctrl = fixed_ctrl_field(data, i);
> >> - u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
> >> + if (!fixed_ctr_is_supported(pmu, i))
> >> + continue;
> >> +
> >> + new_ctrl = fixed_ctrl_field(data, i);
> >> + old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
> >>
> >> if (old_ctrl == new_ctrl)
> >> continue;
> >> @@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
> >>
> >> idx &= ~(3u << 30);
> >>
> >> + if (fixed && !fixed_ctr_is_supported(pmu, idx))
> >> + return false;
> >> +
> >> return fixed ? idx < pmu->nr_arch_fixed_counters
> >> : idx < pmu->nr_arch_gp_counters;
> >> }
> >> @@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
> >> counters = pmu->gp_counters;
> >> num_counters = pmu->nr_arch_gp_counters;
> >> }
> >> - if (idx >= num_counters)
> >> + if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
> >> return NULL;
> >> *mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
> >> return &counters[array_index_nospec(idx, num_counters)];
> >> @@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> >> int i;
> >>
> >> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >> + if (!fixed_ctr_is_supported(pmu, i))
> >> + continue;
> >> +
> >> pmc = &pmu->fixed_counters[i];
> >> event = fixed_pmc_events[array_index_nospec(i, size)];
> >> pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
> >> @@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >>
> >> pmu->nr_arch_gp_counters = 0;
> >> pmu->nr_arch_fixed_counters = 0;
> >> + bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
> >> pmu->counter_bitmask[KVM_PMC_GP] = 0;
> >> pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
> >> pmu->version = 0;
> >> @@ -551,13 +561,24 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >> pmu->available_event_types = ~entry->ebx &
> >> ((1ull << eax.split.mask_length) - 1);
> >>
> >> - if (pmu->version == 1) {
> >> - pmu->nr_arch_fixed_counters = 0;
> >> - } else {
> >> + counter_mask = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
> >> + bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
> >> +
> >> + if (pmu->version > 1) {
> >> pmu->nr_arch_fixed_counters =
> >> - min3(ARRAY_SIZE(fixed_pmc_events),
> >> - (size_t) edx.split.num_counters_fixed,
> >> - (size_t)kvm_pmu_cap.num_counters_fixed);
> >> + min_t(int, ARRAY_SIZE(fixed_pmc_events),
> >> + kvm_pmu_cap.num_counters_fixed);
> >> + for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >> + /* FxCtr[i]_is_supported := CPUID.0xA.ECX[i] || (EDX[4:0] > i) */
> >
> > This is true only when pmu->version >= 5.
>
> This is true in for "Version 5" section, but not mentioned in the CPUID.0xA section.
> I would argue that this is a deliberate omission for the instruction implementation,
> as it does use the word "version>1" in the near CPUID.0xA.EDX section.
Do you have any evidence to support such an argument? The CPUID field
in question was not defined prior to PMU version 5. (Does anyone from
Intel want to chime in?)
> For virtualised use, this feature offers a kind of flexibility as users can
> enable part of
> the fixed counters, don't you think? Or maybe you're more looking forward to the
> patch set that raises the vPMU version number from 2 to 5, that part of the code
> was already in my tree some years ago.
I would not be surprised if a guest OS checked for PMU version 5
before consulting the CPUID fields defined in PMU version 5. Does
Linux even consult the fixed counter bitmask field today?
I'd love to see KVM virtualize PMU version 5!
> >
> > From the SDM, volume 3, section 20.2.5 Architectural Performance
> > Monitoring Version 5:
> >
> > With Architectural Performance Monitoring Version 5, register
> > CPUID.0AH.ECX indicates Fixed Counter enumeration. It is a bit mask
> > which enumerates the supported Fixed Counters in a processor. If bit
> > 'i' is set, it implies that Fixed Counter 'i' is supported. Software
> > is recommended to use the following logic to check if a Fixed Counter
> > is supported on a given processor: FxCtr[i]_is_supported := ECX[i] ||
> > (EDX[4:0] > i);
> >
> > Prior to PMU version 5, all fixed counters from 0 through <number of
> > fixed counters - 1> are supported.
> >
> >> + if (!(entry->ecx & BIT_ULL(i) ||
> >> + edx.split.num_counters_fixed > i))
> >> + continue;
> >> +
> >> + set_bit(i, pmu->supported_fixed_pmc_idx);
> >> + set_bit(INTEL_PMC_MAX_GENERIC + i, pmu->all_valid_pmc_idx);
> >> + pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
> >> + counter_mask &= ~BIT_ULL(INTEL_PMC_MAX_GENERIC + i);
> >> + }
> >> edx.split.bit_width_fixed = min_t(int, edx.split.bit_width_fixed,
> >> kvm_pmu_cap.bit_width_fixed);
> >> pmu->counter_bitmask[KVM_PMC_FIXED] =
> >> @@ -565,10 +586,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >> setup_fixed_pmc_eventsel(pmu);
> >> }
> >>
> >> - for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
> >> - pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
> >> - counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
> >> - (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
> >> pmu->global_ctrl_mask = counter_mask;
> >> pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask
> >> & ~(MSR_CORE_PERF_GLOBAL_OVF_CTRL_OVF_BUF |
> >> @@ -585,11 +602,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >> pmu->raw_event_mask |= (HSW_IN_TX|HSW_IN_TX_CHECKPOINTED);
> >> }
> >>
> >> - bitmap_set(pmu->all_valid_pmc_idx,
> >> - 0, pmu->nr_arch_gp_counters);
> >> - bitmap_set(pmu->all_valid_pmc_idx,
> >> - INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
> >> -
> >> perf_capabilities = vcpu_get_perf_capabilities(vcpu);
> >> if (cpuid_model_is_consistent(vcpu) &&
> >> (perf_capabilities & PMU_CAP_LBR_FMT))
> >> @@ -605,6 +617,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >> pmu->pebs_enable_mask = counter_mask;
> >> pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
> >> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >> + if (!fixed_ctr_is_supported(pmu, i))
> >> + continue;
> >> +
> >> pmu->fixed_ctr_ctrl_mask &=
> >> ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
> >> }
> >>
> >> base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
> >> --
> >> 2.40.0
> >>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
2023-03-27 14:58 ` Jim Mattson
@ 2023-03-28 10:27 ` Like Xu
2023-03-28 16:38 ` Jim Mattson
0 siblings, 1 reply; 7+ messages in thread
From: Like Xu @ 2023-03-28 10:27 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On 27/3/2023 10:58 pm, Jim Mattson wrote:
> On Mon, Mar 27, 2023 at 12:47 AM Like Xu <like.xu.linux@gmail.com> wrote:
>>
>> On 25/3/2023 7:19 am, Jim Mattson wrote:
>>> On Tue, Mar 21, 2023 at 4:28 AM Like Xu <like.xu.linux@gmail.com> wrote:
>>>>
>>>> From: Like Xu <likexu@tencent.com>
>>>>
>>>> Per Intel SDM, fixed-function performance counter 'i' is supported if:
>>>>
>>>> FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
>>>>
>>>> which means that the KVM user space can use EDX to limit the number of
>>>> fixed counters and at the same time, using ECX to enable part of other
>>>> KVM supported fixed counters.
>>>>
>>>> Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
>>>> of the guest available fixed counters and perform the semantic checks.
>>>>
>>>> Signed-off-by: Like Xu <likexu@tencent.com>
>>>> ---
>>>> arch/x86/include/asm/kvm_host.h | 2 ++
>>>> arch/x86/kvm/pmu.h | 8 +++++
>>>> arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
>>>> 3 files changed, 44 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>>> index a45de1118a42..14689e583127 100644
>>>> --- a/arch/x86/include/asm/kvm_host.h
>>>> +++ b/arch/x86/include/asm/kvm_host.h
>>>> @@ -565,6 +565,8 @@ struct kvm_pmu {
>>>> */
>>>> bool need_cleanup;
>>>>
>>>> + DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
>>>> +
>>>> /*
>>>> * The total number of programmed perf_events and it helps to avoid
>>>> * redundant check before cleanup if guest don't use vPMU at all.
>>>> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
>>>> index be62c16f2265..9f4504e5e9d5 100644
>>>> --- a/arch/x86/kvm/pmu.h
>>>> +++ b/arch/x86/kvm/pmu.h
>>>> @@ -111,6 +111,11 @@ static inline struct kvm_pmc *get_gp_pmc(struct kvm_pmu *pmu, u32 msr,
>>>> return NULL;
>>>> }
>>>>
>>>> +static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
>>>> +{
>>>> + return test_bit(idx, pmu->supported_fixed_pmc_idx);
>>>> +}
>>>> +
>>>> /* returns fixed PMC with the specified MSR */
>>>> static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>>>> {
>>>> @@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>>>> u32 index = array_index_nospec(msr - base,
>>>> pmu->nr_arch_fixed_counters);
>>>>
>>>> + if (!fixed_ctr_is_supported(pmu, index))
>>>> + return NULL;
>>>> +
>>>> return &pmu->fixed_counters[index];
>>>> }
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>>>> index e8a3be0b9df9..12f4b2fe7756 100644
>>>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>>>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>>>> @@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
>>>> static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>>>> {
>>>> struct kvm_pmc *pmc;
>>>> - u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>>>> + u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>>>> int i;
>>>>
>>>> pmu->fixed_ctr_ctrl = data;
>>>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>>>> - u8 new_ctrl = fixed_ctrl_field(data, i);
>>>> - u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>>>> + if (!fixed_ctr_is_supported(pmu, i))
>>>> + continue;
>>>> +
>>>> + new_ctrl = fixed_ctrl_field(data, i);
>>>> + old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>>>>
>>>> if (old_ctrl == new_ctrl)
>>>> continue;
>>>> @@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
>>>>
>>>> idx &= ~(3u << 30);
>>>>
>>>> + if (fixed && !fixed_ctr_is_supported(pmu, idx))
>>>> + return false;
>>>> +
>>>> return fixed ? idx < pmu->nr_arch_fixed_counters
>>>> : idx < pmu->nr_arch_gp_counters;
>>>> }
>>>> @@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
>>>> counters = pmu->gp_counters;
>>>> num_counters = pmu->nr_arch_gp_counters;
>>>> }
>>>> - if (idx >= num_counters)
>>>> + if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
>>>> return NULL;
>>>> *mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
>>>> return &counters[array_index_nospec(idx, num_counters)];
>>>> @@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
>>>> int i;
>>>>
>>>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>>>> + if (!fixed_ctr_is_supported(pmu, i))
>>>> + continue;
>>>> +
>>>> pmc = &pmu->fixed_counters[i];
>>>> event = fixed_pmc_events[array_index_nospec(i, size)];
>>>> pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
>>>> @@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>>>
>>>> pmu->nr_arch_gp_counters = 0;
>>>> pmu->nr_arch_fixed_counters = 0;
>>>> + bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
>>>> pmu->counter_bitmask[KVM_PMC_GP] = 0;
>>>> pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
>>>> pmu->version = 0;
>>>> @@ -551,13 +561,24 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>>> pmu->available_event_types = ~entry->ebx &
>>>> ((1ull << eax.split.mask_length) - 1);
>>>>
>>>> - if (pmu->version == 1) {
>>>> - pmu->nr_arch_fixed_counters = 0;
>>>> - } else {
>>>> + counter_mask = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
>>>> + bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
>>>> +
>>>> + if (pmu->version > 1) {
>>>> pmu->nr_arch_fixed_counters =
>>>> - min3(ARRAY_SIZE(fixed_pmc_events),
>>>> - (size_t) edx.split.num_counters_fixed,
>>>> - (size_t)kvm_pmu_cap.num_counters_fixed);
>>>> + min_t(int, ARRAY_SIZE(fixed_pmc_events),
>>>> + kvm_pmu_cap.num_counters_fixed);
>>>> + for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>>>> + /* FxCtr[i]_is_supported := CPUID.0xA.ECX[i] || (EDX[4:0] > i) */
>>>
>>> This is true only when pmu->version >= 5.
>>
>> This is true in for "Version 5" section, but not mentioned in the CPUID.0xA section.
>> I would argue that this is a deliberate omission for the instruction implementation,
>> as it does use the word "version>1" in the near CPUID.0xA.EDX section.
>
> Do you have any evidence to support such an argument? The CPUID field
> in question was not defined prior to PMU version 5. (Does anyone from
> Intel want to chime in?)
>
>> For virtualised use, this feature offers a kind of flexibility as users can
>> enable part of
>> the fixed counters, don't you think? Or maybe you're more looking forward to the
>> patch set that raises the vPMU version number from 2 to 5, that part of the code
>> was already in my tree some years ago.
>
> I would not be surprised if a guest OS checked for PMU version 5
> before consulting the CPUID fields defined in PMU version 5. Does
> Linux even consult the fixed counter bitmask field today?
Yes, this is how host perf developer do it:
if (version >= 5)
x86_pmu.num_counters_fixed = fls(fixed_mask);
based on real fresh hardware (always marked as the latest version).
However, our KVM players can construct different valid CPUIDs, as long as the
hardware is capable,
to emulate some vPMU devices that match the CPUID semantics but do not exist in
the real world.
In the virtualisation world, use cases like "version 2 + fixed ctrs bit mask"
are perfectly possible
and should work as expected. One more case, if the forth fixed counter or more
is enabled in your guest for top-down feature and you may still find the guest's
pmu version number is stuck at 2.
This naturally does not occur in real hardware but no CPUID semantics here are
broken.
As I'm sure you've noticed, the logical relationship between CPUID.0xA.ECX and
PMU version 5
is necessary but not sufficient. Version 5 mush has fixed counters bit mask but
the reverse is not true.
From the end user's point of view, destroying the flexibility of vHW
combinations is a design failure.
So I think we can implement this feature in guest version 2, what do you think ?
>
> I'd love to see KVM virtualize PMU version 5!
Great, I've got you and my plan will cover it.
>
>>>
>>> From the SDM, volume 3, section 20.2.5 Architectural Performance
>>> Monitoring Version 5:
>>>
>>> With Architectural Performance Monitoring Version 5, register
>>> CPUID.0AH.ECX indicates Fixed Counter enumeration. It is a bit mask
>>> which enumerates the supported Fixed Counters in a processor. If bit
>>> 'i' is set, it implies that Fixed Counter 'i' is supported. Software
>>> is recommended to use the following logic to check if a Fixed Counter
>>> is supported on a given processor: FxCtr[i]_is_supported := ECX[i] ||
>>> (EDX[4:0] > i);
>>>
>>> Prior to PMU version 5, all fixed counters from 0 through <number of
>>> fixed counters - 1> are supported.
>>>
>>>> + if (!(entry->ecx & BIT_ULL(i) ||
>>>> + edx.split.num_counters_fixed > i))
>>>> + continue;
>>>> +
>>>> + set_bit(i, pmu->supported_fixed_pmc_idx);
>>>> + set_bit(INTEL_PMC_MAX_GENERIC + i, pmu->all_valid_pmc_idx);
>>>> + pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
>>>> + counter_mask &= ~BIT_ULL(INTEL_PMC_MAX_GENERIC + i);
>>>> + }
>>>> edx.split.bit_width_fixed = min_t(int, edx.split.bit_width_fixed,
>>>> kvm_pmu_cap.bit_width_fixed);
>>>> pmu->counter_bitmask[KVM_PMC_FIXED] =
>>>> @@ -565,10 +586,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>>> setup_fixed_pmc_eventsel(pmu);
>>>> }
>>>>
>>>> - for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
>>>> - pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
>>>> - counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
>>>> - (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
>>>> pmu->global_ctrl_mask = counter_mask;
>>>> pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask
>>>> & ~(MSR_CORE_PERF_GLOBAL_OVF_CTRL_OVF_BUF |
>>>> @@ -585,11 +602,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>>> pmu->raw_event_mask |= (HSW_IN_TX|HSW_IN_TX_CHECKPOINTED);
>>>> }
>>>>
>>>> - bitmap_set(pmu->all_valid_pmc_idx,
>>>> - 0, pmu->nr_arch_gp_counters);
>>>> - bitmap_set(pmu->all_valid_pmc_idx,
>>>> - INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
>>>> -
>>>> perf_capabilities = vcpu_get_perf_capabilities(vcpu);
>>>> if (cpuid_model_is_consistent(vcpu) &&
>>>> (perf_capabilities & PMU_CAP_LBR_FMT))
>>>> @@ -605,6 +617,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>>> pmu->pebs_enable_mask = counter_mask;
>>>> pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
>>>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>>>> + if (!fixed_ctr_is_supported(pmu, i))
>>>> + continue;
>>>> +
>>>> pmu->fixed_ctr_ctrl_mask &=
>>>> ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
>>>> }
>>>>
>>>> base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
>>>> --
>>>> 2.40.0
>>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask
2023-03-28 10:27 ` Like Xu
@ 2023-03-28 16:38 ` Jim Mattson
0 siblings, 0 replies; 7+ messages in thread
From: Jim Mattson @ 2023-03-28 16:38 UTC (permalink / raw)
To: Like Xu; +Cc: Sean Christopherson, Paolo Bonzini, kvm, linux-kernel
On Tue, Mar 28, 2023 at 3:27 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> On 27/3/2023 10:58 pm, Jim Mattson wrote:
> > On Mon, Mar 27, 2023 at 12:47 AM Like Xu <like.xu.linux@gmail.com> wrote:
> >>
> >> On 25/3/2023 7:19 am, Jim Mattson wrote:
> >>> On Tue, Mar 21, 2023 at 4:28 AM Like Xu <like.xu.linux@gmail.com> wrote:
> >>>>
> >>>> From: Like Xu <likexu@tencent.com>
> >>>>
> >>>> Per Intel SDM, fixed-function performance counter 'i' is supported if:
> >>>>
> >>>> FxCtr[i]_is_supported := ECX[i] || (EDX[4:0] > i);
> >>>>
> >>>> which means that the KVM user space can use EDX to limit the number of
> >>>> fixed counters and at the same time, using ECX to enable part of other
> >>>> KVM supported fixed counters.
> >>>>
> >>>> Add a bitmap (instead of always checking the vcpu's CPUIDs) to keep track
> >>>> of the guest available fixed counters and perform the semantic checks.
> >>>>
> >>>> Signed-off-by: Like Xu <likexu@tencent.com>
> >>>> ---
> >>>> arch/x86/include/asm/kvm_host.h | 2 ++
> >>>> arch/x86/kvm/pmu.h | 8 +++++
> >>>> arch/x86/kvm/vmx/pmu_intel.c | 53 +++++++++++++++++++++------------
> >>>> 3 files changed, 44 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >>>> index a45de1118a42..14689e583127 100644
> >>>> --- a/arch/x86/include/asm/kvm_host.h
> >>>> +++ b/arch/x86/include/asm/kvm_host.h
> >>>> @@ -565,6 +565,8 @@ struct kvm_pmu {
> >>>> */
> >>>> bool need_cleanup;
> >>>>
> >>>> + DECLARE_BITMAP(supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
> >>>> +
> >>>> /*
> >>>> * The total number of programmed perf_events and it helps to avoid
> >>>> * redundant check before cleanup if guest don't use vPMU at all.
> >>>> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> >>>> index be62c16f2265..9f4504e5e9d5 100644
> >>>> --- a/arch/x86/kvm/pmu.h
> >>>> +++ b/arch/x86/kvm/pmu.h
> >>>> @@ -111,6 +111,11 @@ static inline struct kvm_pmc *get_gp_pmc(struct kvm_pmu *pmu, u32 msr,
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> +static inline bool fixed_ctr_is_supported(struct kvm_pmu *pmu, unsigned int idx)
> >>>> +{
> >>>> + return test_bit(idx, pmu->supported_fixed_pmc_idx);
> >>>> +}
> >>>> +
> >>>> /* returns fixed PMC with the specified MSR */
> >>>> static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> >>>> {
> >>>> @@ -120,6 +125,9 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
> >>>> u32 index = array_index_nospec(msr - base,
> >>>> pmu->nr_arch_fixed_counters);
> >>>>
> >>>> + if (!fixed_ctr_is_supported(pmu, index))
> >>>> + return NULL;
> >>>> +
> >>>> return &pmu->fixed_counters[index];
> >>>> }
> >>>>
> >>>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> >>>> index e8a3be0b9df9..12f4b2fe7756 100644
> >>>> --- a/arch/x86/kvm/vmx/pmu_intel.c
> >>>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> >>>> @@ -43,13 +43,16 @@ static int fixed_pmc_events[] = {1, 0, 7};
> >>>> static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
> >>>> {
> >>>> struct kvm_pmc *pmc;
> >>>> - u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> >>>> + u8 new_ctrl, old_ctrl, old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> >>>> int i;
> >>>>
> >>>> pmu->fixed_ctr_ctrl = data;
> >>>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >>>> - u8 new_ctrl = fixed_ctrl_field(data, i);
> >>>> - u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
> >>>> + if (!fixed_ctr_is_supported(pmu, i))
> >>>> + continue;
> >>>> +
> >>>> + new_ctrl = fixed_ctrl_field(data, i);
> >>>> + old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
> >>>>
> >>>> if (old_ctrl == new_ctrl)
> >>>> continue;
> >>>> @@ -125,6 +128,9 @@ static bool intel_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx)
> >>>>
> >>>> idx &= ~(3u << 30);
> >>>>
> >>>> + if (fixed && !fixed_ctr_is_supported(pmu, idx))
> >>>> + return false;
> >>>> +
> >>>> return fixed ? idx < pmu->nr_arch_fixed_counters
> >>>> : idx < pmu->nr_arch_gp_counters;
> >>>> }
> >>>> @@ -145,7 +151,7 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
> >>>> counters = pmu->gp_counters;
> >>>> num_counters = pmu->nr_arch_gp_counters;
> >>>> }
> >>>> - if (idx >= num_counters)
> >>>> + if (idx >= num_counters || (fixed && !fixed_ctr_is_supported(pmu, idx)))
> >>>> return NULL;
> >>>> *mask &= pmu->counter_bitmask[fixed ? KVM_PMC_FIXED : KVM_PMC_GP];
> >>>> return &counters[array_index_nospec(idx, num_counters)];
> >>>> @@ -500,6 +506,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> >>>> int i;
> >>>>
> >>>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >>>> + if (!fixed_ctr_is_supported(pmu, i))
> >>>> + continue;
> >>>> +
> >>>> pmc = &pmu->fixed_counters[i];
> >>>> event = fixed_pmc_events[array_index_nospec(i, size)];
> >>>> pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
> >>>> @@ -520,6 +529,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >>>>
> >>>> pmu->nr_arch_gp_counters = 0;
> >>>> pmu->nr_arch_fixed_counters = 0;
> >>>> + bitmap_zero(pmu->supported_fixed_pmc_idx, KVM_PMC_MAX_FIXED);
> >>>> pmu->counter_bitmask[KVM_PMC_GP] = 0;
> >>>> pmu->counter_bitmask[KVM_PMC_FIXED] = 0;
> >>>> pmu->version = 0;
> >>>> @@ -551,13 +561,24 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >>>> pmu->available_event_types = ~entry->ebx &
> >>>> ((1ull << eax.split.mask_length) - 1);
> >>>>
> >>>> - if (pmu->version == 1) {
> >>>> - pmu->nr_arch_fixed_counters = 0;
> >>>> - } else {
> >>>> + counter_mask = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
> >>>> + bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
> >>>> +
> >>>> + if (pmu->version > 1) {
> >>>> pmu->nr_arch_fixed_counters =
> >>>> - min3(ARRAY_SIZE(fixed_pmc_events),
> >>>> - (size_t) edx.split.num_counters_fixed,
> >>>> - (size_t)kvm_pmu_cap.num_counters_fixed);
> >>>> + min_t(int, ARRAY_SIZE(fixed_pmc_events),
> >>>> + kvm_pmu_cap.num_counters_fixed);
> >>>> + for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >>>> + /* FxCtr[i]_is_supported := CPUID.0xA.ECX[i] || (EDX[4:0] > i) */
> >>>
> >>> This is true only when pmu->version >= 5.
> >>
> >> This is true in for "Version 5" section, but not mentioned in the CPUID.0xA section.
> >> I would argue that this is a deliberate omission for the instruction implementation,
> >> as it does use the word "version>1" in the near CPUID.0xA.EDX section.
> >
> > Do you have any evidence to support such an argument? The CPUID field
> > in question was not defined prior to PMU version 5. (Does anyone from
> > Intel want to chime in?)
> >
> >> For virtualised use, this feature offers a kind of flexibility as users can
> >> enable part of
> >> the fixed counters, don't you think? Or maybe you're more looking forward to the
> >> patch set that raises the vPMU version number from 2 to 5, that part of the code
> >> was already in my tree some years ago.
> >
> > I would not be surprised if a guest OS checked for PMU version 5
> > before consulting the CPUID fields defined in PMU version 5. Does
> > Linux even consult the fixed counter bitmask field today?
>
> Yes, this is how host perf developer do it:
>
> if (version >= 5)
> x86_pmu.num_counters_fixed = fls(fixed_mask);
>
> based on real fresh hardware (always marked as the latest version).
>
> However, our KVM players can construct different valid CPUIDs, as long as the
> hardware is capable,
> to emulate some vPMU devices that match the CPUID semantics but do not exist in
> the real world.
>
> In the virtualisation world, use cases like "version 2 + fixed ctrs bit mask"
> are perfectly possible
> and should work as expected. One more case, if the forth fixed counter or more
> is enabled in your guest for top-down feature and you may still find the guest's
> pmu version number is stuck at 2.
> This naturally does not occur in real hardware but no CPUID semantics here are
> broken.
This is completely irrelevant to the current discussion. Nowhere is it
documented that the number of fixed counters has a specific value for
any given PMU version. However, it *is* documented that the fixed
counter availability bitmask is introduced in PMU version 5. Surely,
you understand the difference.
> As I'm sure you've noticed, the logical relationship between CPUID.0xA.ECX and
> PMU version 5
> is necessary but not sufficient. Version 5 mush has fixed counters bit mask but
> the reverse is not true.
The reverse most certainly is true. You are, as is your wont, making
stuff up again.
> From the end user's point of view, destroying the flexibility of vHW
> combinations is a design failure.
>
> So I think we can implement this feature in guest version 2, what do you think ?
How does the userspace VMM query the capability? Certainly, if
KVM_GET_SUPPORTED_CPUID returns a PMU version >= 5, then this bitmap
is supported. But for PMU version 2, up until now, the bitmap has not
been supported.
And do you expect the guest OS to check for the HYPERVISOR bit and the
KVMKVMKVM signature to determine whether or not this bitmap is
meaningful for PMU versions < 5?
> >
> > I'd love to see KVM virtualize PMU version 5!
>
> Great, I've got you and my plan will cover it.
>
> >
> >>>
> >>> From the SDM, volume 3, section 20.2.5 Architectural Performance
> >>> Monitoring Version 5:
> >>>
> >>> With Architectural Performance Monitoring Version 5, register
> >>> CPUID.0AH.ECX indicates Fixed Counter enumeration. It is a bit mask
> >>> which enumerates the supported Fixed Counters in a processor. If bit
> >>> 'i' is set, it implies that Fixed Counter 'i' is supported. Software
> >>> is recommended to use the following logic to check if a Fixed Counter
> >>> is supported on a given processor: FxCtr[i]_is_supported := ECX[i] ||
> >>> (EDX[4:0] > i);
> >>>
> >>> Prior to PMU version 5, all fixed counters from 0 through <number of
> >>> fixed counters - 1> are supported.
> >>>
> >>>> + if (!(entry->ecx & BIT_ULL(i) ||
> >>>> + edx.split.num_counters_fixed > i))
> >>>> + continue;
> >>>> +
> >>>> + set_bit(i, pmu->supported_fixed_pmc_idx);
> >>>> + set_bit(INTEL_PMC_MAX_GENERIC + i, pmu->all_valid_pmc_idx);
> >>>> + pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
> >>>> + counter_mask &= ~BIT_ULL(INTEL_PMC_MAX_GENERIC + i);
> >>>> + }
> >>>> edx.split.bit_width_fixed = min_t(int, edx.split.bit_width_fixed,
> >>>> kvm_pmu_cap.bit_width_fixed);
> >>>> pmu->counter_bitmask[KVM_PMC_FIXED] =
> >>>> @@ -565,10 +586,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >>>> setup_fixed_pmc_eventsel(pmu);
> >>>> }
> >>>>
> >>>> - for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
> >>>> - pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
> >>>> - counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
> >>>> - (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
> >>>> pmu->global_ctrl_mask = counter_mask;
> >>>> pmu->global_ovf_ctrl_mask = pmu->global_ctrl_mask
> >>>> & ~(MSR_CORE_PERF_GLOBAL_OVF_CTRL_OVF_BUF |
> >>>> @@ -585,11 +602,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >>>> pmu->raw_event_mask |= (HSW_IN_TX|HSW_IN_TX_CHECKPOINTED);
> >>>> }
> >>>>
> >>>> - bitmap_set(pmu->all_valid_pmc_idx,
> >>>> - 0, pmu->nr_arch_gp_counters);
> >>>> - bitmap_set(pmu->all_valid_pmc_idx,
> >>>> - INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
> >>>> -
> >>>> perf_capabilities = vcpu_get_perf_capabilities(vcpu);
> >>>> if (cpuid_model_is_consistent(vcpu) &&
> >>>> (perf_capabilities & PMU_CAP_LBR_FMT))
> >>>> @@ -605,6 +617,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >>>> pmu->pebs_enable_mask = counter_mask;
> >>>> pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
> >>>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> >>>> + if (!fixed_ctr_is_supported(pmu, i))
> >>>> + continue;
> >>>> +
> >>>> pmu->fixed_ctr_ctrl_mask &=
> >>>> ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
> >>>> }
> >>>>
> >>>> base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
> >>>> --
> >>>> 2.40.0
> >>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-28 16:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-21 11:27 [PATCH] KVM: x86/pmu: Add Intel PMU supported fixed counters bit mask Like Xu
2023-03-24 22:46 ` Sean Christopherson
2023-03-24 23:19 ` Jim Mattson
2023-03-27 7:47 ` Like Xu
2023-03-27 14:58 ` Jim Mattson
2023-03-28 10:27 ` Like Xu
2023-03-28 16:38 ` Jim Mattson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).