[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

[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

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