Re: [PATCH v10 16/29] KVM: selftests: Test Intel PMU architectural events on gp counters

[Sean Christopherson <seanjc@google.com>]

On Mon, Jan 15, 2024, Dapeng Mi wrote:
> 
> On 1/13/2024 5:37 AM, Sean Christopherson wrote:
> > On Fri, Jan 12, 2024, Dapeng Mi wrote:
> > > On 1/10/2024 7:02 AM, Sean Christopherson wrote:
> > > > +/*
> > > > + * If an architectural event is supported and guaranteed to generate at least
> > > > + * one "hit, assert that its count is non-zero.  If an event isn't supported or
> > > > + * the test can't guarantee the associated action will occur, then all bets are
> > > > + * off regarding the count, i.e. no checks can be done.
> > > > + *
> > > > + * Sanity check that in all cases, the event doesn't count when it's disabled,
> > > > + * and that KVM correctly emulates the write of an arbitrary value.
> > > > + */
> > > > +static void guest_assert_event_count(uint8_t idx,
> > > > +				     struct kvm_x86_pmu_feature event,
> > > > +				     uint32_t pmc, uint32_t pmc_msr)
> > > > +{
> > > > +	uint64_t count;
> > > > +
> > > > +	count = _rdpmc(pmc);
> > > > +	if (!this_pmu_has(event))
> > > > +		goto sanity_checks;
> > > > +
> > > > +	switch (idx) {
> > > > +	case INTEL_ARCH_INSTRUCTIONS_RETIRED_INDEX:
> > > > +		GUEST_ASSERT_EQ(count, NUM_INSNS_RETIRED);
> > > > +		break;
> > > > +	case INTEL_ARCH_BRANCHES_RETIRED_INDEX:
> > > > +		GUEST_ASSERT_EQ(count, NUM_BRANCHES);
> > > > +		break;
> > > > +	case INTEL_ARCH_CPU_CYCLES_INDEX:
> > > > +	case INTEL_ARCH_REFERENCE_CYCLES_INDEX:
> > > Since we already support slots event in below guest_test_arch_event(), we
> > > can add check for INTEL_ARCH_TOPDOWN_SLOTS_INDEX here.
> > Can that actually be tested at this point, since KVM doesn't support
> > X86_PMU_FEATURE_TOPDOWN_SLOTS, i.e. this_pmu_has() above should always fail, no?
> 
> I suppose X86_PMU_FEATURE_TOPDOWN_SLOTS has been supported in KVM.  The
> following output comes from a guest with latest kvm-x86 code on the Sapphire
> Rapids platform.
> 
> sudo cpuid -l 0xa
> CPU 0:
>    Architecture Performance Monitoring Features (0xa):
>       version ID                               = 0x2 (2)
>       number of counters per logical processor = 0x8 (8)
>       bit width of counter                     = 0x30 (48)
>       length of EBX bit vector                 = 0x8 (8)
>       core cycle event                         = available
>       instruction retired event                = available
>       reference cycles event                   = available
>       last-level cache ref event               = available
>       last-level cache miss event              = available
>       branch inst retired event                = available
>       branch mispred retired event             = available
>       top-down slots event                     = available
> 
> Current KVM doesn't support fixed counter 3 and pseudo slots event yet, but
> the architectural slots event is supported and can be programed on a GP
> counter. Current test code can cover this case, so I think we'd better add
> the check for the slots count.

Can you submit a patch on top, with a changelog that includes justification that
that explains exactly what assertions can be made on the top-down slots event
given the "workload" being measured?  I'm definitely not opposed to adding coverage
for top-down slots, but at this point, I don't want to respin this series, nor do
I want to make that change when applying on the fly.