On Tue, Nov 08, 2022, Paolo Bonzini wrote:
> On 11/2/22 23:51, Sean Christopherson wrote:
> > + pmu.msr_gp_counter_base = MSR_F15H_PERF_CTR0;
> > + pmu.msr_gp_event_select_base = MSR_F15H_PERF_CTL0;
> > + if (!this_cpu_has(X86_FEATURE_PERFCTR_CORE))
> > + pmu.nr_gp_counters = AMD64_NUM_COUNTERS;
> > + else
> > + pmu.nr_gp_counters = AMD64_NUM_COUNTERS_CORE;
> > +
>
> If X86_FEATURE_PERFCTR_CORE is not set, pmu.msr_gp_*_base should point to
> MSR_K7_PERFCTR0/MSR_K7_EVNTSEL0:
/facepalm
I only ran the PMU tests, which all passthrough the relevant host CPUID.
Glad you debugged this, all tests were failing on Milan due to this. I shudder
to think about how long it would have taken me to figure this out.
Thanks!
> diff --git a/lib/x86/pmu.c b/lib/x86/pmu.c
> index af68f3a..8d5f69f 100644
> --- a/lib/x86/pmu.c
> +++ b/lib/x86/pmu.c
> @@ -47,10 +47,13 @@ void pmu_init(void)
> pmu.msr_gp_event_select_base = MSR_F15H_PERF_CTL0;
> if (this_cpu_has(X86_FEATURE_AMD_PMU_V2))
> pmu.nr_gp_counters = cpuid(0x80000022).b & 0xf;
> - else if (!this_cpu_has(X86_FEATURE_PERFCTR_CORE))
> - pmu.nr_gp_counters = AMD64_NUM_COUNTERS;
> - else
> + else if (this_cpu_has(X86_FEATURE_PERFCTR_CORE))
Nit, the if and else-if statements should also have braces.
> pmu.nr_gp_counters = AMD64_NUM_COUNTERS_CORE;
> + else {
> + pmu.nr_gp_counters = AMD64_NUM_COUNTERS;
> + pmu.msr_gp_counter_base = MSR_K7_PERFCTR0;
> + pmu.msr_gp_event_select_base = MSR_K7_EVNTSEL0;
> + }
>
> pmu.gp_counter_width = PMC_DEFAULT_WIDTH;
> pmu.gp_counter_mask_length = pmu.nr_gp_counters;
>
> Paolo
>
