Re: [PATCH v3 04/18] KVM: x86/pmu: Add PMC bitmap accessor helpers

[Yosry Ahmed <yosry@kernel.org>]

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))