Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e1003a0c-7388-9dce-3fc2-8a1f9288e23f@gmail.com>
Date:   Fri, 14 Oct 2022 16:44:47 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.2
Subject: Re: [PATCH v2 2/2] KVM: x86/pmu: Add PEBS support for SPR and future
 non-hybird models
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Jim Mattson <jmattson@google.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
References: <20220922051929.89484-1-likexu@tencent.com>
 <20220922051929.89484-2-likexu@tencent.com> <Y0W/YR6gXhunJYry@google.com>
From:   Like Xu <like.xu.linux@gmail.com>
In-Reply-To: <Y0W/YR6gXhunJYry@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/10/2022 3:09 am, Sean Christopherson wrote:
> Kind of a nit, but I would prefer a shortlog that talks about the pdit/pdir
> stuff and not a "enable PEBS" bucket.
> 
> On Thu, Sep 22, 2022, Like Xu wrote:
>> @@ -140,6 +149,16 @@ static void kvm_perf_overflow(struct perf_event *perf_event,
>>   	__kvm_perf_overflow(pmc, true);
>>   }
>>   
>> +static bool need_max_precise(struct kvm_pmc *pmc)
>> +{
>> +	if (pmc->idx == 0 && x86_match_cpu(vmx_pebs_pdist_cpu))
>> +		return true;
>> +	if (pmc->idx == 32 && x86_match_cpu(vmx_pebs_pdir_cpu))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>>   static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
>>   				  u64 config, bool exclude_user,
>>   				  bool exclude_kernel, bool intr)
>> @@ -181,11 +200,11 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
>>   		 * the accuracy of the PEBS profiling result, because the "event IP"
>>   		 * in the PEBS record is calibrated on the guest side.
>>   		 *
>> -		 * On Icelake everything is fine. Other hardware (GLC+, TNT+) that
>> +		 * On Icelake everything is fine. Other hardware (TNT+) that
>>   		 * could possibly care here is unsupported and needs changes.
> 
> This part of the comment is still somewhat stale, and for me at least it's not at
> all helpful.

SPR has GLC core, and this patch adds this support and removes the unsupported 
statements.
Not sure how else I may help you.

> 
>>   		 */
>>   		attr.precise_ip = 1;
>> -		if (x86_match_cpu(vmx_icl_pebs_cpu) && pmc->idx == 32)
>> +		if (need_max_precise(pmc))
>>   			attr.precise_ip = 3;
> 
> What about writing this as:
> 
> 		attr.precise_ip = pmc_get_pebs_precision(pmc);
> 
> (or whatever name is appropriate for "pebs_precision").

The comment says, "the difference in the software precision levels of guest and
host PEBS events will not affect the accuracy of the PEBS profiling result".

> 
> Then in the helper, add comments to explaint the magic numbers and the interaction
> with PDIST and PDIR.  Bonus points if #defines for the the magic numbers can be
> added somewher

KVM just uses "precision_ip = 3" to request hw pdit/pdir counters from host perf,
It only works at 3. For 1 or 2, there is no difference.

> 
> 				 *  0 - SAMPLE_IP can have arbitrary skid
> 				 *  1 - SAMPLE_IP must have constant skid
> 				 *  2 - SAMPLE_IP requested to have 0 skid
> 				 *  3 - SAMPLE_IP must have 0 skid
> 
> static u64 pmc_get_pebs_precision(struct kvm_pmc *pmc)
> {
> 	/* Comment that explains why PDIST/PDIR require 0 skid? */
> 	if ((pmc->idx == 0 && x86_match_cpu(vmx_pebs_pdist_cpu)) ||
> 	    (pmc->idx == 32 && x86_match_cpu(vmx_pebs_pdir_cpu)))
> 		return 3;
> 
> 	/* Comment about constant skid? */
> 	return 1;
> }
, therefore 0 or constant skid makes no difference.

> 
> 
>>   	}
>>   
>> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
>> index c5e5dfef69c7..4dc4bbe18821 100644
>> --- a/arch/x86/kvm/vmx/capabilities.h
>> +++ b/arch/x86/kvm/vmx/capabilities.h
>> @@ -398,7 +398,9 @@ static inline bool vmx_pt_mode_is_host_guest(void)
>>   
>>   static inline bool vmx_pebs_supported(void)
>>   {
>> -	return boot_cpu_has(X86_FEATURE_PEBS) && kvm_pmu_cap.pebs_ept;
>> +	return boot_cpu_has(X86_FEATURE_PEBS) &&
>> +		!boot_cpu_has(X86_FEATURE_HYBRID_CPU) &&
> 
> This belongs in a separate patch, and it should be ordered before patch 1 so that
> there's no window where KVM can see pebs_ept==1 on a hybrid CPU.
> 
> Actually, shouldn't everything in this patch land before core enabling?

Perf core enabling about SPR is already upstreamed.
KVM support is not too late either, thanks to SPR's relentless delays.

> 
>> +		kvm_pmu_cap.pebs_ept;
> 
> Please align indentation:
> 
> 	return boot_cpu_has(X86_FEATURE_PEBS) &&
> 	       !boot_cpu_has(X86_FEATURE_HYBRID_CPU) &&
> 	       kvm_pmu_cap.pebs_ept;
>