Messages in this thread | | | Date | Fri, 19 Nov 2021 15:16:57 +0800 | From | Like Xu <> | Subject | Re: [PATCH 3/4] KVM: x86/pmu: Reuse find_perf_hw_id() and drop find_fixed_event() |
| |
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, > >
| |