Received-SPF: pass (google.com: domain of linux-kernel+bounces-45625-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <d24dc389-8e73-4a7a-9970-1022dcbfa39c@linux.intel.com>
Date: Wed, 31 Jan 2024 10:11:20 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 16/29] KVM: selftests: Test Intel PMU architectural
 events on gp counters
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
 linux-kernel@vger.kernel.org, Kan Liang <kan.liang@linux.intel.com>,
 Jim Mattson <jmattson@google.com>, Jinrong Liang <cloudliang@tencent.com>,
 Aaron Lewis <aaronlewis@google.com>, Like Xu <likexu@tencent.com>
References: <20240109230250.424295-1-seanjc@google.com>
 <20240109230250.424295-17-seanjc@google.com>
 <5f51fda5-bc07-42ac-a723-d09d90136961@linux.intel.com>
 <ZaGxNsrf_pUHkFiY@google.com>
 <cce0483f-539b-4be3-838d-af0ec91db8f0@linux.intel.com>
 <ZbmF9eM84cQhdvGf@google.com>
Content-Language: en-US
From: "Mi, Dapeng" <dapeng1.mi@linux.intel.com>
In-Reply-To: <ZbmF9eM84cQhdvGf@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 1/31/2024 7:27 AM, Sean Christopherson wrote:
> 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.

Yeah, I'm glad to submit a patch for this. :)

BTW, I have a patch series to do the bug fixes and improvements for 
kvm-unit-tests/pmu test. (some improvement ideas come from this patchset.)

https://lore.kernel.org/kvm/20240103031409.2504051-1-dapeng1.mi@linux.intel.com/

Could you please kindly review them? Thanks.

>