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

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

On 11/6/2026 4:35 PM, Yosry Ahmed wrote:
> On Fri, Nov 6, 2026 at 2:19 PM Chen, Zide <zide.chen@intel.com> wrote:
>>
>>
>>
>> On 11/6/2026 1:09 PM, Yosry Ahmed wrote:
>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>>> index b8875ed5502c..4f192429ed93 100644
>>>> --- a/arch/x86/include/asm/kvm_host.h
>>>> +++ b/arch/x86/include/asm/kvm_host.h
>>>> @@ -595,7 +595,10 @@ struct kvm_pmu {
>>>>              DECLARE_BITMAP(reprogram_pmi, X86_PMC_IDX_MAX);
>>>>              atomic64_t __reprogram_pmi;
>>>>      };
>>>> -    DECLARE_BITMAP(pmc_exists, X86_PMC_IDX_MAX);
>>>> +    union {
>>>> +            DECLARE_BITMAP(pmc_exists, X86_PMC_IDX_MAX);
>>>> +            u64 pmc_exists64;
>>>
>>> __pmc_exists would be more consistent with __reprogam_pmi above.
>>
>> I used the *64 suffix and without '__' to match the cntr_mask/cntr_mask64
>> and fixed_cntr_mask/fixed_cntr_mask64 pattern in struct x86_pmu_capability
>> (patch 7), whereas __reprogram_pmi used '__' because it is an atomic64_t
>> rather than a scalar mask.
> 
> I don't follow why the '__' prefix is specific to atomics, but anyway,
> I will leave this up to you and Sean.
> 
>>
>> Also, arch/x86/events/perf_event.h has similar names:
>>
>>         union {
>>                         u64             cntr_mask64;
>>                         unsigned long
>> cntr_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
>>         };
>>         union {
>>                         u64             fixed_cntr_mask64;
>>                         unsigned long
>> fixed_cntr_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
>>         };
>>
>> Seems either way won't be absolutely unifie
>>
>>>> @@ -120,6 +144,17 @@ static inline struct kvm_pmc *kvm_pmc_idx_to_pmc(struct kvm_pmu *pmu, int idx)
>>>>                      continue;                               \
>>>>              else                                            \
>>>>
>>>> +/*
>>>> + * @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.

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)