Re: [PATCH v3 04/18] KVM: x86/pmu: Add PMC bitmap accessor helpers
From: Yosry Ahmed
Date: Mon Nov 09 2026 - 16:14:44 EST
On Mon, Nov 09, 2026 at 12:59:01PM -0600, Chen, Zide wrote:
>
>
> On 11/9/2026 12:07 PM, Yosry Ahmed wrote:
> >>>>>> +/*
> >>>>>> + * @mask is expected to be a scalar unsigned long derived from pmu->pmc_exists,
> >>>>>> + * which is already constrained by KVM_MAX_NR_{AMD,INTEL}_{GP,FIXED}_COUNTERS,
> >>>>>> + * so iteration up to KVM's maximum counter count is safe.
> >>>>>> + */
> >>>>>> +#define kvm_for_each_gp_counter(i, mask) \
> >>>>>> + for_each_set_bit((i), &(mask), KVM_MAX_NR_GP_COUNTERS)
> >>>>>> +
> >>>>>> +#define kvm_for_each_fixed_counter(i, mask) \
> >>>>>> + for_each_set_bit((i), &(mask), KVM_MAX_NR_FIXED_COUNTERS)
> >>>>>
> >>>>> These are not used in this patch, are they?
> >>>>>
> >>>>> They should probably be introduced when they are used. Also, do we
> >>>>> always end up passing kvm_gp_pmc_mask() and kvm_fixed_pmc_mask()? If
> >>>>> yes, maybe we should pass in the pmu here instead?
> >>>>
> >>>> The intent was to group all bitmap accessor helpers together in this
> >>>> patch for easier review so that the big patch 5/18 could be kept to
> >>>> mostly mechanical replacements.
> >>>>
> >>>> But if you prefer introducing them on first use, I'll move their
> >>>> definitions to patch 5.
> >>>
> >>> Yeah I think it's clearer to see what their definitions are with their usage.
> >>
> >> OK, then I can move these two macros to the next patch.
> >>>>
> >>>> Regarding passing 'pmu' directly, I initially implemented it in this
> >>>> way, but I can't recall why I discarded it. I will take another look.
> >>>
> >>> Thanks.
> >>
> >> How about these? With them, we can remove the {gp,fixed}_mask variables
> >> that you pointed out in your review of 5/18. The APIs are more
> >> complicated, but the caller code is much cleaner.
> >
> > I don't understand why we can't keep the macros as-is, but just move the
> > calls to kvm_gp_pmc_mask() and kvm_fixed_pmc_mask() inside the macros?
>
> for_each_set_bit() requires a pointer (`const unsigned long *`) as
> input, but AFAICT a loop macro cannot declare a temporary local
> variable, nor it can use do { ... } while(0). So something like:
>
> #define kvm_for_each_gp_counter(i, pmu) \
> unsigned long gp_mask = kvm_gp_pmc_mask(pmu); \
> for_each_set_bit((i), &(gp_mask), KVM_MAX_NR_GP_COUNTERS)
>
Gemini suggested compount literals, and it seems to at least compile and
I see a few instances around the kernel:
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index f949fdbf359c6..3930680937577 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -1347,8 +1347,6 @@ static __always_inline u32 gp_eventsel_msr(u32 idx)
static void kvm_pmu_load_guest_pmcs(struct kvm_vcpu *vcpu)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
- unsigned long fixed_mask = kvm_fixed_pmc_mask(pmu);
- unsigned long gp_mask = kvm_gp_pmc_mask(pmu);
struct kvm_pmc *pmc;
u32 i;
@@ -1357,14 +1355,14 @@ static void kvm_pmu_load_guest_pmcs(struct kvm_vcpu *vcpu)
* is intercepted if hardware has counters that aren't visible to the
* guest (KVM will inject #GP as appropriate).
*/
- kvm_for_each_gp_counter(i, gp_mask) {
+ kvm_for_each_gp_counter(i, pmu) {
pmc = &pmu->gp_counters[i];
if (pmc->counter != rdpmc(i))
wrmsrq(gp_counter_msr(i), pmc->counter);
wrmsrq(gp_eventsel_msr(i), pmc->eventsel_hw);
}
- kvm_for_each_fixed_counter(i, fixed_mask) {
+ kvm_for_each_fixed_counter(i, pmu) {
pmc = &pmu->fixed_counters[i];
if (pmc->counter != rdpmc(INTEL_PMC_FIXED_RDPMC_BASE | i))
@@ -1407,8 +1405,6 @@ void kvm_mediated_pmu_load(struct kvm_vcpu *vcpu)
static void kvm_pmu_put_guest_pmcs(struct kvm_vcpu *vcpu)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
- unsigned long fixed_mask = kvm_fixed_pmc_mask(pmu);
- unsigned long gp_mask = kvm_gp_pmc_mask(pmu);
struct kvm_pmc *pmc;
u32 i;
@@ -1416,7 +1412,7 @@ static void kvm_pmu_put_guest_pmcs(struct kvm_vcpu *vcpu)
* Clear selectors and counters to ensure hardware doesn't count using
* guest controls when the host (perf) restores its state.
*/
- kvm_for_each_gp_counter(i, gp_mask) {
+ kvm_for_each_gp_counter(i, pmu) {
pmc = &pmu->gp_counters[i];
pmc->counter = rdpmc(i);
@@ -1426,7 +1422,7 @@ static void kvm_pmu_put_guest_pmcs(struct kvm_vcpu *vcpu)
wrmsrq(gp_eventsel_msr(i), 0);
}
- kvm_for_each_fixed_counter(i, fixed_mask) {
+ kvm_for_each_fixed_counter(i, pmu) {
pmc = &pmu->fixed_counters[i];
pmc->counter = rdpmc(INTEL_PMC_FIXED_RDPMC_BASE | i);
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 98e6f2443b82e..37a6664fdb9c9 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -149,11 +149,11 @@ static inline struct kvm_pmc *kvm_pmc_idx_to_pmc(struct kvm_pmu *pmu, int idx)
* which is already constrained by KVM_MAX_NR_{AMD,INTEL}_{GP,FIXED}_COUNTERS,
* so iteration up to KVM's maximum counter count is safe.
*/
-#define kvm_for_each_gp_counter(i, mask) \
- for_each_set_bit((i), &(mask), KVM_MAX_NR_GP_COUNTERS)
+#define kvm_for_each_gp_counter(i, pmu) \
+ for_each_set_bit((i), &(unsigned long){ kvm_gp_pmc_mask(pmu) }, KVM_MAX_NR_GP_COUNTERS)
-#define kvm_for_each_fixed_counter(i, mask) \
- for_each_set_bit((i), &(mask), KVM_MAX_NR_FIXED_COUNTERS)
+#define kvm_for_each_fixed_counter(i, pmu) \
+ for_each_set_bit((i), &(unsigned long){ kvm_fixed_pmc_mask(pmu) }, KVM_MAX_NR_FIXED_COUNTERS)
static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
{
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 6dade9d26d1ad..9b25590469712 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -704,8 +704,6 @@ static void nested_vmx_merge_pmu_msr_bitmaps(struct kvm_vcpu *vcpu,
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
struct vcpu_vmx *vmx = to_vmx(vcpu);
- unsigned long fixed_mask = kvm_fixed_pmc_mask(pmu);
- unsigned long gp_mask = kvm_gp_pmc_mask(pmu);
int i;
/*
@@ -715,12 +713,12 @@ static void nested_vmx_merge_pmu_msr_bitmaps(struct kvm_vcpu *vcpu,
if (!kvm_vcpu_has_mediated_pmu(vcpu))
return;
- kvm_for_each_gp_counter(i, gp_mask) {
+ kvm_for_each_gp_counter(i, pmu) {
nested_vmx_merge_msr_bitmaps_rw(MSR_IA32_PERFCTR0 + i);
nested_vmx_merge_msr_bitmaps_rw(MSR_IA32_PMC0 + i);
}
- kvm_for_each_fixed_counter(i, fixed_mask)
+ kvm_for_each_fixed_counter(i, pmu)
nested_vmx_merge_msr_bitmaps_rw(MSR_CORE_PERF_FIXED_CTR0 + i);
nested_vmx_merge_msr_bitmaps_rw(MSR_CORE_PERF_GLOBAL_CTRL);
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 961d16e7f31f5..d3a00ae8018eb 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -57,11 +57,10 @@ static struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu)
static u64 intel_pmu_fixed_ctrl_bits(struct kvm_pmu *pmu, u64 bits)
{
- unsigned long fixed_mask = kvm_fixed_pmc_mask(pmu);
u64 fixed_ctrl_bits = 0;
int i;
- kvm_for_each_fixed_counter(i, fixed_mask)
+ kvm_for_each_fixed_counter(i, pmu)
fixed_ctrl_bits |= intel_fixed_bits_by_idx(i, bits);
return fixed_ctrl_bits;
@@ -85,13 +84,12 @@ static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
* hardware, e.g. to ensure the event filter is enforced.
*/
u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl_hw;
- unsigned long fixed_mask = kvm_fixed_pmc_mask(pmu);
struct kvm_pmc *pmc;
int i;
pmu->fixed_ctr_ctrl = data;
pmu->fixed_ctr_ctrl_hw = data & ~intel_pmu_fixed_anythread_mask(pmu);
- kvm_for_each_fixed_counter(i, fixed_mask) {
+ kvm_for_each_fixed_counter(i, pmu) {
u8 new_ctrl = fixed_ctrl_field(pmu->fixed_ctr_ctrl_hw, i);
u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
@@ -369,21 +367,19 @@ static bool intel_pmu_handle_lbr_msrs_access(struct kvm_vcpu *vcpu,
static u64 intel_pmu_get_global_inuse(struct kvm_vcpu *vcpu)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
- unsigned long fixed_mask = kvm_fixed_pmc_mask(pmu);
- unsigned long gp_mask = kvm_gp_pmc_mask(pmu);
bool pmi_inuse = false;
u64 eventsel, data = 0;
u32 fixed_ctrl;
int i;
- kvm_for_each_gp_counter(i, gp_mask) {
+ kvm_for_each_gp_counter(i, pmu) {
eventsel = pmu->gp_counters[i].eventsel;
if (eventsel & ARCH_PERFMON_EVENTSEL_EVENT)
data |= BIT_ULL(i);
pmi_inuse |= !!(eventsel & ARCH_PERFMON_EVENTSEL_INT);
}
- kvm_for_each_fixed_counter(i, fixed_mask) {
+ kvm_for_each_fixed_counter(i, pmu) {
fixed_ctrl = fixed_ctrl_field(pmu->fixed_ctr_ctrl, i);
if (fixed_ctrl & (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER))