[PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wei Wang]

By default, the old P-state driver (acpi-freq) is used. Adding
"intel_pstate" to the Xen booting param list to enable the
use of intel_pstate. However, if intel_pstate is enabled on a
machine which does not support the driver (e.g. Nehalem), the
old P-state driver will be loaded due to the failure loading of
intel_pstate.

Also, adding the intel_pstate booting parameter to
xen-command-line.markdown.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 docs/misc/xen-command-line.markdown      | 7 +++++++
 xen/arch/x86/acpi/cpufreq/cpufreq.c      | 9 ++++++---
 xen/arch/x86/acpi/cpufreq/intel_pstate.c | 4 ++++
 3 files changed, 17 insertions(+), 3 deletions(-)

 changes in v5:
 1) move the booting parameter into the intel_pstate_init() function - have
    it be a local variable;
 2) rename "intel_pstate_load" to "load".

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a2e427c..2d70137 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -849,6 +849,13 @@ debug hypervisor only).
 ### idle\_latency\_factor
 > `= <integer>`
 
+### intel\_pstate
+> `= <boolean>`
+
+> Default: `false`
+
+Enable the loading of the intel pstate driver.
+
 ### ioapic\_ack
 > `= old | new`
 
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 8494fa0..7e517b9 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -40,6 +40,7 @@
 #include <asm/processor.h>
 #include <asm/percpu.h>
 #include <asm/cpufeature.h>
+#include <asm/cpufreq.h>
 #include <acpi/acpi.h>
 #include <acpi/cpufreq/cpufreq.h>
 
@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
     int ret = 0;
 
     if ((cpufreq_controller == FREQCTL_xen) &&
-        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
-        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
-    else if ((cpufreq_controller == FREQCTL_xen) &&
+        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
+            ret = intel_pstate_init();
+            if (ret)
+                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
+    } else if ((cpufreq_controller == FREQCTL_xen) &&
         (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
         ret = powernow_register_driver();
 
diff --git a/xen/arch/x86/acpi/cpufreq/intel_pstate.c b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
index 3292bcd..4ebd9c7 100644
--- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
+++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
@@ -843,7 +843,11 @@ int __init intel_pstate_init(void)
     int cpu, rc = 0;
     const struct x86_cpu_id *id;
     struct cpu_defaults *cpu_info;
+    static bool_t __read_mostly load;
+    boolean_param("intel_pstate", load);
 
+    if ( !load )
+        return -ENODEV;
 
     id = x86_match_cpu(intel_pstate_cpu_ids);
     if ( !id )
-- 
1.9.1

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Jan Beulich]

>>> On 14.09.15 at 04:42, <wei.w.wang@intel.com> wrote:
> By default, the old P-state driver (acpi-freq) is used. Adding
> "intel_pstate" to the Xen booting param list to enable the
> use of intel_pstate. However, if intel_pstate is enabled on a
> machine which does not support the driver (e.g. Nehalem), the
> old P-state driver will be loaded due to the failure loading of
> intel_pstate.
> 
> Also, adding the intel_pstate booting parameter to
> xen-command-line.markdown.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---

Was this a resend? Without any note it's unclear whether the earlier
or later one with the same title should be looked at.

Jan

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wang, Wei W]

On 14/09/2015 19:18,  Jan Beulich wrote:
>>> On 14.09.15 at 04:42, <wei.w.wang@intel.com> wrote:
> By default, the old P-state driver (acpi-freq) is used. Adding 
> "intel_pstate" to the Xen booting param list to enable the use of 
> intel_pstate. However, if intel_pstate is enabled on a machine which 
> does not support the driver (e.g. Nehalem), the old P-state driver 
> will be loaded due to the failure loading of intel_pstate.
> 
> Also, adding the intel_pstate booting parameter to 
> xen-command-line.markdown.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---

> Was this a resend? Without any note it's unclear whether the earlier or later one with the same title should be looked at.

I just didn't see this one appearing in the mailing list, so I re-sent the  [PATCH v5 7/9] separately. Sorry about the confusion.

Best,
Wei

[PATCH v5 0/9] Porting the intel_pstate driver to Xen

[Wei Wang]

v5 changes:
We have made various changes in this version, including introducing new
data structures, coding styles changes etc. Please see each patch's commit
message for change details.

v4 changes:
1) introduce a new struct, internal_governor, to "cpufreq_policy";
2) add a new header file, xen/include/asm-x86/cpufreq.h;
3) remove the APERF/MPERF feature detection code in cpufreq.c and powernow.c;
4) coding style changes.

Please check each patch's commit message for details.

v3 Changes:
1) coding style changes based on Jan's comments;
2) remove the function - unregister_cpu_notifier();
3) solve a bug in the CPU offline code (Patch 0007);
4) move the perf_limits struct into the per-CPU policy struct, so that
each CPU can be managed individually;
5) "load_intel_pstate" is changed local to the intel_pstate.c file, and
add its description to the xen-command-line.markdown.

v2 Changes:
1) The intel_pstate driver can be controlled via two ways:
A. min_perf_pct and max_perf_pct
   The user directly adjusts min_perf_pct and max_perf_pct to get what 
   they want. For example, if min_perf_pct=max_perf_pct=60%, then the 
   user is asking for something similar to a userspace governor with 
   setting the requested performance=60%.
B. set-scaling-governor
   This one is functionally redundant, since A. can achieve all the
   governor functions. It is remained to give people time to get
   familiar with method A.
   Users can choose from the four governors: Powersave, Ondemand,
   Powersave, Performance. The driver achieves the functionality of 
   the selected governor via adjusting the min_perf_pct and max_perf_pct
   itself.
2) The xenpm "get-cpufreq-para" displays the following things:
cpu id               : 10
affected_cpus        : 10
cpuinfo frequency    : max [3700000] min [1200000] cur [1400000]
scaling_driver       : intel_pstate
scaling_avail_gov    : performance powersave userspace ondemand
current_governor     : ondemand
max_perf_pct         : 100
min_perf_pct         : 32
turbo_pct            : 54
turbo mode           : enabled
3) Changed "intel_pstate=disable" to "intel_pstate=enable". 
If "intel_pstate=enable" is added, but the CPU does not support the
intel_pstate driver, the old P-state driver (acpi-cpufreq) will be loaded.
4) Moved the declarations under xen/include/acpi to an x86-specific header.

v1:
This patch series ports the intel_pstate driver from the Linux kernel to
Xen. The intel_pstate driver is used to tune P states for SandyBridge+
processors. It needs to be enabled by adding "intel_pstate=enable" to the
booting parameter list.

The intel_pstate.c file under xen/arch/x86/acpi/cpufreq/
contains all the logic for selecting the current P-state. It follows its
implementation in the kernel. In order to better support future Intel CPUs
(e.g. the HWP feature on Skylake+), intel_pstate changes to tune P-state
based on percentage values.

The xenpm tool is also upgraded to support the intel_pstate driver. If
intel_pstate is used, "get-cpufreq-para" displays percentage value based
feedback. If the intel_pstate driver is not enabled, xenpm will work in
the old style.

Wei Wang (9):
  x86/intel_pstate: add some calculation related support
  x86/intel_pstate: APERF/MPERF feature detect
  x86/intel_pstate: add a new driver interface, setpolicy()
  x86/intel_pstate: relocate the driver register function
  x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline
  x86/intel_pstate: the main boby of the intel_pstate driver
  x86/intel_pstate: add a booting param to select the driver to load
  x86/intel_pstate: support the use of intel_pstate in pmstat.c
  tools: enable xenpm to control the intel_pstate driver

 docs/misc/xen-command-line.markdown      |   7 +
 tools/libxc/include/xenctrl.h            |  21 +-
 tools/libxc/xc_pm.c                      |  16 +-
 tools/misc/xenpm.c                       | 116 +++-
 xen/arch/x86/acpi/cpufreq/Makefile       |   1 +
 xen/arch/x86/acpi/cpufreq/cpufreq.c      |  15 +-
 xen/arch/x86/acpi/cpufreq/intel_pstate.c | 884 +++++++++++++++++++++++++++++++
 xen/arch/x86/acpi/cpufreq/powernow.c     |   6 +-
 xen/arch/x86/cpu/common.c                |   4 +
 xen/arch/x86/oprofile/op_model_athlon.c  |   9 -
 xen/drivers/acpi/pmstat.c                | 179 ++++++-
 xen/drivers/cpufreq/cpufreq.c            |  21 +-
 xen/drivers/cpufreq/utility.c            |   3 +
 xen/include/acpi/cpufreq/cpufreq.h       |  54 +-
 xen/include/asm-x86/cpufeature.h         |   5 +
 xen/include/asm-x86/cpufreq.h            |  34 ++
 xen/include/asm-x86/div64.h              |  79 +++
 xen/include/asm-x86/msr-index.h          |   3 +
 xen/include/public/sysctl.h              |  29 +-
 xen/include/xen/kernel.h                 |  23 +
 20 files changed, 1384 insertions(+), 125 deletions(-)
 create mode 100644 xen/arch/x86/acpi/cpufreq/intel_pstate.c
 create mode 100644 xen/include/asm-x86/cpufreq.h

-- 
1.9.1

[PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wei Wang]

By default, the old P-state driver (acpi-freq) is used. Adding
"intel_pstate" to the Xen booting param list to enable the
use of intel_pstate. However, if intel_pstate is enabled on a
machine which does not support the driver (e.g. Nehalem), the
old P-state driver will be loaded due to the failure loading of
intel_pstate.

Also, adding the intel_pstate booting parameter to
xen-command-line.markdown.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 docs/misc/xen-command-line.markdown      | 7 +++++++
 xen/arch/x86/acpi/cpufreq/cpufreq.c      | 9 ++++++---
 xen/arch/x86/acpi/cpufreq/intel_pstate.c | 4 ++++
 3 files changed, 17 insertions(+), 3 deletions(-)

 changes in v5:
 1) move the booting parameter into the intel_pstate_init() function - have
    it be a local variable;
 2) rename "intel_pstate_load" to "load".

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a2e427c..2d70137 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -849,6 +849,13 @@ debug hypervisor only).
 ### idle\_latency\_factor
 > `= <integer>`
 
+### intel\_pstate
+> `= <boolean>`
+
+> Default: `false`
+
+Enable the loading of the intel pstate driver.
+
 ### ioapic\_ack
 > `= old | new`
 
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 8494fa0..7e517b9 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -40,6 +40,7 @@
 #include <asm/processor.h>
 #include <asm/percpu.h>
 #include <asm/cpufeature.h>
+#include <asm/cpufreq.h>
 #include <acpi/acpi.h>
 #include <acpi/cpufreq/cpufreq.h>
 
@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
     int ret = 0;
 
     if ((cpufreq_controller == FREQCTL_xen) &&
-        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
-        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
-    else if ((cpufreq_controller == FREQCTL_xen) &&
+        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
+            ret = intel_pstate_init();
+            if (ret)
+                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
+    } else if ((cpufreq_controller == FREQCTL_xen) &&
         (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
         ret = powernow_register_driver();
 
diff --git a/xen/arch/x86/acpi/cpufreq/intel_pstate.c b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
index 3292bcd..4ebd9c7 100644
--- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
+++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
@@ -843,7 +843,11 @@ int __init intel_pstate_init(void)
     int cpu, rc = 0;
     const struct x86_cpu_id *id;
     struct cpu_defaults *cpu_info;
+    static bool_t __read_mostly load;
+    boolean_param("intel_pstate", load);
 
+    if ( !load )
+        return -ENODEV;
 
     id = x86_match_cpu(intel_pstate_cpu_ids);
     if ( !id )
-- 
1.9.1

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Andrew Cooper]

On 14/09/15 03:32, Wei Wang wrote:
> By default, the old P-state driver (acpi-freq) is used. Adding
> "intel_pstate" to the Xen booting param list to enable the
> use of intel_pstate. However, if intel_pstate is enabled on a
> machine which does not support the driver (e.g. Nehalem), the
> old P-state driver will be loaded due to the failure loading of
> intel_pstate.
>
> Also, adding the intel_pstate booting parameter to
> xen-command-line.markdown.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  docs/misc/xen-command-line.markdown      | 7 +++++++
>  xen/arch/x86/acpi/cpufreq/cpufreq.c      | 9 ++++++---
>  xen/arch/x86/acpi/cpufreq/intel_pstate.c | 4 ++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
>
>  changes in v5:
>  1) move the booting parameter into the intel_pstate_init() function - have
>     it be a local variable;
>  2) rename "intel_pstate_load" to "load".
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index a2e427c..2d70137 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -849,6 +849,13 @@ debug hypervisor only).
>  ### idle\_latency\_factor
>  > `= <integer>`
>  
> +### intel\_pstate
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Enable the loading of the intel pstate driver.
> +
>  ### ioapic\_ack
>  > `= old | new`
>  
> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> index 8494fa0..7e517b9 100644
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -40,6 +40,7 @@
>  #include <asm/processor.h>
>  #include <asm/percpu.h>
>  #include <asm/cpufeature.h>
> +#include <asm/cpufreq.h>
>  #include <acpi/acpi.h>
>  #include <acpi/cpufreq/cpufreq.h>
>  
> @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
>      int ret = 0;
>  
>      if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> -    else if ((cpufreq_controller == FREQCTL_xen) &&
> +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> +            ret = intel_pstate_init();
> +            if (ret)
> +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);

This looks like a mess (although not your fault to start with).

We shouldn't have multiple different top level command line options.  In
particular, having "mwait-idle" and "intel_pstate" seems wrong, given a
perfectly good "cpufreq=" option.

Is the driver in use available to change at runtime from `xenpm` ?

> +    } else if ((cpufreq_controller == FREQCTL_xen) &&
>          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>          ret = powernow_register_driver();
>  
> diff --git a/xen/arch/x86/acpi/cpufreq/intel_pstate.c b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> index 3292bcd..4ebd9c7 100644
> --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> @@ -843,7 +843,11 @@ int __init intel_pstate_init(void)
>      int cpu, rc = 0;
>      const struct x86_cpu_id *id;
>      struct cpu_defaults *cpu_info;
> +    static bool_t __read_mostly load;

__initdata

~Andrew

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Jan Beulich]

>>> On 17.09.15 at 18:08, <andrew.cooper3@citrix.com> wrote:
> We shouldn't have multiple different top level command line options.  In
> particular, having "mwait-idle" and "intel_pstate" seems wrong, given a
> perfectly good "cpufreq=" option.

"mwait-idle" is C-state related.

Jan

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Jan Beulich]

>>> On 14.09.15 at 04:32, <wei.w.wang@intel.com> wrote:
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -40,6 +40,7 @@
>  #include <asm/processor.h>
>  #include <asm/percpu.h>
>  #include <asm/cpufeature.h>
> +#include <asm/cpufreq.h>

I think to make clear why this include is needed here it would be better
if you added the declaration of intel_pstate_init() also in this patch
instead of in the previous one.

> @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
>      int ret = 0;
>  
>      if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> -    else if ((cpufreq_controller == FREQCTL_xen) &&
> +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> +            ret = intel_pstate_init();
> +            if (ret)
> +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> +    } else if ((cpufreq_controller == FREQCTL_xen) &&
>          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>          ret = powernow_register_driver();

Since you're basically modifying the entire body of the function,
please gets its coding style corrected as you fiddle with it.

> --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> @@ -843,7 +843,11 @@ int __init intel_pstate_init(void)
>      int cpu, rc = 0;
>      const struct x86_cpu_id *id;
>      struct cpu_defaults *cpu_info;
> +    static bool_t __read_mostly load;

Why __read_mostly when the function is __init?

Jan

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wang, Wei W]

On 07/10/2015 11:46,  Jan Beulich wrote:
> >>> On 14.09.15 at 04:32, <wei.w.wang@intel.com> wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
> >      int ret = 0;
> >
> >      if ((cpufreq_controller == FREQCTL_xen) &&
> > -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> > -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > -    else if ((cpufreq_controller == FREQCTL_xen) &&
> > +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> > +            ret = intel_pstate_init();
> > +            if (ret)
> > +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > +    } else if ((cpufreq_controller == FREQCTL_xen) &&
> >          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> >          ret = powernow_register_driver();
> 
> Since you're basically modifying the entire body of the function, please gets its
> coding style corrected as you fiddle with it.
> 

The coding style here is aligned with the existing piece of code in this file. If it's also good to make the file have two coding styles, please let me know. 

Best,
Wei

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wang, Wei W]

On 07/10/2015 23:46,  Jan Beulich wrote:
> >>> On 14.09.15 at 04:32, <wei.w.wang@intel.com> wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
> >      int ret = 0;
> >
> >      if ((cpufreq_controller == FREQCTL_xen) &&
> > -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> > -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > -    else if ((cpufreq_controller == FREQCTL_xen) &&
> > +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> > +            ret = intel_pstate_init();
> > +            if (ret)
> > +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > +    } else if ((cpufreq_controller == FREQCTL_xen) &&
> >          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> >          ret = powernow_register_driver();
> 
> Since you're basically modifying the entire body of the function, please gets its
> coding style corrected as you fiddle with it.

OK, I guess you was probably referring to the  this line - (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)).
Will align that as well.

Best,
Wei

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wang, Wei W]

On 07/10/2015 23:46,  Jan Beulich wrote:
> >>> On 14.09.15 at 04:32, <wei.w.wang@intel.com> wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
> >      int ret = 0;
> >
> >      if ((cpufreq_controller == FREQCTL_xen) &&
> > -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> > -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > -    else if ((cpufreq_controller == FREQCTL_xen) &&
> > +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> > +            ret = intel_pstate_init();
> > +            if (ret)
> > +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > +    } else if ((cpufreq_controller == FREQCTL_xen) &&
> >          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> >          ret = powernow_register_driver();
> 
> Since you're basically modifying the entire body of the function, please gets its
> coding style corrected as you fiddle with it.

Ok, I guess you was probably referring to the remaining lines in the function - "(boot_cpu_data.x86_vendor == X86_VENDOR_AMD))".. Will align them as well.

Best,
Wei

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Jan Beulich]

>>> On 23.10.15 at 10:18, <wei.w.wang@intel.com> wrote:
> On 07/10/2015 23:46,  Jan Beulich wrote:
>> >>> On 14.09.15 at 04:32, <wei.w.wang@intel.com> wrote:
>> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> > @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
>> >      int ret = 0;
>> >
>> >      if ((cpufreq_controller == FREQCTL_xen) &&
>> > -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
>> > -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>> > -    else if ((cpufreq_controller == FREQCTL_xen) &&
>> > +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
>> > +            ret = intel_pstate_init();
>> > +            if (ret)
>> > +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>> > +    } else if ((cpufreq_controller == FREQCTL_xen) &&
>> >          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>> >          ret = powernow_register_driver();
>> 
>> Since you're basically modifying the entire body of the function, please 
> gets its
>> coding style corrected as you fiddle with it.
> 
> Ok, I guess you was probably referring to the remaining lines in the 
> function - "(boot_cpu_data.x86_vendor == X86_VENDOR_AMD))".. Will align them 
> as well.

No, I'm not just talking about alignment. And the coding style of the
file is mixed already (see e.g. the following function, which admittedly
has even more blanks than needed), so getting this function into
proper shape since you modify it in its entirety is a step in the right
direction.

Jan

Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Wang, Wei W]

On 23/10/2015 16:36,  Jan Beulich wrote:
> >>> On 23.10.15 at 10:18, <wei.w.wang@intel.com> wrote:
> > On 07/10/2015 23:46,  Jan Beulich wrote:
> >> >>> On 14.09.15 at 04:32, <wei.w.wang@intel.com> wrote:
> >> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >> > @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
> >> >      int ret = 0;
> >> >
> >> >      if ((cpufreq_controller == FREQCTL_xen) &&
> >> > -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> >> > -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> >> > -    else if ((cpufreq_controller == FREQCTL_xen) &&
> >> > +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> >> > +            ret = intel_pstate_init();
> >> > +            if (ret)
> >> > +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> >> > +    } else if ((cpufreq_controller == FREQCTL_xen) &&
> >> >          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> >> >          ret = powernow_register_driver();
> >>
> >> Since you're basically modifying the entire body of the function,
> >> please
> > gets its
> >> coding style corrected as you fiddle with it.
> >
> > Ok, I guess you was probably referring to the remaining lines in the
> > function - "(boot_cpu_data.x86_vendor == X86_VENDOR_AMD))".. Will
> > align them as well.
> 
> No, I'm not just talking about alignment. And the coding style of the file is mixed
> already (see e.g. the following function, which admittedly has even more blanks
> than needed), so getting this function into proper shape since you modify it in its
> entirety is a step in the right direction.
> 

Ok. I will use Xen style brackets for this function.

Best,
Wei