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

[Chen, Zide]

On 11/9/2026 3:12 PM, Yosry Ahmed wrote:
> 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);

I am very happy to see these variables go away. Thank you!

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

Right, I didn't realize C99 compound literals could be used here.

However, with this suggested code, kvm_gp_pmc_mask() would be
re-evaluated on every iteration. It seems this “expression with side
effects evaluated more than once” is not encouraged by the coding style,
is it?