Re: kernel 3.7+ cpufreq regression on AMD system running as dom0

Konrad Rzeszutek Wilk
2013-01-18T22:00:36Z

>
> > Right, that information is gathered from the MSRs. I think the Xen would
> > need to do this since it can do the MSRs correctly and modify the P-states.
> >
> > So something like this in the hypervisor maybe (not even tested):
>
> Yeah, something like that. Basically you can copy the quirk down to the
> hypervisor.

<nods> Need also to include the comments from Matthew in it.
>
> But, Andre was explaining to me the other day that those P-states
> frequencies are not that important.
>
> Let me explain: the ondemand governor, for example, computes idle time
> and each time it needs to increase, it switches straight up to the
> highest frequency. When it decreases the freq. though, it goes down in a
> staircase manner, going over all P-states, AFAICT.
>
> So we use them but not for all decisions. The question is, what does the
> xen governor(s) do?

should look in the code. I know it borrowed from the Linux code - but
I don't know from which era - 2.6.18 maybe?

>
> If it only uses the frequencies for reporting, then it is not that big
> of a deal. If it uses their values for switching decisions, then it
> probably needs the correct ones.

OK.
>
> > diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
> > index a9b7792..54e7808 100644
> > --- a/xen/arch/x86/acpi/cpufreq/powernow.c
> > +++ b/xen/arch/x86/acpi/cpufreq/powernow.c
> > @@ -146,7 +146,40 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy,
> >
> > return 0;
> > }
> > +#define MSR_AMD_PSTATE_DEF_BASE 0xc0010064
> > +static void amd_fixup_frequency(struct xen_processor_px *px, int i)
> > +{
> > + u32 hi, lo, fid, did;
> > + int index = px->control & 0x00000007;
> > +
> > + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > + return;
> > +
> > + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10)
> > + || boot_cpu_data.x86 == 0x11) {
> > + rdmsr(MSR_AMD_PSTATE_DEF_BASE + index, lo, hi);
> > + /* Bit 63 indicates whether contents are valid */
> > + if (!(hi & 0x80000000))
> > + return;
>
> Something's funny with this indentation.

That was copy-n-paste, so I must have done something incorrectly. Anyhow
I still need to actually test this code.

>
> > +
> > + fid = lo & 0x3f;
> > + did = (lo >> 6) & 7;
> > + if (boot_cpu_data.x86 == 0x10)
> > + px->core_frequency = (100 * (fid + 0x10)) >> did;
> > + else
> > + px->core_frequency = (100 * (fid + 8)) >> did;
> > + }
> > +}
> > +
> > +static void amd_fixup_freq(struct processor_performance *perf)
> > +{
> >
> > + int i;
> > +
> > + for (i = 0; i < perf->state_count; i++)
> > + amd_fixup_frequency(perf->states, i);
> > +
> > +}
> > static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
> > {
> > struct acpi_cpufreq_data *data;
> > @@ -158,6 +191,8 @@ static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
> >
> > perf = &processor_pminfo[policy->cpu]->perf;
> >
> > + amd_fixup_freq(perf);
> > +
> > cpufreq_verify_within_limits(policy, 0,
> > perf->states[perf->platform_limit].core_frequency * 1000);
>
> Thanks.
>
> --
> Regards/Gruss,
> Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --