On 11/20/2024 2:58 AM, Sean Christopherson wrote:
> 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.
Sure. I ever noticed that this in-consistence, but "put" seem not so
intuitionistic as "save", so didn't change it.
>
> 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@xxxxxxx>
>> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
>> ---
>> 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.
Not quite sure about AMD platforms, but it seems both Intel and AMD
platforms follow below sequence to manipulated PMU MSRs.
disable PERF_GLOBAL_CTRL MSR
manipulate counter-level PMU MSR
enable PERF_GLOBAL_CTRL MSR
It seems there is no issues?
>
> 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.
Sure. Would add a callback and vendor specific code would fill the base and
shift fields by leveraging the callback.
>
