Messages in this thread | | | Date | Mon, 6 Feb 2023 15:53:00 +0800 | Subject | Re: [PATCH v3 6/8] KVM: x86/svm/pmu: Add AMD PerfMonV2 support | From | Like Xu <> |
| |
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; >
| |