Re: [RFC PATCH v3 28/58] KVM: x86/pmu: Add intel_passthrough_pmu_msrs() to pass-through PMU MSRs

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 01, 2024, Mingwei Zhang wrote:
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index 02c9019c6f85..737de5bf1eee 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -740,6 +740,52 @@ static bool intel_is_rdpmc_passthru_allowed(struct kvm_vcpu *vcpu)
>  	return true;
>  }
>  
> +/*
> + * Setup PMU MSR interception for both mediated passthrough vPMU and legacy
> + * emulated vPMU. Note that this function is called after each time userspace
> + * set CPUID.
> + */
> +static void intel_passthrough_pmu_msrs(struct kvm_vcpu *vcpu)

Function verb is misleading.  This doesn't always "passthrough" MSRs, it's also
responsible for enabling interception as needed.  intel_pmu_update_msr_intercepts()?

> +{
> +	bool msr_intercept = !is_passthrough_pmu_enabled(vcpu);
> +	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> +	int i;
> +
> +	/*
> +	 * Unexposed PMU MSRs are intercepted by default. However,
> +	 * KVM_SET_CPUID{,2} may be invoked multiple times. To ensure MSR
> +	 * interception is correct after each call of setting CPUID, explicitly
> +	 * touch msr bitmap for each PMU MSR.
> +	 */
> +	for (i = 0; i < kvm_pmu_cap.num_counters_gp; i++) {
> +		if (i >= pmu->nr_arch_gp_counters)
> +			msr_intercept = true;

Hmm, I like the idea and that y'all remembered to intercept unsupported MSRs, but
it's way, way too easy to clobber msr_intercept and fail to re-initialize across
for-loops.

Rather than update the variable mid-loop, how about this?

	for (i = 0; i < pmu->nr_arch_gp_counters; i++) {
		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PERFCTR0 + i, MSR_TYPE_RW, intercept);
		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PMC0 + i, MSR_TYPE_RW,
					  intercept || !fw_writes_is_enabled(vcpu));
	}
	for ( ; i < kvm_pmu_cap.num_counters_gp; i++) {
		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PERFCTR0 + i, MSR_TYPE_RW, true);
		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PMC0 + i, MSR_TYPE_RW, true);
	}

	for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
		vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_TYPE_RW, intercept);
	for ( ; i < kvm_pmu_cap.num_counters_fixed; i++)
		vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_TYPE_RW, true);


> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PERFCTR0 + i, MSR_TYPE_RW, msr_intercept);
> +		if (fw_writes_is_enabled(vcpu))
> +			vmx_set_intercept_for_msr(vcpu, MSR_IA32_PMC0 + i, MSR_TYPE_RW, msr_intercept);
> +		else
> +			vmx_set_intercept_for_msr(vcpu, MSR_IA32_PMC0 + i, MSR_TYPE_RW, true);
> +	}
> +
> +	msr_intercept = !is_passthrough_pmu_enabled(vcpu);
> +	for (i = 0; i < kvm_pmu_cap.num_counters_fixed; i++) {
> +		if (i >= pmu->nr_arch_fixed_counters)
> +			msr_intercept = true;
> +		vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_TYPE_RW, msr_intercept);
> +	}
> +
> +	if (pmu->version > 1 && is_passthrough_pmu_enabled(vcpu) &&

Don't open code kvm_pmu_has_perf_global_ctrl().

> +	    pmu->nr_arch_gp_counters == kvm_pmu_cap.num_counters_gp &&
> +	    pmu->nr_arch_fixed_counters == kvm_pmu_cap.num_counters_fixed)
> +		msr_intercept = false;
> +	else
> +		msr_intercept = true;

This reinforces that checking PERF_CAPABILITIES for PERF_METRICS is likely doomed
to fail, because doesn't PERF_GLOBAL_CTRL need to be intercepted, strictly speaking,
to prevent setting EN_PERF_METRICS?

> +	vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_GLOBAL_STATUS, MSR_TYPE_RW, msr_intercept);
> +	vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL, MSR_TYPE_RW, msr_intercept);
> +	vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_GLOBAL_OVF_CTRL, MSR_TYPE_RW, msr_intercept);
> +}
> +
>  struct kvm_pmu_ops intel_pmu_ops __initdata = {
>  	.rdpmc_ecx_to_pmc = intel_rdpmc_ecx_to_pmc,
>  	.msr_idx_to_pmc = intel_msr_idx_to_pmc,
> @@ -752,6 +798,7 @@ struct kvm_pmu_ops intel_pmu_ops __initdata = {
>  	.deliver_pmi = intel_pmu_deliver_pmi,
>  	.cleanup = intel_pmu_cleanup,
>  	.is_rdpmc_passthru_allowed = intel_is_rdpmc_passthru_allowed,
> +	.passthrough_pmu_msrs = intel_passthrough_pmu_msrs,
>  	.EVENTSEL_EVENT = ARCH_PERFMON_EVENTSEL_EVENT,
>  	.MAX_NR_GP_COUNTERS = KVM_INTEL_PMC_MAX_GENERIC,
>  	.MIN_NR_GP_COUNTERS = 1,
> -- 
> 2.46.0.rc1.232.g9752f9e123-goog
>