Xen devel - May 2010 - [PATCH] x86/cpufreq: fix turbo mode detection

acpi_cpufreq_cpu_init() does generally not run on the CPU the policy
it deals with is related to, hence using cpuid() directly works only
as long as all CPUs in the system are identical (which admittedly
is commonly the case).

Also fix a minor glitch in xenpm, which I noticed while looking into
the original inconsistencies on one of my systems.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- 2010-05-04.orig/tools/misc/xenpm.c	2010-04-14 14:05:14.000000000 +0200
+++ 2010-05-04/tools/misc/xenpm.c	2010-05-06 13:37:30.000000000 +0200
@@ -499,10 +499,7 @@ static void print_cpufreq_para(int cpuid
 
     printf("affected_cpus        :");
     for ( i = 0; i < p_cpufreq->cpu_num; i++ )
-        if ( i == cpuid )
-            printf(" *%d", p_cpufreq->affected_cpus[i]);
-        else
-            printf(" %d", p_cpufreq->affected_cpus[i]);
+        printf(" %d", p_cpufreq->affected_cpus[i]);
     printf("\n");
 
     printf("cpuinfo frequency    : max [%u] min [%u] cur [%u]\n",
--- 2010-05-04.orig/xen/arch/x86/acpi/cpufreq/cpufreq.c	2010-04-12
11:28:20.000000000 +0200
+++ 2010-05-04/xen/arch/x86/acpi/cpufreq/cpufreq.c	2010-05-06 13:24:41.000000000
+0200
@@ -377,6 +377,23 @@ static unsigned int get_cur_freq_on_cpu(
     return freq;
 }
 
+static void feature_detect(void *info)
+{
+    struct cpufreq_policy *policy = info;
+    unsigned int eax, ecx;
+
+    ecx = cpuid_ecx(6);
+    if (ecx & CPUID_6_ECX_APERFMPERF_CAPABILITY)
+        acpi_cpufreq_driver.getavg = get_measured_perf;
+    eax = cpuid_eax(6);
+    if (eax & 0x2) {
+        policy->turbo = CPUFREQ_TURBO_ENABLED;
+        if (cpufreq_verbose)
+            printk(XENLOG_INFO "CPU%u: Turbo Mode detected and
enabled\n",
+                   smp_processor_id());
+    }
+}
+
 static unsigned int check_freqs(cpumask_t mask, unsigned int freq,
                                 struct acpi_cpufreq_data *data)
 {
@@ -615,18 +632,8 @@ acpi_cpufreq_cpu_init(struct cpufreq_pol
 
     /* Check for APERF/MPERF support in hardware
      * also check for boost support */
-    if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >=
6) {
-        unsigned int ecx;
-        unsigned int eax;
-        ecx = cpuid_ecx(6);
-        if (ecx & CPUID_6_ECX_APERFMPERF_CAPABILITY)
-            acpi_cpufreq_driver.getavg = get_measured_perf;
-        eax = cpuid_eax(6);
-        if ( eax & 0x2 ) {
-            policy->turbo = CPUFREQ_TURBO_ENABLED;
-            printk(XENLOG_INFO "Turbo Mode detected and enabled!\n");
-        }
-    }
+    if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >=
6)
+        on_selected_cpus(cpumask_of(cpu), feature_detect, policy, 1);
 
     /*
      * the first call to ->target() should result in us actually





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 06/05/2010 14:10, "Jan Beulich" <JBeulich@novell.com> wrote:
> acpi_cpufreq_cpu_init() does generally not run on the CPU the policy
> it deals with is related to, hence using cpuid() directly works only
> as long as all CPUs in the system are identical (which admittedly
> is commonly the case).
Doesn''t the fact that acpi_cpufreq_driver.getavg is a single global
function
pointer defeat the object of this patch? The effect of proper localised
checking of cpuid_ecx(6) is still global.

The check of cpuid_eax(6) yields a localised effect (on policy->turbo, where
policy does appear to be per-cpu). But whether ''fixing'' that
but not
properly the other is worthwhile, I''m not sure.
> Also fix a minor glitch in xenpm, which I noticed while looking into
> the original inconsistencies on one of my systems.
Would belong in a separate patch. Also it''s not clear to me what the
''glitch'' is. Printing an asterisk might be
frivolous/pointless, but is it
wrong?

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Keir Fraser <keir.fraser@eu.citrix.com> 06.05.10 17:16
>>>
>On 06/05/2010 14:10, "Jan Beulich" <JBeulich@novell.com>
wrote:
>
>> acpi_cpufreq_cpu_init() does generally not run on the CPU the policy
>> it deals with is related to, hence using cpuid() directly works only
>> as long as all CPUs in the system are identical (which admittedly
>> is commonly the case).
>
>Doesn''t the fact that acpi_cpufreq_driver.getavg is a single global
function
>pointer defeat the object of this patch? The effect of proper localised
>checking of cpuid_ecx(6) is still global.
>
>The check of cpuid_eax(6) yields a localised effect (on policy->turbo,
where
>policy does appear to be per-cpu). But whether ''fixing''
that but not
>properly the other is worthwhile, I''m not sure.
Yes, only one of the two were really an issue, but since I needed
to move part of this code around, I decided to move it all at once
in case later other bits (with non-global) meaning would get used.
>> Also fix a minor glitch in xenpm, which I noticed while looking into
>> the original inconsistencies on one of my systems.
>
>Would belong in a separate patch. Also it''s not clear to me what
the
>''glitch'' is. Printing an asterisk might be
frivolous/pointless, but is it
>wrong?
Yes, it printed the * on the wrong field (used the CPU number as an
index into an array consisting of only the members of one domain).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 06/05/2010 16:25, "Jan Beulich" <JBeulich@novell.com> wrote:
>> Doesn''t the fact that acpi_cpufreq_driver.getavg is a single
global function
>> pointer defeat the object of this patch? The effect of proper localised
>> checking of cpuid_ecx(6) is still global.
>> 
>> The check of cpuid_eax(6) yields a localised effect (on
policy->turbo, where
>> policy does appear to be per-cpu). But whether
''fixing'' that but not
>> properly the other is worthwhile, I''m not sure.
> 
> Yes, only one of the two were really an issue, but since I needed
> to move part of this code around, I decided to move it all at once
> in case later other bits (with non-global) meaning would get used.
Yeah, but do cpuid_ecx(6) cross-cpu differences really not matter, or is it
just that it has global effect so there was nothing more you could (easily)
do? I mean, if that latter then the solution is really half-baked isn''t
it.
And is half-baked worth doing?

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> acpi_cpufreq_cpu_init() does generally not run on the CPU the policy
> it deals with is related to, hence using cpuid() directly works only
> as long as all CPUs in the system are identical (which admittedly
> is commonly the case).
Is a similar patch necessary for AMD?  If so, are
you going to handle it or should I?

--Mark Langsdorf
Operating System Reesarch Center
AMD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Keir Fraser <keir.fraser@eu.citrix.com> 06.05.10 18:46
>>>
>On 06/05/2010 16:25, "Jan Beulich" <JBeulich@novell.com>
wrote:
>
>>> Doesn''t the fact that acpi_cpufreq_driver.getavg is a
single global function
>>> pointer defeat the object of this patch? The effect of proper
localised
>>> checking of cpuid_ecx(6) is still global.
>>> 
>>> The check of cpuid_eax(6) yields a localised effect (on
policy->turbo, where
>>> policy does appear to be per-cpu). But whether
''fixing'' that but not
>>> properly the other is worthwhile, I''m not sure.
>> 
>> Yes, only one of the two were really an issue, but since I needed
>> to move part of this code around, I decided to move it all at once
>> in case later other bits (with non-global) meaning would get used.
>
>Yeah, but do cpuid_ecx(6) cross-cpu differences really not matter, or is it
>just that it has global effect so there was nothing more you could (easily)
>do? I mean, if that latter then the solution is really half-baked
isn''t it.
>And is half-baked worth doing?
Hmm, yes, this shouldn''t be a global thing either. But perhaps for
this one it''s better to set the function pointer globally if any CPU
supports the functionality, and then simply return an error from
the function if the individual CPU doesn''t support it. I''ll
rev the
patch accordingly.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 06.05.10
20:35 >>>
>> acpi_cpufreq_cpu_init() does generally not run on the CPU the policy
>> it deals with is related to, hence using cpuid() directly works only
>> as long as all CPUs in the system are identical (which admittedly
>> is commonly the case).
>
>Is a similar patch necessary for AMD?  If so, are
>you going to handle it or should I?
Oh, yes - I think I checked and found it not necessary, but now that I
checked again, I seem basically identical behavior there. As I''ll rev
the
patch anyway, I''ll include powernow then.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel