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>
Subject: Re: [PATCH v4 11/12] KVM: x86/svm/pmu: Add AMD PerfMonV2 support
Date: Fri, 7 Apr 2023 15:08:30 +0800 [thread overview]
Message-ID: <dfc5cba8-5efb-8ad6-01e0-2800290a9ac1@gmail.com> (raw)
In-Reply-To: <ZC9zSjZ7A47v7VCD@google.com>
On 7/4/2023 9:35 am, Sean Christopherson wrote:
> On Tue, Feb 14, 2023, Like Xu wrote:
>> + case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
>> + if (!msr_info->host_initiated)
>> + return 0; /* Writes are ignored */
>
> Where is the "writes ignored" behavior documented? I can't find anything in the
> APM that defines write behavior.
KVM would follow the real hardware behavior once specifications stay silent on
details or secret.
>
>>
>> pmu->global_status = data;
>> return 0;
>> case MSR_CORE_PERF_GLOBAL_CTRL:
>> if (!kvm_valid_perf_global_ctrl(pmu, data))
>> return 1;
>> -
>> + fallthrough;
>
> This _definitely_ needs a comment. Hmm, and I would prefer to reverse these, i.e.
>
> case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
> data &= ~pmu->global_ctrl_mask;
> fallthrough;
> case MSR_CORE_PERF_GLOBAL_CTRL:
> if (!kvm_valid_perf_global_ctrl(pmu, data))
> return 1;
>
> It's a bit arbitrary, but either Intel or AMD is going to end up with extra code,
> and IMO skipping a validity check is more alarming than skipping clearing of
> reserved bits, i.e. will look like a bug to future readers.
>
>> + case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
>> + data &= ~pmu->global_ctrl_mask;
>> if (pmu->global_ctrl != data) {
>> diff = pmu->global_ctrl ^ data;
>> pmu->global_ctrl = data;
>> @@ -616,7 +625,8 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
>> if (data & pmu->global_ovf_ctrl_mask)
>> return 1;
>> -
>> + fallthrough;
>
> Here too. Argh, the APM doesn't actually define what happens on reserved bits,
> it just says "WO". I vote to be conservative and ignore writes to reserved bits.
> And then we can have one comment for the whole block, e.g.
>
> /*
> * Note, AMD ignores writes to read-only PMU MSRs/bits, whereas Intel
> * generates #GP on attempts to write reserved bits or RO MSRs.
> */
> switch (msr) {
> case MSR_CORE_PERF_GLOBAL_STATUS:
> if (!msr_info->host_initiated)
> return 1; /* RO MSR */
> fallthrough;
> case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
> if (!msr_info->host_initiated)
> break;
>
> pmu->global_status = data;
> break;
> case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
> data &= ~pmu->global_ctrl_mask;
> fallthrough;
> case MSR_CORE_PERF_GLOBAL_CTRL:
> if (!kvm_valid_perf_global_ctrl(pmu, data))
> return 1;
>
> if (pmu->global_ctrl != data) {
> diff = pmu->global_ctrl ^ data;
> pmu->global_ctrl = data;
> reprogram_counters(pmu, diff);
> }
> break;
> case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
> fallthrough;
> case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> if (data & pmu->global_ovf_ctrl_mask)
> return 1;
>
> if (!msr_info->host_initiated)
> pmu->global_status &= ~data;
> break;
> default:
> kvm_pmu_mark_pmc_in_use(vcpu, msr_info->index);
> return static_call(kvm_x86_pmu_set_msr)(vcpu, msr_info);
> }
>
> return 0;
AMD doesn't generates #GP on attempts to write PMU RO MSRs and reserved bits.
How about this:
/*
* Note, AMD ignores writes to reserved bits and read-only PMU MSRs,
* whereas Intel generates #GP on attempts to write reserved/RO MSRs.
*/
switch (msr) {
case MSR_CORE_PERF_GLOBAL_STATUS:
if (!msr_info->host_initiated || (data & pmu->global_ovf_ctrl_mask))
return 1; /* RO MSR */
fallthrough;
case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
if (!msr_info->host_initiated)
break;
pmu->global_status = data;
break;
case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
data &= ~pmu->global_ctrl_mask;
fallthrough;
case MSR_CORE_PERF_GLOBAL_CTRL:
if (!kvm_valid_perf_global_ctrl(pmu, data))
return 1;
if (pmu->global_ctrl != data) {
diff = pmu->global_ctrl ^ data;
pmu->global_ctrl = data;
reprogram_counters(pmu, diff);
}
break;
case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
if (data & pmu->global_ovf_ctrl_mask)
return 1;
fallthrough;
case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
if (!msr_info->host_initiated)
pmu->global_status &= ~data;
break;
default:
kvm_pmu_mark_pmc_in_use(vcpu, msr_info->index);
return static_call(kvm_x86_pmu_set_msr)(vcpu, msr_info);
}
return 0;
>
>> @@ -164,20 +181,34 @@ 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->version = 1;
>> + if (guest_cpuid_has(vcpu, X86_FEATURE_PERFMON_V2)) {
>> + pmu->version = 2;
>> + entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
>
> No need for the intermediate "entry".
>> + ebx.full = entry->ebx;
>
> Oof, at first glance this looks like a potential null-pointer deref bug. I
> believe we can do
>
> /*
> * Note, PERFMON_V2 is also in 0x80000022.0x0, i.e. the guest
> * CPUID entry is guaranteed to be non-NULL.
> */
> BUILD_BUG_ON(x86_feature_cpuid(X86_FEATURE_PERFMON_V2).function != 0x80000022 ||
> x86_feature_cpuid(X86_FEATURE_PERFMON_V2).index != 0x80000022);
x86_feature_cpuid(X86_FEATURE_PERFMON_V2).index);
> ebx.full = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0)->ebx;
>
>> + pmu->nr_arch_gp_counters = min_t(unsigned int,
>> + 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;
>
> This needs to be sanitized, no? E.g. if KVM only has access to 4 counters, but
> userspace sets X86_FEATURE_PERFCTR_CORE anyways. Hrm, unless I'm missing something,
> that's a pre-existing bug.
Now your point is that if a user space more capbility than KVM can support, KVM
should constrain it.
Your previous preference was that the user space can set capbilities that evene
if KVM doesn't support
as long as it doesn't break KVM and host and the guest will eat its own.
>
> If I'm right, can you add a patch to cap nr_arch_gp_counters at
> kvm_pmu_cap.num_counters_gp in the common flow, i.e. after this if-else block?
> Then there is no change needed in this patch, e.g. we'll naturally end up with:
>
> union cpuid_0x80000022_ebx ebx;
>
> pmu->version = 1;
> if (guest_cpuid_has(vcpu, X86_FEATURE_PERFMON_V2)) {
> pmu->version = 2;
> /*
> * Note, PERFMON_V2 is also in 0x80000022.0x0, i.e. the guest
> * CPUID entry is guaranteed to be non-NULL.
> */
> BUILD_BUG_ON(x86_feature_cpuid(X86_FEATURE_PERFMON_V2).function != 0x80000022 ||
> x86_feature_cpuid(X86_FEATURE_PERFMON_V2).index);
> ebx.full = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0)->ebx;
> pmu->nr_arch_gp_counters = ebx.split.num_core_pmc;
> } 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_COUNTERS;
> }
>
> pmu->nr_arch_gp_counters = min_t(unsigned int,
> pmu->nr_arch_gp_counters,
> kvm_pmu_cap.num_counters_gp);
next prev parent reply other threads:[~2023-04-07 7:08 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-14 5:07 [PATCH v4 00/12] KVM: x86: Add AMD Guest PerfMonV2 PMU support Like Xu
2023-02-14 5:07 ` [PATCH v4 01/12] KVM: x86/pmu: Rename pmc_is_enabled() to pmc_is_globally_enabled() Like Xu
2023-02-14 5:07 ` [PATCH v4 02/12] KVM: VMX: Refactor intel_pmu_set_msr() to align with other set_msr() helpers Like Xu
2023-02-16 21:13 ` Sean Christopherson
2023-02-21 8:44 ` Like Xu
2023-03-23 7:43 ` Like Xu
2023-03-23 14:28 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 03/12] KVM: x86/pmu: Rewrite reprogram_counters() to improve performance Like Xu
2023-02-14 5:07 ` [PATCH v4 04/12] KVM: x86/pmu: Expose reprogram_counters() in pmu.h Like Xu
2023-02-14 5:07 ` [PATCH v4 05/12] KVM: x86/pmu: Error when user sets the GLOBAL_STATUS reserved bits Like Xu
2023-04-06 23:45 ` Sean Christopherson
2023-04-07 5:08 ` Like Xu
2023-04-07 15:43 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 06/12] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
2023-04-06 23:57 ` Sean Christopherson
2023-04-07 1:39 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 07/12] KVM: x86/cpuid: Use fast return for cpuid "0xa" leaf when !enable_pmu Like Xu
2023-04-06 23:59 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 08/12] KVM: x86/pmu: Disable vPMU if the minimum num of counters isn't met Like Xu
2023-04-07 0:06 ` Sean Christopherson
2023-04-07 0:23 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 09/12] KVM: x86/pmu: Forget PERFCTR_CORE if the min " Like Xu
2023-04-07 0:32 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 10/12] KVM: x86/cpuid: Add X86_FEATURE_PERFMON_V2 as a scattered flag Like Xu
2023-04-07 0:41 ` Sean Christopherson
2023-02-14 5:07 ` [PATCH v4 11/12] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
2023-04-07 1:35 ` Sean Christopherson
2023-04-07 7:08 ` Like Xu [this message]
2023-04-07 14:44 ` Sean Christopherson
2023-04-10 11:34 ` Like Xu
2023-02-14 5:07 ` [PATCH v4 12/12] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
2023-04-07 1:50 ` Sean Christopherson
2023-04-07 7:19 ` Like Xu
2023-04-07 2:02 ` [PATCH v4 00/12] KVM: x86: Add AMD Guest PerfMonV2 PMU support Sean Christopherson
2023-04-07 7:28 ` Like Xu
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=dfc5cba8-5efb-8ad6-01e0-2800290a9ac1@gmail.com \
--to=like.xu.linux@gmail.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).