linux-kernel - Re: [PATCH V5 05/10] KVM: x86/pmu: Disable vPMU if the minimum num of counters isn't met

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 29 May 2023 22:25:22 +0800
From:   Like Xu <like.xu.linux@...il.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Jim Mattson <jmattson@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V5 05/10] KVM: x86/pmu: Disable vPMU if the minimum num of
 counters isn't met

On 25/5/2023 7:37 am, Sean Christopherson wrote:
> 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!

The below diff looks good to me. Please confirm that the other patches are in 
good shape
so that we can iterate on the next version. Thanks.

> 
> 	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@google.com