Re: [PATCH v2 17/54] KVM: x86/pmu: Always set global enable bits in passthrough mode

Mingwei Zhang
2024-05-09T00:14:07Z

On Wed, May 8, 2024 at 7:13 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, May 08, 2024, Dapeng Mi wrote:
> >
> > On 5/8/2024 12:36 PM, Mingwei Zhang wrote:
> > > if (pmu->passthrough && pmu->nr_arch_gp_counters)
> > >
> > > Since mediated passthrough PMU requires PerfMon v4 in Intel (PerfMon
> > > v2 in AMD), once it is enabled (pmu->passthrough = true), then global
> > > ctrl _must_ exist phyiscally. Regardless of whether we expose it to
> > > the guest VM, at reset time, we need to ensure enabling bits for GP
> > > counters are set (behind the screen). This is critical for AMD, since
> > > most of the guests are usually in (AMD) PerfMon v1 in which global
> > > ctrl MSR is inaccessible, but does exist and is operating in HW.
> > >
> > > Yes, if we eliminate that requirement (pmu->passthrough -> Perfmon v4
> > > Intel / Perfmon v2 AMD), then this code will have to change. However,
> > Yeah, that's what I'm worrying about. We ever discussed to support mediated
> > vPMU on HW below perfmon v4. When someone implements this, he may not
> > notice this place needs to be changed as well, this introduces a potential
> > bug and we should avoid this.

I think you might have worried too much about future problems, but
yes, things are under the radar. For Intel, this version constraint
might be ok as Perfmon v4 is skylake, which is already pretty early.

For AMD, things are slightly different, PerfMon v2 in AMD requires
Genoa, which is pretty new. So, this problem probably could be
something for AMD if they want to extend the new vPMU design to Milan,
but we will see how people think. So one potential (easy) extension
for AMD is host PerfMon v1 + guest PerfMon v1 support for mediated
passthrough vPMU.

>
> Just add a WARN on the PMU version. I haven't thought much about whether or not
> KVM should support mediated PMU for earlier hardware, but having a sanity check
> on the assumptions of this code is reasonable even if we don't _plan_ on supporting
> earlier hardware.

Sure. That sounds pretty reasonable.

Thanks.
-Mingwei