On Mon, Sep 19, 2022, Like Xu wrote:
> @@ -162,20 +179,43 @@ 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;
> - else
> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
> + pmu->version = 1;
> + if (kvm_pmu_cap.version > 1) {
> + pmu->version = min_t(unsigned int, 2, kvm_pmu_cap.version);
This is wrong, it forces the guest PMU verson to match the max version supported
by KVM. E.g. if userspace wants to expose v1 for whatever reason, pmu->version
will still end up 2+.
> + entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
> + if (entry) {
> + 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);
This is technically wrong, the number of counters is supposed to be valid if and
only if v2 is supported. On a related topic, does KVM explode if userspace
specifies a bogus PMU version on Intel? I don't see any sanity checks there...
With a proper feature flag
pmu->version = 1;
if (kvm_cpu_has(X86_FEATURE_AMD_PMU_V2) &&
guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)) {
pmu->version = 2;
entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
if (entry) {
...
Though technically the "if (entry)" check is unnecesary.
> + }
> + }
> +
> + /* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> + if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE)) {
Unnecessary braces.
> + pmu->nr_arch_gp_counters = max_t(unsigned int,
> + pmu->nr_arch_gp_counters,
> + AMD64_NUM_COUNTERS_CORE);
What happens if userspace sets X86_FEATURE_PERFCTR_CORE when its not supported?
E.g. will KVM be coerced into taking a #GP on a non-existent counter?
> + } else {
> + pmu->nr_arch_gp_counters = max_t(unsigned int,
> + pmu->nr_arch_gp_counters,
> + AMD64_NUM_COUNTERS);
> + }
