Message-ID: <f7a84eb0-eb64-4cdf-801d-088bbdbce0b4@linux.intel.com>
Date: Wed, 26 Nov 2025 08:23:07 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>,
 Tianrui Zhao <zhaotianrui@...ngson.cn>, Bibo Mao <maobibo@...ngson.cn>,
 Huacai Chen <chenhuacai@...nel.org>, Anup Patel <anup@...infault.org>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Xin Li <xin@...or.com>, "H. Peter Anvin" <hpa@...or.com>,
 Andy Lutomirski <luto@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Paolo Bonzini <pbonzini@...hat.com>,
 linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
 kvm@...r.kernel.org, loongarch@...ts.linux.dev,
 kvm-riscv@...ts.infradead.org, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
 Kan Liang <kan.liang@...ux.intel.com>, Yongwei Ma <yongwei.ma@...el.com>,
 Mingwei Zhang <mizhang@...gle.com>,
 Xiong Zhang <xiong.y.zhang@...ux.intel.com>,
 Sandipan Das <sandipan.das@....com>
Subject: Re: [PATCH v5 28/44] KVM: x86/pmu: Load/save GLOBAL_CTRL via
 entry/exit fields for mediated PMU


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.

Powered by blists - more mailing lists