Re: [RFC PATCH v3 31/58] KVM: x86/pmu: Add counter MSR and selector MSR index into struct kvm_pmc

[Sean Christopherson <seanjc@google.com>]

Please squash this with the patch that does the actual save/load.  Hmm, maybe it
should be put/load, now that I think about it more?  That's more consitent with
existing KVM terminology.

Anyways, please squash them together, it's very difficult to review them separately.

On Thu, Aug 01, 2024, Mingwei Zhang wrote:
> Add the MSR indices for both selector and counter in each kvm_pmc. Giving
> convenience to mediated passthrough vPMU in scenarios of querying MSR from
> a given pmc. Note that legacy vPMU does not need this because it never
> directly accesses PMU MSRs, instead each kvm_pmc is bound to a perf_event.
> 
> For actual Zen 4 and later hardware, it will never be the case that the
> PerfMonV2 CPUID bit is set but the PerfCtrCore bit is not. However, a
> guest can be booted with PerfMonV2 enabled and PerfCtrCore disabled.
> KVM does not clear the PerfMonV2 bit from guest CPUID as long as the
> host has the PerfCtrCore capability.
> 
> In this case, passthrough mode will use the K7 legacy MSRs to program
> events but with the incorrect assumption that there are 6 such counters
> instead of 4 as advertised by CPUID leaf 0x80000022 EBX. The host kernel
> will also report unchecked MSR accesses for the absent counters while
> saving or restoring guest PMU contexts.
> 
> Ensure that K7 legacy MSRs are not used as long as the guest CPUID has
> either PerfCtrCore or PerfMonV2 set.
> 
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/svm/pmu.c          | 13 +++++++++++++
>  arch/x86/kvm/vmx/pmu_intel.c    | 13 +++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 4b3ce6194bdb..603727312f9c 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -522,6 +522,8 @@ struct kvm_pmc {
>  	 */
>  	u64 emulated_counter;
>  	u64 eventsel;
> +	u64 msr_counter;
> +	u64 msr_eventsel;

There's no need to track these per PMC, the tracking can be per PMU, e.g.

	u64 gp_eventsel_base;
	u64 gp_counter_base;
	u64 gp_shift;
	u64 fixed_base;

Actually, there's no need for a per-PMU fixed base, as that can be shoved into
kvm_pmu_ops.  LOL, and the upcoming patch hardcodes INTEL_PMC_FIXED_RDPMC_BASE.
Naughty, naughty ;-)

It's not pretty, but 16 bytes per PMC isn't trivial. 

Hmm, actually, scratch all that.  A better alternative would be to provide a
helper to put/load counter/selector MSRs, and call that from vendor code.  Ooh,
I think there's a bug here.  On AMD, the guest event selector MSRs need to be
loaded _before_ PERF_GLOBAL_CTRL, no?  I.e. enable the guest's counters only
after all selectors have been switched AMD64_EVENTSEL_GUESTONLY.  Otherwise there
would be a brief window where KVM could incorrectly enable counters in the host.
And the reverse that for put().

But Intel has the opposite ordering, because MSR_CORE_PERF_GLOBAL_CTRL needs to
be cleared before changing event selectors.

And so trying to handle this entirely in common code, while noble, is at best
fragile and at worst buggy.

The common helper can take the bases and shift, and if we want to have it handle
fixed counters, the base for that too.