Re: [PATCH v5 28/44] KVM: x86/pmu: Load/save GLOBAL_CTRL via entry/exit fields for mediated PMU

Mi, Dapeng
2025-11-25T05:02:56Z

On 11/25/2025 9:48 AM, Sean Christopherson wrote:
> On Wed, Aug 06, 2025, Sean Christopherson wrote:
>> From: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>
>> When running a guest with a mediated PMU, context switch PERF_GLOBAL_CTRL
>> via the dedicated VMCS fields for both host and guest. For the host,
>> always zero GLOBAL_CTRL on exit as the guest's state will still be loaded
>> in hardware (KVM will context switch the bulk of PMU state outside of the
>> inner run loop). For the guest, use the dedicated fields to atomically
>> load and save PERF_GLOBAL_CTRL on all entry/exits.
>>
>> Note, VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL was introduced by Sapphire
>> Rapids, and is expected to be supported on all CPUs with PMU v4+. WARN if
>> that expectation is not met. Alternatively, KVM could manually save
>> PERF_GLOBAL_CTRL via the MSR save list, but the associated complexity and
>> runtime overhead is unjustified given that the feature should always be
>> available on relevant CPUs.
> This is wrong, PMU v4 has been supported since Skylake.

Yes, the v4+ restriction is to meet the requirement of existence of
IA32_PERF_GLOBAL_STATUS_SET MSR which is needed to restore the guest
global_ctrl.

>
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index 7ab35ef4a3b1..98f7b45ea391 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -787,7 +787,23 @@ static bool intel_pmu_is_mediated_pmu_supported(struct x86_pmu_capability *host_
>> * Require v4+ for MSR_CORE_PERF_GLOBAL_STATUS_SET, and full-width
>> * writes so that KVM can precisely load guest counter values.
>> */
>> - return host_pmu->version >= 4 && host_perf_cap & PERF_CAP_FW_WRITES;
>> + if (host_pmu->version < 4 || !(host_perf_cap & PERF_CAP_FW_WRITES))
>> + return false;
>> +
>> + /*
>> + * All CPUs that support a mediated PMU are expected to support loading
>> + * and saving PERF_GLOBAL_CTRL via dedicated VMCS fields.
>> + */
>> + if (WARN_ON_ONCE(!cpu_has_load_perf_global_ctrl() ||
>> + !cpu_has_save_perf_global_ctrl()))
>> + return false;
> And so this WARN fires due to cpu_has_save_perf_global_ctrl() being false. The
> bad changelog is mine, but the code isn't entirely my fault. I did suggest the
> WARN in v3[1], probably because I forgot when PMU v4 was introduced and no one
> corrected me.
>
> v4 of the series[2] then made cpu_has_save_perf_global_ctrl() a hard requirement,
> based on my miguided feedback.
>
> * Only support GLOBAL_CTRL save/restore with VMCS exec_ctrl, drop the MSR
> save/retore list support for GLOBAL_CTRL, thus the support of mediated
> vPMU is constrained to SapphireRapids and later CPUs on Intel side.
>
> Doubly frustrating is that this was discussed in the original RFC, where Jim
> pointed out[3] that requiring VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL would prevent
> enabling the mediated PMU on Skylake+, and I completely forgot that conversation
> by the time v3 of the series rolled around :-(

VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL is introduced from SPR and later. I
remember the original requirements includes to support Skylake and Icelake,
but I ever thought there were some offline sync and the requirement changed...

My bad, I should double confirm this at then.

>
> As mentioned in the discussion with Jim, _if_ PMU v4 was introduced with ICX (or
> later), then I'd be in favor of making VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL a hard
> requirement. But losing supporting Skylake+ is a bit much.
>
> There are a few warts with nVMX's use of the auto-store list that need to be
> cleaned up, but on the plus side it's also a good excuse to clean up
> {add,clear}_atomic_switch_msr(), which have accumulated some cruft and quite a
> bit of duplicate code. And while I still dislike using the auto-store list, the
> code isn't as ugly as it was back in v3 because we _can_ make the "load" VMCS
> controls mandatory without losing support for any CPUs (they predate PMU v4).

Yes, xxx_atomic_switch_msr() helpers need to be cleaned up and optimized. I
suppose we can have an independent patch-set to clean up and support
global_ctrl with auto-store list for Skylake and Icelake.

>
> [1] https://lore.kernel.org/all/ZzyWKTMdNi5YjvEM@google.com
> [2] https://lore.kernel.org/all/20250324173121.1275209-1-mizhang@google.com
> [3] https://lore.kernel.org/all/CALMp9eQ+-wcj8QMmFR07zvxFF22-bWwQgV-PZvD04ruQ=0NBBA@mail.gmail.com