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

Mi, Dapeng
2025-11-26T00:23:17Z

On 11/26/2025 1:08 AM, Sean Christopherson wrote:
> On Tue, Nov 25, 2025, Dapeng Mi wrote:
>> On 11/25/2025 9:48 AM, Sean Christopherson wrote:
>>>> + 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...
> Two things:
>
> 1) Upstream's "requirements" are not the same as Google's requirements (or those
> of any company/individual). Upstream most definitely is influenced by the
> needs and desires of end users, but ultimately the decision to do something
> (or not) is one that needs to be made by the upstream community.
>
> 2) Decisions made off-list need to be summarized and communicated on-list,
> especially in cases like this where it's a relatively minor detail in a
> large series/feature, and thus easy to overlook.
>
> I'll follow-up internally to make sure these points are well-understood by Google
> folks as well (at least, those working on KVM).

Understood and would follow.

>
>> My bad,
> Eh, this was a group "effort". I'm as much to blame as anyone else.
>
>> I should double confirm this at then.
> No need, as above, Google's requirements (assuming the requirements you're referring
> to are coming from Google people) are effectively just one data point. At this
> point, I want to drive the decision to support Sylake+ (or not) purely through
> discussion of upstream patches.
>
>>> 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.
> I have the code written (I wanted to see how much complexity it would add before
> re-opening this discussion). My plan is to put the Skylake+ support at the end
> of the series, not a separate series, so that it can be reviewed in one shot.
> E.g. if we can make a change in the "main" series that would simplify Skylake+
> support, then I'd prefer to find and implement any such change right away.

Sure. Thanks.