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

[Chen, Zide]

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.

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.

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.
>> @@ -183,9 +221,12 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>>  {
>>  	int base = MSR_CORE_PERF_FIXED_CTR0;
>>  
>> -	if (msr >= base && msr < base + pmu->nr_arch_fixed_counters) {
>> +	if (msr >= base && msr < base + KVM_MAX_NR_FIXED_COUNTERS) {
>>  		u32 index = array_index_nospec(msr - base,
>> -					       pmu->nr_arch_fixed_counters);
>> +					       KVM_MAX_NR_FIXED_COUNTERS);
>> +
>> +		if (!kvm_is_fixed_pmc_supported(pmu, index))
>> +			return NULL;
> 
> Do all callers handle NULL correctly? At least
> reprogram_fixed_counters() doesn't look like it. It may be fine since
> NULL can already be returned today. Not sure if this series makes it
> more likely.

All other callers properly check NULL.

In reprogram_fixed_counters(), 'i' comes from
kvm_for_each_fixed_counter(), so it is guaranteed supported and cannot
return NULL (just as it was safe previously via "i <
pmu->nr_arch_fixed_counters").

However, calling get_fixed_pmc() in reprogram_fixed_counters() was
unnecessary indirection. In patch 5, I can clean this up to index directly:

@@ -98,7 +98,7 @@ static void reprogram_fixed_counters(struct kvm_pmu
*pmu, u64 data)
                if (old_ctrl == new_ctrl)
                        continue;

-               pmc = get_fixed_pmc(pmu, MSR_CORE_PERF_FIXED_CTR0 + i);
+               pmc = &pmu->fixed_counters[i];

>>  
>>  		return &pmu->fixed_counters[index];
>>  	}
>> -- 
>> 2.56.0
>>