On Wed, Apr 19, 2023, Like Xu wrote:
> Jim, sorry for the late reply.
>
> On 11/4/2023 10:58 pm, Jim Mattson wrote:
> > > > > Jim, does this help you or could you explain more about your confusion ?
> > > >
> > > > You say that "fewer than four counters can lead to guest instability
> > > > as software expects four counters to be available." Your solution is
> > > > to disable the PMU, which leaves zero counters available. Zero is less
> > > > than four. Hence, by your claim, disabling the PMU can lead to guest
> > > > instability. I don't see how this is an improvement over one, two, or
> > > > three counters.
KVM can't do the right thing regardless. I would rather have KVM explicitly tell
userspace via that it can't support a vPMU than to carry on with a bogus and
unexpected setup.
> > Does this actually guarantee that the requisite number of counters are
> > available and will always be available while the guest is running?
>
> Not 100%, the scheduling of physical counters depends on the host perf scheduler.
Or put differently, the same thing that happens on Intel. kvm_pmu_cap.num_counters_gp
is the number of counters reported by perf when KVM loads, i.e. barring oddities,
it's the number of counters present in the host. Most importantly, if perf doesn't
find the expected number of counters, perf will bail and use software only events,
and then clear all of x86_pmu.
In other words, KVM's new sanity *should* be a nop with respect to current
behavior. If we're concerned about "unnecessarily" hiding the PMU when there are
1-3 counters, I'd be ok with a WARN_ON_ONCE().
Actually, looking more closely, there's unaddressed feedback from v4[*]. Folding
that in, we can enable the sanity check for both Intel and AMD, though that's a
bit of a lie since Intel will be '1'. But the code looks pretty!
if (enable_pmu) {
perf_get_x86_pmu_capability(&kvm_pmu_cap);
/*
* WARN if perf did NOT disable hardware PMU if the number of
* architecturally required GP counters aren't present, i.e. if
* there are a non-zero number of counters, but fewer than what
* is architecturally required.
*/
if (!kvm_pmu_cap.num_counters_gp ||
WARN_ON_ONCE(kvm_pmu_cap.num_counters_gp < pmu_ops->MIN_NR_GP_COUNTERS))
enable_pmu = false;
else if (is_intel && !kvm_pmu_cap.version)
enable_pmu = false;
}
if (!enable_pmu) {
memset(&kvm_pmu_cap, 0, sizeof(kvm_pmu_cap));
return;
}
[*] https://lore.kernel.org/all/ZC9ijgZBaz6p1ecw@xxxxxxxxxx
