Re: [PATCH] KVM: x86/pmu: Add PEBS support for Intel Sapphire Rapids

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2022-09-21 2:48 a.m., Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> Virtualization support for SPR PEBS has officially available in the
> Intel SDM (June 2022) and has been validated on late stepping machines:
> 
> Compared to Ice Lake Server, the PDIR counter available (Fixed 0) on SPR
> is unchanged, but the capability is enhanced to Instruction-Accurate PDIR
> (PDIR++), where PEBS is taken on the next instruction after the one that
> caused the overflow. Also, it introduces a new Precise Distribution (PDist)
> facility that eliminates the skid when a precise event is programmed
> on general programmable counter 0.
> 
> For guest usage, KVM will raise attr.precise_ip to 3 in both cases
> mentioned above, requesting the correct hardware counter (PRIR++
> or PDist) from the perf sub-system on the host as usual.
> 
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/events/intel/core.c |  1 +
>  arch/x86/kvm/pmu.c           | 17 ++++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 2db93498ff71..804540ba4599 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -6288,6 +6288,7 @@ __init int intel_pmu_init(void)
>  		x86_pmu.pebs_constraints = intel_spr_pebs_event_constraints;
>  		x86_pmu.extra_regs = intel_spr_extra_regs;
>  		x86_pmu.limit_period = spr_limit_period;
> +		x86_pmu.pebs_ept = 1;
>  		x86_pmu.pebs_aliases = NULL;
>  		x86_pmu.pebs_prec_dist = true;
>  		x86_pmu.pebs_block = true;


I think the perf patch should be a separate patch.

According to the SDM, the EPT-friendly PEBS is supported by all the
platforms after ICX and ADL.

Besides the SPR, I think we should further patch the future platforms
with PEBS format 5 as below as well, to avoid patching every future model.

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 3ece4ab54d2c..4608d5821e38 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -2333,8 +2333,10 @@ void __init intel_ds_init(void)
 			x86_pmu.large_pebs_flags |= PERF_SAMPLE_TIME;
 			break;

-		case 4:
 		case 5:
+			x86_pmu.pebs_ept = 1;
+			fallthrough;
+		case 4:
 			x86_pmu.drain_pebs = intel_pmu_drain_pebs_icl;
 			x86_pmu.pebs_record_size = sizeof(struct pebs_basic);
 			if (x86_pmu.intel_cap.pebs_baseline) {




> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 02f9e4f245bd..81e9d7c2332d 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -28,9 +28,18 @@
>  struct x86_pmu_capability __read_mostly kvm_pmu_cap;
>  EXPORT_SYMBOL_GPL(kvm_pmu_cap);
>  
> -static const struct x86_cpu_id vmx_icl_pebs_cpu[] = {
> +/* Precise Distribution of Instructions Retired (PDIR) */
> +static const struct x86_cpu_id vmx_pebs_pdir_cpu[] = {
>  	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D, NULL),
>  	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, NULL),
> +	/* Instruction-Accurate PDIR (PDIR++) */
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
> +	{}
> +};
> +
> +/* Precise Distribution (PDist) */
> +static const struct x86_cpu_id vmx_pebs_pdist_cpu[] = {
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
>  	{}
>  };
>  
> @@ -181,12 +190,14 @@ 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.
>  		 */
>  		attr.precise_ip = 1;
> -		if (x86_match_cpu(vmx_icl_pebs_cpu) && pmc->idx == 32)
> +		if ((pmc->idx == 32 && x86_match_cpu(vmx_pebs_pdir_cpu)) ||
> +		    (pmc->idx == 0 && x86_match_cpu(vmx_pebs_pdist_cpu))) {


It may be better to move the check into a function. There may be other
conditions that require the max precise level later. Something like below.
	if (need_max_precise(pmc))

Thanks,
Kan

>  			attr.precise_ip = 3;
> +		}
>  	}
>  
>  	event = perf_event_create_kernel_counter(&attr, -1, current,