On Fri, Nov 03, 2023, Jim Mattson wrote: > On Fri, Nov 3, 2023 at 8:13 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote: > > > > > > > > On 2023-11-02 1:45 p.m., Jim Mattson wrote: > > > On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@xxxxxxxxxxxxxxx> wrote: > > >> > > >> > > >> On 11/1/2023 9:33 PM, Liang, Kan wrote: > > >>> > > >>> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote: > > >>>> On 11/1/2023 11:04 AM, Jim Mattson wrote: > > >>>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng > > >>>>> <dapeng1.mi@xxxxxxxxxxxxxxx> wrote: > > >>>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote: > > >>>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi > > >>>>>>> <dapeng1.mi@xxxxxxxxxxxxxxx> wrote: > > >>>>>>>> This patch adds support for the architectural topdown slots event > > >>>>>>>> which > > >>>>>>>> is hinted by CPUID.0AH.EBX. > > >>>>>>> Can't a guest already program an event selector to count event select > > >>>>>>> 0xa4, unit mask 1, unless the event is prohibited by > > >>>>>>> KVM_SET_PMU_EVENT_FILTER? > > >>>>>> Actually defining this new slots arch event is to do the sanity check > > >>>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX. > > >>>>>> Currently vPMU would check if the arch event from guest is supported by > > >>>>>> KVM. If not, it would be rejected just like intel_hw_event_available() > > >>>>>> shows. > > >>>>>> > > >>>>>> If we don't add the slots event in the intel_arch_events[] array, guest > > >>>>>> may program the slots event and pass the sanity check of KVM on a > > >>>>>> platform which actually doesn't support slots event and program the > > >>>>>> event on a real GP counter and got an invalid count. This is not > > >>>>>> correct. > > >>>>> On physical hardware, it is possible to program a GP counter with the > > >>>>> event selector and unit mask of the slots event whether or not the > > >>>>> platform supports it. Isn't KVM wrong to disallow something that a > > >>>>> physical CPU allows? > > >>>> > > >>>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an > > >>>> event is not supported by the hardware, we can't predict the PMU's > > >>>> behavior and a meaningless count may be returned and this could mislead > > >>>> the user. > > >>> The user can program any events on the GP counter. The perf doesn't > > >>> limit it. For the unsupported event, 0 should be returned. Please keep > > >>> in mind, the event list keeps updating. If the kernel checks for each > > >>> event, it could be a disaster. I don't think it's a flaw. > > >> > > >> > > >> Thanks Kan, it would be ok as long as 0 is always returned for > > >> unsupported events. IMO, it's a nice to have feature that KVM does this > > >> sanity check for supported arch events, it won't break anything. > > > > > > The hardware PMU most assuredly does not return 0 for unsupported events. > > > > > > For example, if I use host perf to sample event selector 0xa4 unit > > > mask 1 on a Broadwell host (406f1), I get... > > > > I think we have different understanding about the meaning of the > > "unsupported". There is no enumeration of the Architectural Topdown > > Slots, which only means the Topdown Slots/01a4 is not an architectural > > event on the platform. It doesn't mean that the event encoding is > > unsupported. It could be used by another event, especially on the > > previous platform. > > If the same event encoding could be used by a microarchitectural event > on a prior platform, then it is *definitely* wrong for KVM to refuse > to monitor the event just because it isn't enumerated as a supported > architectural event. +1000! Thanks Kan, this is exactly the info we need! I'll add a patch to build on "Always treat Fixed counters as available when supported"[*] and rip out intel_hw_event_available(). [*] https://lore.kernel.org/all/20231024002633.2540714-4-seanjc@xxxxxxxxxx
