Re: [PATCH v6 09/20] KVM: selftests: Add pmu.h and lib/pmu.c for common PMU assets

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sean Christopherson <seanjc@xxxxxxxxxx> 于2023年11月7日周二 04:40写道:
>
> On Mon, Nov 06, 2023, JinrongLiang wrote:
> > 在 2023/11/4 21:20, Jim Mattson 写道:
> > > > diff --git a/tools/testing/selftests/kvm/include/pmu.h b/tools/testing/selftests/kvm/include/pmu.h
> > > > new file mode 100644
> > > > index 000000000000..987602c62b51
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/kvm/include/pmu.h
> > > > @@ -0,0 +1,84 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * Copyright (C) 2023, Tencent, Inc.
> > > > + */
> > > > +#ifndef SELFTEST_KVM_PMU_H
> > > > +#define SELFTEST_KVM_PMU_H
> > > > +
> > > > +#include <stdint.h>
> > > > +
> > > > +#define X86_PMC_IDX_MAX                                64
> > > > +#define INTEL_PMC_MAX_GENERIC                          32
> > >
> > > I think this is actually 15. Note that IA32_PMC0 through IA32_PMC7
> > > have MSR indices from 0xc1 through 0xc8, and MSR 0xcf is
> > > IA32_CORE_CAPABILITIES. At the very least, we have to handle
> > > non-contiguous MSR indices if we ever go beyond IA32_PMC14.
>
> There's no reason to define this, it's not used in selftests.
>
> > > > +#define KVM_PMU_EVENT_FILTER_MAX_EVENTS                300
> > > > +
> > > > +#define GP_COUNTER_NR_OFS_BIT                          8
> > > > +#define EVENT_LENGTH_OFS_BIT                           24
> > > > +
> > > > +#define PMU_VERSION_MASK                               GENMASK_ULL(7, 0)
> > > > +#define EVENT_LENGTH_MASK                              GENMASK_ULL(31, EVENT_LENGTH_OFS_BIT)
> > > > +#define GP_COUNTER_NR_MASK                             GENMASK_ULL(15, GP_COUNTER_NR_OFS_BIT)
> > > > +#define FIXED_COUNTER_NR_MASK                          GENMASK_ULL(4, 0)
>
> These are also unneeded, they're superseded by CPUID properties.
>
> > > > +#define ARCH_PERFMON_EVENTSEL_EVENT                    GENMASK_ULL(7, 0)
> > > > +#define ARCH_PERFMON_EVENTSEL_UMASK                    GENMASK_ULL(15, 8)
> > > > +#define ARCH_PERFMON_EVENTSEL_USR                      BIT_ULL(16)
> > > > +#define ARCH_PERFMON_EVENTSEL_OS                       BIT_ULL(17)
> > > > +#define ARCH_PERFMON_EVENTSEL_EDGE                     BIT_ULL(18)
> > > > +#define ARCH_PERFMON_EVENTSEL_PIN_CONTROL              BIT_ULL(19)
> > > > +#define ARCH_PERFMON_EVENTSEL_INT                      BIT_ULL(20)
> > > > +#define ARCH_PERFMON_EVENTSEL_ANY                      BIT_ULL(21)
> > > > +#define ARCH_PERFMON_EVENTSEL_ENABLE                   BIT_ULL(22)
> > > > +#define ARCH_PERFMON_EVENTSEL_INV                      BIT_ULL(23)
> > > > +#define ARCH_PERFMON_EVENTSEL_CMASK                    GENMASK_ULL(31, 24)
> > > > +
> > > > +#define PMC_MAX_FIXED                                  16
>
> Also unneeded.
>
> > > > +#define PMC_IDX_FIXED                                  32
>
> This one is absolutely ridiculous.  It's the shift for the enable bit in global
> control, which is super obvious from the name. /s
>
> > > > +
> > > > +/* RDPMC offset for Fixed PMCs */
> > > > +#define PMC_FIXED_RDPMC_BASE                           BIT_ULL(30)
> > > > +#define PMC_FIXED_RDPMC_METRICS                        BIT_ULL(29)
> > > > +
> > > > +#define FIXED_BITS_MASK                                0xFULL
> > > > +#define FIXED_BITS_STRIDE                              4
> > > > +#define FIXED_0_KERNEL                                 BIT_ULL(0)
> > > > +#define FIXED_0_USER                                   BIT_ULL(1)
> > > > +#define FIXED_0_ANYTHREAD                              BIT_ULL(2)
> > > > +#define FIXED_0_ENABLE_PMI                             BIT_ULL(3)
> > > > +
> > > > +#define fixed_bits_by_idx(_idx, _bits)                 \
> > > > +       ((_bits) << ((_idx) * FIXED_BITS_STRIDE))
>
> *sigh*  And now I see where the "i * 4" stuff in the new test comes from.  My
> plan is to redo the above as:
>
> /* RDPMC offset for Fixed PMCs */
> #define FIXED_PMC_RDPMC_METRICS                 BIT_ULL(29)
> #define FIXED_PMC_RDPMC_BASE                    BIT_ULL(30)
>
> #define FIXED_PMC_GLOBAL_CTRL_ENABLE(_idx)      BIT_ULL((32 + (_idx)))
>
> #define FIXED_PMC_KERNEL                        BIT_ULL(0)
> #define FIXED_PMC_USER                          BIT_ULL(1)
> #define FIXED_PMC_ANYTHREAD                     BIT_ULL(2)
> #define FIXED_PMC_ENABLE_PMI                    BIT_ULL(3)
> #define FIXED_PMC_NR_BITS                       4
> #define FIXED_PMC_CTRL(_idx, _val)              ((_val) << ((_idx) * FIXED_PMC_NR_BITS))
>
> > > > +#define AMD64_NR_COUNTERS                              4
> > > > +#define AMD64_NR_COUNTERS_CORE                         6
>
> These too can be dropped for now.
>
> > > > +#define PMU_CAP_FW_WRITES                              BIT_ULL(13)
> > > > +#define PMU_CAP_LBR_FMT                                0x3f
> > > > +
> > > > +enum intel_pmu_architectural_events {
> > > > +       /*
> > > > +        * The order of the architectural events matters as support for each
> > > > +        * event is enumerated via CPUID using the index of the event.
> > > > +        */
> > > > +       INTEL_ARCH_CPU_CYCLES,
> > > > +       INTEL_ARCH_INSTRUCTIONS_RETIRED,
> > > > +       INTEL_ARCH_REFERENCE_CYCLES,
> > > > +       INTEL_ARCH_LLC_REFERENCES,
> > > > +       INTEL_ARCH_LLC_MISSES,
> > > > +       INTEL_ARCH_BRANCHES_RETIRED,
> > > > +       INTEL_ARCH_BRANCHES_MISPREDICTED,
> > > > +       NR_INTEL_ARCH_EVENTS,
> > > > +};
> > > > +
> > > > +enum amd_pmu_k7_events {
> > > > +       AMD_ZEN_CORE_CYCLES,
> > > > +       AMD_ZEN_INSTRUCTIONS,
> > > > +       AMD_ZEN_BRANCHES,
> > > > +       AMD_ZEN_BRANCH_MISSES,
> > > > +       NR_AMD_ARCH_EVENTS,
> > > > +};
> > > > +
> > > > +extern const uint64_t intel_pmu_arch_events[];
> > > > +extern const uint64_t amd_pmu_arch_events[];
> > >
> > > AMD doesn't define *any* architectural events. Perhaps
> > > amd_pmu_zen_events[], though who knows what Zen5 and  beyond will
> > > bring?
> > >
> > > > +extern const int intel_pmu_fixed_pmc_events[];
> > > > +
> > > > +#endif /* SELFTEST_KVM_PMU_H */
> > > > diff --git a/tools/testing/selftests/kvm/lib/pmu.c b/tools/testing/selftests/kvm/lib/pmu.c
> > > > new file mode 100644
> > > > index 000000000000..27a6c35f98a1
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/kvm/lib/pmu.c
> > > > @@ -0,0 +1,28 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright (C) 2023, Tencent, Inc.
> > > > + */
> > > > +
> > > > +#include <stdint.h>
> > > > +
> > > > +#include "pmu.h"
> > > > +
> > > > +/* Definitions for Architectural Performance Events */
> > > > +#define ARCH_EVENT(select, umask) (((select) & 0xff) | ((umask) & 0xff) << 8)
> > >
> > > There's nothing architectural about this. Perhaps RAW_EVENT() for
> > > consistency with perf?
>
> Works for me.
>
> > > > +const uint64_t intel_pmu_arch_events[] = {
> > > > +       [INTEL_ARCH_CPU_CYCLES]                 = ARCH_EVENT(0x3c, 0x0),
> > > > +       [INTEL_ARCH_INSTRUCTIONS_RETIRED]       = ARCH_EVENT(0xc0, 0x0),
> > > > +       [INTEL_ARCH_REFERENCE_CYCLES]           = ARCH_EVENT(0x3c, 0x1),
> > > > +       [INTEL_ARCH_LLC_REFERENCES]             = ARCH_EVENT(0x2e, 0x4f),
> > > > +       [INTEL_ARCH_LLC_MISSES]                 = ARCH_EVENT(0x2e, 0x41),
> > > > +       [INTEL_ARCH_BRANCHES_RETIRED]           = ARCH_EVENT(0xc4, 0x0),
> > > > +       [INTEL_ARCH_BRANCHES_MISPREDICTED]      = ARCH_EVENT(0xc5, 0x0),
> > >
> > > [INTEL_ARCH_TOPDOWN_SLOTS] = ARCH_EVENT(0xa4, 1),
>
> ...
>
> > > > @@ -63,7 +50,6 @@
> > > >
> > > >   #define AMD_ZEN_BR_RETIRED EVENT(0xc2, 0)
> > >
> > > Now AMD_ZEN_BRANCHES, above?
> >
> > Yes, I forgot to replace INTEL_BR_RETIRED, AMD_ZEN_BR_RETIRED and
> > INST_RETIRED in pmu_event_filter_test.c and remove their macro definitions.
>
> Having to go through an array to get a hardcoded value is silly, e.g. it makes
> it unnecessarily difficult to reference the encodings because they aren't simple
> literals.
>
> My vote is this:
>
> #define INTEL_ARCH_CPU_CYCLES                   RAW_EVENT(0x3c, 0x00)
> #define INTEL_ARCH_INSTRUCTIONS_RETIRED         RAW_EVENT(0xc0, 0x00)
> #define INTEL_ARCH_REFERENCE_CYCLES             RAW_EVENT(0x3c, 0x01)
> #define INTEL_ARCH_LLC_REFERENCES               RAW_EVENT(0x2e, 0x4f)
> #define INTEL_ARCH_LLC_MISSES                   RAW_EVENT(0x2e, 0x41)
> #define INTEL_ARCH_BRANCHES_RETIRED             RAW_EVENT(0xc4, 0x00)
> #define INTEL_ARCH_BRANCHES_MISPREDICTED        RAW_EVENT(0xc5, 0x00)
> #define INTEL_ARCH_TOPDOWN_SLOTS                RAW_EVENT(0xa4, 0x01)
>
> #define AMD_ZEN_CORE_CYCLES                     RAW_EVENT(0x76, 0x00)
> #define AMD_ZEN_INSTRUCTIONS_RETIRED            RAW_EVENT(0xc0, 0x00)
> #define AMD_ZEN_BRANCHES_RETIRED                RAW_EVENT(0xc2, 0x00)
> #define AMD_ZEN_BRANCHES_MISPREDICTED           RAW_EVENT(0xc3, 0x00)
>
> /*
>  * Note!  The order and thus the index of the architectural events matters as
>  * support for each event is enumerated via CPUID using the index of the event.
>  */
> enum intel_pmu_architectural_events {
>         INTEL_ARCH_CPU_CYCLES_INDEX,
>         INTEL_ARCH_INSTRUCTIONS_RETIRED_INDEX,
>         INTEL_ARCH_REFERENCE_CYCLES_INDEX,
>         INTEL_ARCH_LLC_REFERENCES_INDEX,
>         INTEL_ARCH_LLC_MISSES_INDEX,
>         INTEL_ARCH_BRANCHES_RETIRED_INDEX,
>         INTEL_ARCH_BRANCHES_MISPREDICTED_INDEX,
>         INTEL_ARCH_TOPDOWN_SLOTS_INDEX,
>         NR_INTEL_ARCH_EVENTS,
> };
>
> enum amd_pmu_zen_events {
>         AMD_ZEN_CORE_CYCLES_INDEX,
>         AMD_ZEN_INSTRUCTIONS_INDEX,
>         AMD_ZEN_BRANCHES_INDEX,
>         AMD_ZEN_BRANCH_MISSES_INDEX,
>         NR_AMD_ZEN_EVENTS,
> };
>
> extern const uint64_t intel_pmu_arch_events[];
> extern const uint64_t amd_pmu_zen_events[];
>
> ...
>
>
> const uint64_t intel_pmu_arch_events[] = {
>         INTEL_ARCH_CPU_CYCLES,
>         INTEL_ARCH_INSTRUCTIONS_RETIRED,
>         INTEL_ARCH_REFERENCE_CYCLES,
>         INTEL_ARCH_LLC_REFERENCES,
>         INTEL_ARCH_LLC_MISSES,
>         INTEL_ARCH_BRANCHES_RETIRED,
>         INTEL_ARCH_BRANCHES_MISPREDICTED,
>         INTEL_ARCH_TOPDOWN_SLOTS,
> };
> kvm_static_assert(ARRAY_SIZE(intel_pmu_arch_events) == NR_INTEL_ARCH_EVENTS);
>
> const uint64_t amd_pmu_zen_events[] = {
>         AMD_ZEN_CORE_CYCLES,
>         AMD_ZEN_INSTRUCTIONS_RETIRED,
>         AMD_ZEN_BRANCHES_RETIRED,
>         AMD_ZEN_BRANCHES_MISPREDICTED,
> };
> kvm_static_assert(ARRAY_SIZE(amd_pmu_zen_events) == NR_AMD_ZEN_EVENTS);

LGTM, thanks.





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux