Magic numbers are used to manipulate the bit fields of
FIXED_CTR_CTRL MSR. This is not read-friendly and use macros to replace
these magic numbers to increase the readability.
Signed-off-by: Dapeng Mi <[email protected]>
---
arch/x86/include/asm/perf_event.h | 2 ++
arch/x86/kvm/pmu.c | 16 +++++++---------
arch/x86/kvm/pmu.h | 10 +++++++---
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 85a9fd5a3ec3..018441211af1 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -38,6 +38,8 @@
#define INTEL_FIXED_0_USER (1ULL << 1)
#define INTEL_FIXED_0_ANYTHREAD (1ULL << 2)
#define INTEL_FIXED_0_ENABLE_PMI (1ULL << 3)
+#define INTEL_FIXED_0_ENABLE \
+ (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER)
#define HSW_IN_TX (1ULL << 32)
#define HSW_IN_TX_CHECKPOINTED (1ULL << 33)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index edb89b51b383..03fb6b4bca2c 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -418,13 +418,12 @@ static void reprogram_counter(struct kvm_pmc *pmc)
printk_once("kvm pmu: pin control bit is ignored\n");
if (pmc_is_fixed(pmc)) {
- fixed_ctr_ctrl = fixed_ctrl_field(pmu->fixed_ctr_ctrl,
- pmc->idx - INTEL_PMC_IDX_FIXED);
- if (fixed_ctr_ctrl & 0x1)
+ fixed_ctr_ctrl = pmu_fixed_ctrl_field(pmu, pmc->idx);
+ if (fixed_ctr_ctrl & INTEL_FIXED_0_KERNEL)
eventsel |= ARCH_PERFMON_EVENTSEL_OS;
- if (fixed_ctr_ctrl & 0x2)
+ if (fixed_ctr_ctrl & INTEL_FIXED_0_USER)
eventsel |= ARCH_PERFMON_EVENTSEL_USR;
- if (fixed_ctr_ctrl & 0x8)
+ if (fixed_ctr_ctrl & INTEL_FIXED_0_ENABLE_PMI)
eventsel |= ARCH_PERFMON_EVENTSEL_INT;
new_config = (u64)fixed_ctr_ctrl;
}
@@ -747,10 +746,9 @@ static inline bool cpl_is_matched(struct kvm_pmc *pmc)
select_os = config & ARCH_PERFMON_EVENTSEL_OS;
select_user = config & ARCH_PERFMON_EVENTSEL_USR;
} else {
- config = fixed_ctrl_field(pmc_to_pmu(pmc)->fixed_ctr_ctrl,
- pmc->idx - INTEL_PMC_IDX_FIXED);
- select_os = config & 0x1;
- select_user = config & 0x2;
+ config = pmu_fixed_ctrl_field(pmc_to_pmu(pmc), pmc->idx);
+ select_os = config & INTEL_FIXED_0_KERNEL;
+ select_user = config & INTEL_FIXED_0_USER;
}
return (static_call(kvm_x86_get_cpl)(pmc->vcpu) == 0) ? select_os : select_user;
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 7d9ba301c090..2d098aa2fcc6 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -12,7 +12,11 @@
MSR_IA32_MISC_ENABLE_BTS_UNAVAIL)
/* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
-#define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
+#define fixed_ctrl_field(ctrl_reg, idx) \
+ (((ctrl_reg) >> ((idx) * INTEL_FIXED_BITS_STRIDE)) & INTEL_FIXED_BITS_MASK)
+
+#define pmu_fixed_ctrl_field(pmu, idx) \
+ fixed_ctrl_field((pmu)->fixed_ctr_ctrl, (idx) - INTEL_PMC_IDX_FIXED)
#define VMWARE_BACKDOOR_PMC_HOST_TSC 0x10000
#define VMWARE_BACKDOOR_PMC_REAL_TIME 0x10001
@@ -164,8 +168,8 @@ static inline bool pmc_speculative_in_use(struct kvm_pmc *pmc)
struct kvm_pmu *pmu = pmc_to_pmu(pmc);
if (pmc_is_fixed(pmc))
- return fixed_ctrl_field(pmu->fixed_ctr_ctrl,
- pmc->idx - INTEL_PMC_IDX_FIXED) & 0x3;
+ return pmu_fixed_ctrl_field(pmu, pmc->idx) &
+ INTEL_FIXED_0_ENABLE;
return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
}
--
2.34.1
On Tue, Aug 08, 2023, Dapeng Mi wrote:
> Magic numbers are used to manipulate the bit fields of
> FIXED_CTR_CTRL MSR. This is not read-friendly and use macros to replace
> these magic numbers to increase the readability.
>
> Signed-off-by: Dapeng Mi <[email protected]>
> ---
> arch/x86/include/asm/perf_event.h | 2 ++
> arch/x86/kvm/pmu.c | 16 +++++++---------
> arch/x86/kvm/pmu.h | 10 +++++++---
> 3 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index 85a9fd5a3ec3..018441211af1 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -38,6 +38,8 @@
> #define INTEL_FIXED_0_USER (1ULL << 1)
> #define INTEL_FIXED_0_ANYTHREAD (1ULL << 2)
> #define INTEL_FIXED_0_ENABLE_PMI (1ULL << 3)
> +#define INTEL_FIXED_0_ENABLE \
> + (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER)
I vote to omit INTEL_FIXED_0_ENABLE and open code the "KERNEL | USER" logic in
the one place that uses it. I dislike macros that sneakily cover multiple bits,
i.e. don't have a name that strongly suggests a multi-bit masks. It's too easy
to misread the code, especially for readers that aren't familiar with PMCs, e.g.
in the usage below, it's not at all obvious that the "in use" check will evaluate
true if the PMC is enabled for the kernel *or* user.
> #define HSW_IN_TX (1ULL << 32)
> #define HSW_IN_TX_CHECKPOINTED (1ULL << 33)
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index edb89b51b383..03fb6b4bca2c 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -418,13 +418,12 @@ static void reprogram_counter(struct kvm_pmc *pmc)
> printk_once("kvm pmu: pin control bit is ignored\n");
>
> if (pmc_is_fixed(pmc)) {
> - fixed_ctr_ctrl = fixed_ctrl_field(pmu->fixed_ctr_ctrl,
> - pmc->idx - INTEL_PMC_IDX_FIXED);
> - if (fixed_ctr_ctrl & 0x1)
> + fixed_ctr_ctrl = pmu_fixed_ctrl_field(pmu, pmc->idx);
> + if (fixed_ctr_ctrl & INTEL_FIXED_0_KERNEL)
> eventsel |= ARCH_PERFMON_EVENTSEL_OS;
Please split this into two patches, one to do a straight replacement of literals
with existing macros, and one to add pmu_fixed_ctrl_field(). Using existing
macros is a no-brainer, but I'm less convinced that pmu_fixed_ctrl_field() is
a good idea, e.g. both pmu_fixed_ctrl_field() and fixed_ctrl_field() take "idx",
but use a different base. That is bound to cause problems.
> - if (fixed_ctr_ctrl & 0x2)
> + if (fixed_ctr_ctrl & INTEL_FIXED_0_USER)
> eventsel |= ARCH_PERFMON_EVENTSEL_USR;
> - if (fixed_ctr_ctrl & 0x8)
> + if (fixed_ctr_ctrl & INTEL_FIXED_0_ENABLE_PMI)
> eventsel |= ARCH_PERFMON_EVENTSEL_INT;
> new_config = (u64)fixed_ctr_ctrl;
> }
> @@ -747,10 +746,9 @@ static inline bool cpl_is_matched(struct kvm_pmc *pmc)
> select_os = config & ARCH_PERFMON_EVENTSEL_OS;
> select_user = config & ARCH_PERFMON_EVENTSEL_USR;
> } else {
> - config = fixed_ctrl_field(pmc_to_pmu(pmc)->fixed_ctr_ctrl,
> - pmc->idx - INTEL_PMC_IDX_FIXED);
> - select_os = config & 0x1;
> - select_user = config & 0x2;
> + config = pmu_fixed_ctrl_field(pmc_to_pmu(pmc), pmc->idx);
> + select_os = config & INTEL_FIXED_0_KERNEL;
> + select_user = config & INTEL_FIXED_0_USER;
> }
>
> return (static_call(kvm_x86_get_cpl)(pmc->vcpu) == 0) ? select_os : select_user;
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 7d9ba301c090..2d098aa2fcc6 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -12,7 +12,11 @@
> MSR_IA32_MISC_ENABLE_BTS_UNAVAIL)
>
> /* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
> -#define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
> +#define fixed_ctrl_field(ctrl_reg, idx) \
> + (((ctrl_reg) >> ((idx) * INTEL_FIXED_BITS_STRIDE)) & INTEL_FIXED_BITS_MASK)
> +
> +#define pmu_fixed_ctrl_field(pmu, idx) \
> + fixed_ctrl_field((pmu)->fixed_ctr_ctrl, (idx) - INTEL_PMC_IDX_FIXED)
>
> #define VMWARE_BACKDOOR_PMC_HOST_TSC 0x10000
> #define VMWARE_BACKDOOR_PMC_REAL_TIME 0x10001
> @@ -164,8 +168,8 @@ static inline bool pmc_speculative_in_use(struct kvm_pmc *pmc)
> struct kvm_pmu *pmu = pmc_to_pmu(pmc);
>
> if (pmc_is_fixed(pmc))
> - return fixed_ctrl_field(pmu->fixed_ctr_ctrl,
> - pmc->idx - INTEL_PMC_IDX_FIXED) & 0x3;
> + return pmu_fixed_ctrl_field(pmu, pmc->idx) &
> + INTEL_FIXED_0_ENABLE;
>
> return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
> }
> --
> 2.34.1
>
On Fri, Aug 11, 2023 at 02:52:47PM -0700, Sean Christopherson wrote:
> Date: Fri, 11 Aug 2023 14:52:47 -0700
> From: Sean Christopherson <[email protected]>
> Subject: Re: [PATCH] KVM: x86/pmu: Manipulate FIXED_CTR_CTRL MSR with macros
>
> On Tue, Aug 08, 2023, Dapeng Mi wrote:
> > Magic numbers are used to manipulate the bit fields of
> > FIXED_CTR_CTRL MSR. This is not read-friendly and use macros to replace
> > these magic numbers to increase the readability.
> >
> > Signed-off-by: Dapeng Mi <[email protected]>
> > ---
> > arch/x86/include/asm/perf_event.h | 2 ++
> > arch/x86/kvm/pmu.c | 16 +++++++---------
> > arch/x86/kvm/pmu.h | 10 +++++++---
> > 3 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> > index 85a9fd5a3ec3..018441211af1 100644
> > --- a/arch/x86/include/asm/perf_event.h
> > +++ b/arch/x86/include/asm/perf_event.h
> > @@ -38,6 +38,8 @@
> > #define INTEL_FIXED_0_USER (1ULL << 1)
> > #define INTEL_FIXED_0_ANYTHREAD (1ULL << 2)
> > #define INTEL_FIXED_0_ENABLE_PMI (1ULL << 3)
> > +#define INTEL_FIXED_0_ENABLE \
> > + (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER)
>
> I vote to omit INTEL_FIXED_0_ENABLE and open code the "KERNEL | USER" logic in
> the one place that uses it. I dislike macros that sneakily cover multiple bits,
> i.e. don't have a name that strongly suggests a multi-bit masks. It's too easy
> to misread the code, especially for readers that aren't familiar with PMCs, e.g.
> in the usage below, it's not at all obvious that the "in use" check will evaluate
> true if the PMC is enabled for the kernel *or* user.
>
Ok, you convinced me. I would drop this change. Thanks.
> > #define HSW_IN_TX (1ULL << 32)
> > #define HSW_IN_TX_CHECKPOINTED (1ULL << 33)
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index edb89b51b383..03fb6b4bca2c 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -418,13 +418,12 @@ static void reprogram_counter(struct kvm_pmc *pmc)
> > printk_once("kvm pmu: pin control bit is ignored\n");
> >
> > if (pmc_is_fixed(pmc)) {
> > - fixed_ctr_ctrl = fixed_ctrl_field(pmu->fixed_ctr_ctrl,
> > - pmc->idx - INTEL_PMC_IDX_FIXED);
> > - if (fixed_ctr_ctrl & 0x1)
> > + fixed_ctr_ctrl = pmu_fixed_ctrl_field(pmu, pmc->idx);
> > + if (fixed_ctr_ctrl & INTEL_FIXED_0_KERNEL)
> > eventsel |= ARCH_PERFMON_EVENTSEL_OS;
>
> Please split this into two patches, one to do a straight replacement of literals
> with existing macros, and one to add pmu_fixed_ctrl_field(). Using existing
> macros is a no-brainer, but I'm less convinced that pmu_fixed_ctrl_field() is
> a good idea, e.g. both pmu_fixed_ctrl_field() and fixed_ctrl_field() take "idx",
> but use a different base. That is bound to cause problems.
Ok. It makes sense. It may be a good thing that let users know some
details. Would drop this new macro. Thanks.
>
> > - if (fixed_ctr_ctrl & 0x2)
> > + if (fixed_ctr_ctrl & INTEL_FIXED_0_USER)
> > eventsel |= ARCH_PERFMON_EVENTSEL_USR;
> > - if (fixed_ctr_ctrl & 0x8)
> > + if (fixed_ctr_ctrl & INTEL_FIXED_0_ENABLE_PMI)
> > eventsel |= ARCH_PERFMON_EVENTSEL_INT;
> > new_config = (u64)fixed_ctr_ctrl;
> > }
> > @@ -747,10 +746,9 @@ static inline bool cpl_is_matched(struct kvm_pmc *pmc)
> > select_os = config & ARCH_PERFMON_EVENTSEL_OS;
> > select_user = config & ARCH_PERFMON_EVENTSEL_USR;
> > } else {
> > - config = fixed_ctrl_field(pmc_to_pmu(pmc)->fixed_ctr_ctrl,
> > - pmc->idx - INTEL_PMC_IDX_FIXED);
> > - select_os = config & 0x1;
> > - select_user = config & 0x2;
> > + config = pmu_fixed_ctrl_field(pmc_to_pmu(pmc), pmc->idx);
> > + select_os = config & INTEL_FIXED_0_KERNEL;
> > + select_user = config & INTEL_FIXED_0_USER;
> > }
> >
> > return (static_call(kvm_x86_get_cpl)(pmc->vcpu) == 0) ? select_os : select_user;
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 7d9ba301c090..2d098aa2fcc6 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -12,7 +12,11 @@
> > MSR_IA32_MISC_ENABLE_BTS_UNAVAIL)
> >
> > /* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
> > -#define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
> > +#define fixed_ctrl_field(ctrl_reg, idx) \
> > + (((ctrl_reg) >> ((idx) * INTEL_FIXED_BITS_STRIDE)) & INTEL_FIXED_BITS_MASK)
> > +
> > +#define pmu_fixed_ctrl_field(pmu, idx) \
> > + fixed_ctrl_field((pmu)->fixed_ctr_ctrl, (idx) - INTEL_PMC_IDX_FIXED)
> >
> > #define VMWARE_BACKDOOR_PMC_HOST_TSC 0x10000
> > #define VMWARE_BACKDOOR_PMC_REAL_TIME 0x10001
> > @@ -164,8 +168,8 @@ static inline bool pmc_speculative_in_use(struct kvm_pmc *pmc)
> > struct kvm_pmu *pmu = pmc_to_pmu(pmc);
> >
> > if (pmc_is_fixed(pmc))
> > - return fixed_ctrl_field(pmu->fixed_ctr_ctrl,
> > - pmc->idx - INTEL_PMC_IDX_FIXED) & 0x3;
> > + return pmu_fixed_ctrl_field(pmu, pmc->idx) &
> > + INTEL_FIXED_0_ENABLE;
> >
> > return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
> > }
> > --
> > 2.34.1
> >
--
Thanks,
Dapeng Mi