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

[Mi, Dapeng]

On 11/7/2026 6:35 AM, Yosry Ahmed wrote:
> On Fri, Nov 6, 2026 at 2:19 PM Chen, Zide <zide.chen@xxxxxxxxx> 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.

Per my understanding, '__' prefix hints this function/variable should not
be accessed directly, it should be accessed via some wrapper helpers. So I
think it depends on if we hope the variable pmc_exists64 should be accessed
directly.

Thanks.


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