Re: [PATCH 3/4] KVM: x86/pmu: Reuse find_perf_hw_id() and drop find_fixed_event()

[Like Xu <like.xu.linux@gmail.com>]

On 18/11/2021 11:00 pm, Paolo Bonzini wrote:
> On 11/16/21 13:20, Like Xu wrote:
>> +static inline unsigned int intel_find_fixed_event(int idx)
>> +{
>> +    u32 event;
>> +    size_t size = ARRAY_SIZE(fixed_pmc_events);
>> +
>> +    if (idx >= size)
>> +        return PERF_COUNT_HW_MAX;
>> +
>> +    event = fixed_pmc_events[array_index_nospec(idx, size)];
>> +    return intel_arch_events[event].event_type;
>> +}
>> +
>> +
>>   static unsigned int intel_find_perf_hw_id(struct kvm_pmc *pmc)
>>   {
>>       struct kvm_pmu *pmu = pmc_to_pmu(pmc);
>> @@ -75,6 +88,9 @@ static unsigned int intel_find_perf_hw_id(struct kvm_pmc *pmc)
>>       u8 unit_mask = (pmc->eventsel & ARCH_PERFMON_EVENTSEL_UMASK) >> 8;
>>       int i;
>> +    if (pmc_is_fixed(pmc))
>> +        return intel_find_fixed_event(pmc->idx - INTEL_PMC_IDX_FIXED);
> 
> Is intel_find_fixed_event needed at all?  As you point out in the commit
> message, eventsel/unit_mask are valid so you can do
> 
> @@ -88,13 +75,11 @@ static unsigned int intel_pmc_perf_hw_id(struct kvm_pmc *pmc)
>       u8 unit_mask = (pmc->eventsel & ARCH_PERFMON_EVENTSEL_UMASK) >> 8;
>       int i;
> 
> -    if (pmc_is_fixed(pmc))
> -        return intel_find_fixed_event(pmc->idx - INTEL_PMC_IDX_FIXED);
> -
>       for (i = 0; i < ARRAY_SIZE(intel_arch_events); i++)
>           if (intel_arch_events[i].eventsel == event_select
>               && intel_arch_events[i].unit_mask == unit_mask
> -            && (pmu->available_event_types & (1 << i)))
> +            && (pmc_is_fixed(pmc) ||
> +            pmu->available_event_types & (1 << i)))

It's a good move while the tricky thing I've found recently is:

the events masked in pmu->available_event_types are just *Intel CPUID* events
(they're not a subset or superset of the *kernel generic* hw events),
some of which can be programmed and enabled with generic or fixed counter.

According Intel SDM, when an Intel CPUID event (e.g. "instructions retirement")
is not masked in pmu->available_event_types (configured via Intel CPUID.0A.EBX),
the guest should not use generic or fixed counter to count/sample this event.

This issue is detailed in another patch set [1] and comments need to be collected.

It's proposed to get [V2] merged and continue to review the fixes from [1] 
seamlessly,
and then further unify all fixed/gp stuff including intel_find_fixed_event() as 
a follow up.

[1] https://lore.kernel.org/kvm/20211112095139.21775-1-likexu@tencent.com/
[V2] https://lore.kernel.org/kvm/20211119064856.77948-1-likexu@tencent.com/

>               break;
> 
>       if (i == ARRAY_SIZE(intel_arch_events))
> 
> What do you think?  It's less efficient but makes fixed/gp more similar.
> 
> Can you please resubmit the series based on the review feedback?
> 
> Thanks,
> 
>