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

["Chen, Zide" <zide.chen@intel.com>]

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)

It compiles in current code, but it may cause a few issues:

1. It fails to compile if called twice in the same function.

 kvm_for_each_gp_counter(i, pmu) { blahblah; }
 kvm_for_each_gp_counter(i, pmu) { blahblah; }

This would result in a redefinition of gp_mask.

2. It could fail to compile or lead to subtle bugs when used under if (...):

if (pmu->version >= 2)
	kvm_for_each_gp_counter(i, pmu)
		foo(i);

which would expand to:

if (pmu->version >= 2)
	unsigned long gp_mask = kvm_gp_pmc_mask(pmu);
for_each_set_bit((i), &(gp_mask), ...)
	foo(i);

>>
>> static inline unsigned int __kvm_find_next_counter(struct kvm_pmu *pmu,
>>                                              unsigned int base,
>>                                              unsigned int max,
>>                                              unsigned int prev)
>> {
>>         return find_next_bit(pmu->pmc_exists, base + max, base + prev) -
>> base;
>> }
>>
>> #define __kvm_for_each_counter(i, pmu, base, max)                       \
>>         for ((i) = __kvm_find_next_counter((pmu), (base), (max), 0);
>>      \
>>              (i) < (max);                                               \
>>              (i) = __kvm_find_next_counter((pmu), (base), (max), (i) + 1))
>>
>> /*
>>  * Iterate over all general-purpose or fixed counters supported by @pmu.
>>  */
>> #define kvm_for_each_gp_counter(i, pmu) \
>>         __kvm_for_each_counter(i, pmu, 0, KVM_MAX_NR_GP_COUNTERS)
>>
>> #define kvm_for_each_fixed_counter(i, pmu) \
>>         __kvm_for_each_counter(i, pmu, KVM_FIXED_PMC_BASE_IDX,
>> KVM_MAX_NR_FIXED_COUNTERS)