From: Like Xu <like.xu.linux@gmail.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Sandipan Das <sandipan.das@amd.com>,
Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
Date: Mon, 6 Feb 2023 15:53:00 +0800 [thread overview]
Message-ID: <c7e0a5b1-0fd0-e2b5-20ca-fc86a1d883db@gmail.com> (raw)
In-Reply-To: <Y9BzYzEjAwUA+wuy@google.com>
On 25/1/2023 8:10 am, Sean Christopherson wrote:
> On Fri, Nov 11, 2022, Like Xu wrote:
> On Fri, Nov 11, 2022, Like Xu wrote:
>> @@ -162,20 +179,42 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
>> {
>> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>> + struct kvm_cpuid_entry2 *entry;
>> + union cpuid_0x80000022_ebx ebx;
>>
>> - if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
>> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
>> + pmu->version = 1;
>> + if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2) &&
>
> Why check kvm_cpu_cap support? I.e. what will go wrong if userspace enumerates
> PMU v2 to the guest without proper hardware/KVM support.
>
> If this is _necessary_ to protect the host kernel, then we should probably have
> a helper to query PMU features, e.g.
>
> static __always_inline bool guest_pmu_has(struct kvm_vcpu *vcpu,
> unsigned int x86_feature)
> {
> return kvm_cpu_cap_has(x86_feature) &&
> guest_cpuid_has(vcpu, x86_feature);
> }
>
>
>
>> + guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2)) {
>> + pmu->version = 2;
>> + entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
>> + ebx.full = entry->ebx;
>> + pmu->nr_arch_gp_counters = min3((unsigned int)ebx.split.num_core_pmc,
>> + (unsigned int)kvm_pmu_cap.num_counters_gp,
>> + (unsigned int)KVM_AMD_PMC_MAX_GENERIC);
>
> Blech. This really shouldn't be necessary, KVM should tweak kvm_pmu_cap.num_counters_gp
> as needed during initialization to ensure num_counters_gp doesn't exceed KVM's
> internal limits.
>
> Posted a patch[*], please take a look. As mentioned in that thread, I'll somewhat
> speculatively apply that series sooner than later so that you can use it a base
> for this series (assuming the patch isn't busted).
>
> [*] https://lore.kernel.org/all/20230124234905.3774678-2-seanjc@google.com
>
>> + }
>> +
>> + /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
>
> Please expand this comment. I'm still not entirely sure I've interpreted it correctly,
> and I'm not sure that I agree with the code.
In the first version [1], I used almost the same if-elif-else sequence
but the concerns from JimM[2] has changed my mind:
"Nonetheless, for compatibility with old software, Fn8000_0022_EBX can never
report less than four counters (or six, if Fn8000_0001_ECX[PerfCtrExtCore] is set)."
Both in amd_pmu_refresh() and in __do_cpuid_func(), KVM implements
this using the override approach of first applying the semantics of
AMD_PMU_V2 and then implementing a minimum number of counters
supported based on whether or not guest have PERFCTR_CORE,
the proposed if-elif-else does not fulfill this need.
[1] 20220905123946.95223-4-likexu@tencent.com/
[2] CALMp9eQObuiJGV=YrAU9Fw+KoXfJtZMJ-KUs-qCOVd+R9zGBpw@mail.gmail.com
>
>> + if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE) &&
>
> AFAICT, checking kvm_cpu_cap_has() is an unrelated change. Either it's a bug fix
> and belongs in a separate patch, or it's unnecessary and should be dropped.
>
>> + guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
>> + pmu->nr_arch_gp_counters = max_t(unsigned int,
>> + pmu->nr_arch_gp_counters,
>> + AMD64_NUM_COUNTERS_CORE);
>> else
>> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
>> + pmu->nr_arch_gp_counters = max_t(unsigned int,
>> + pmu->nr_arch_gp_counters,
>> + AMD64_NUM_COUNTERS);
>
> Using max() doesn't look right. E.g. if KVM ends up running on some odd setup
> where ebx.split.num_core_pmc/kvm_pmu_cap.num_counters_gp is less than
> AMD64_NUM_COUNTERS_CORE or AMD64_NUM_COUNTERS.
>
> Or more likely, if userspace says "only expose N counters to this guest".
>
> Shouldn't this be something like?
>
> if (guest_cpuid_has(vcpu, X86_FEATURE_AMD_PMU_V2))
> pmu->nr_arch_gp_counters = min(ebx.split.num_core_pmc,
> kvm_pmu_cap.num_counters_gp);
> else if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
> pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
> else
> pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERSE;
>
next prev parent reply other threads:[~2023-02-06 7:54 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-11 10:26 [PATCH v3 0/8] KVM: x86: Add AMD Guest PerfMonV2 PMU support Like Xu
2022-11-11 10:26 ` [PATCH v3 1/8] KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled() Like Xu
2023-01-27 2:03 ` Sean Christopherson
2023-02-02 11:46 ` Like Xu
2022-11-11 10:26 ` [PATCH v3 2/8] KVM: VMX: Refactor intel_pmu_set_msr() to align with other set_msr() helpers Like Xu
2023-01-27 2:09 ` Sean Christopherson
2022-11-11 10:26 ` [PATCH v3 3/8] KVM: x86/pmu: Rewrite reprogram_counters() to improve performance Like Xu
2023-01-20 1:09 ` Sean Christopherson
2023-01-24 20:16 ` Sean Christopherson
2022-11-11 10:26 ` [PATCH v3 4/8] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
2022-11-11 10:26 ` [PATCH v3 5/8] KVM: x86/cpuid: Add X86_FEATURE_AMD_PMU_V2 as a KVM-only leaf entry Like Xu
2023-01-24 19:47 ` Sean Christopherson
2023-02-06 11:47 ` Like Xu
2023-02-10 1:32 ` Sean Christopherson
2022-11-11 10:26 ` [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
2023-01-25 0:10 ` Sean Christopherson
2023-02-06 7:53 ` Like Xu [this message]
2023-02-06 22:22 ` Sean Christopherson
2022-11-11 10:26 ` [PATCH v3 7/8] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
2023-01-25 0:16 ` Sean Christopherson
2022-11-11 10:26 ` [PATCH v3 8/8] KVM: x86/cpuid: Use fast return for cpuid "0xa" leaf when !enable_pmu Like Xu
2023-01-20 1:11 ` Sean Christopherson
2022-12-06 8:32 ` [PATCH v3 0/8] KVM: x86: Add AMD Guest PerfMonV2 PMU support Like Xu
2023-01-20 1:13 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c7e0a5b1-0fd0-e2b5-20ca-fc86a1d883db@gmail.com \
--to=like.xu.linux@gmail.com \
--cc=jmattson@google.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=sandipan.das@amd.com \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).