Re: [PATCH v7 03/19] KVM: x86/pmu: Remove KVM's enumeration of Intel's architectural encodings

[Liang, Kan]

On 2023-11-07 7:31 p.m., Sean Christopherson wrote:
> Drop KVM's enumeration of Intel's architectural event encodings, and
> instead open code the three encodings (of which only two are real) that
> KVM uses to emulate fixed counters.  Now that KVM doesn't incorrectly
> enforce the availability of architectural encodings, there is no reason
> for KVM to ever care about the encodings themselves, at least not in the
> current format of an array indexed by the encoding's position in CPUID.
> 
> Opportunistically add a comment to explain why KVM cares about eventsel
> values for fixed counters.
> 
> Suggested-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/vmx/pmu_intel.c | 72 ++++++++++++------------------------
>  1 file changed, 23 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index 7737ee2fc62f..c4f2c6a268e7 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -22,52 +22,6 @@
>  
>  #define MSR_PMC_FULL_WIDTH_BIT      (MSR_IA32_PMC0 - MSR_IA32_PERFCTR0)
>  
> -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_REAL_INTEL_ARCH_EVENTS,
> -
> -	/*
> -	 * Pseudo-architectural event used to implement IA32_FIXED_CTR2, a.k.a.
> -	 * TSC reference cycles.  The architectural reference cycles event may
> -	 * or may not actually use the TSC as the reference, e.g. might use the
> -	 * core crystal clock or the bus clock (yeah, "architectural").
> -	 */
> -	PSEUDO_ARCH_REFERENCE_CYCLES = NR_REAL_INTEL_ARCH_EVENTS,
> -	NR_INTEL_ARCH_EVENTS,
> -};
> -
> -static struct {
> -	u8 eventsel;
> -	u8 unit_mask;
> -} const intel_arch_events[] = {
> -	[INTEL_ARCH_CPU_CYCLES]			= { 0x3c, 0x00 },
> -	[INTEL_ARCH_INSTRUCTIONS_RETIRED]	= { 0xc0, 0x00 },
> -	[INTEL_ARCH_REFERENCE_CYCLES]		= { 0x3c, 0x01 },
> -	[INTEL_ARCH_LLC_REFERENCES]		= { 0x2e, 0x4f },
> -	[INTEL_ARCH_LLC_MISSES]			= { 0x2e, 0x41 },
> -	[INTEL_ARCH_BRANCHES_RETIRED]		= { 0xc4, 0x00 },
> -	[INTEL_ARCH_BRANCHES_MISPREDICTED]	= { 0xc5, 0x00 },
> -	[PSEUDO_ARCH_REFERENCE_CYCLES]		= { 0x00, 0x03 },
> -};
> -
> -/* mapping between fixed pmc index and intel_arch_events array */
> -static int fixed_pmc_events[] = {
> -	[0] = INTEL_ARCH_INSTRUCTIONS_RETIRED,
> -	[1] = INTEL_ARCH_CPU_CYCLES,
> -	[2] = PSEUDO_ARCH_REFERENCE_CYCLES,
> -};
> -
>  static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>  {
>  	struct kvm_pmc *pmc;
> @@ -442,8 +396,29 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	return 0;
>  }
>  
> +/*
> + * Map fixed counter events to architectural general purpose event encodings.
> + * Perf doesn't provide APIs to allow KVM to directly program a fixed counter,
> + * and so KVM instead programs the architectural event to effectively request
> + * the fixed counter.  Perf isn't guaranteed to use a fixed counter and may
> + * instead program the encoding into a general purpose counter, e.g. if a
> + * different perf_event is already utilizing the requested counter, but the end
> + * result is the same (ignoring the fact that using a general purpose counter
> + * will likely exacerbate counter contention).
> + *
> + * Note, reference cycles is counted using a perf-defined "psuedo-encoding",
> + * as there is no architectural general purpose encoding for reference cycles.

It's not the case for the latest Intel platforms anymore. Please see
ffbe4ab0beda ("perf/x86/intel: Extend the ref-cycles event to GP counters").

Maybe perf should export .event_map to KVM somehow.

Thanks,
Kan
> + */
>  static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
>  {
> +	const struct {
> +		u8 eventsel;
> +		u8 unit_mask;
> +	} fixed_pmc_events[] = {
> +		[0] = { 0xc0, 0x00 }, /* Instruction Retired / PERF_COUNT_HW_INSTRUCTIONS. */
> +		[1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
> +		[2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
> +	};
>  	int i;
>  
>  	BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);
> @@ -451,10 +426,9 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
>  	for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>  		int index = array_index_nospec(i, KVM_PMC_MAX_FIXED);
>  		struct kvm_pmc *pmc = &pmu->fixed_counters[index];
> -		u32 event = fixed_pmc_events[index];
>  
> -		pmc->eventsel = (intel_arch_events[event].unit_mask << 8) |
> -				 intel_arch_events[event].eventsel;
> +		pmc->eventsel = (fixed_pmc_events[index].unit_mask << 8) |
> +				 fixed_pmc_events[index].eventsel;
>  	}
>  }
>