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

[Yosry Ahmed]

> >>>> +/*
> >>>> + * @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?


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