Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-06-04 Thread Jan Beulich 
 "Wang, Wei W"  06/04/15 3:15 AM >>>
>On 03/06/2015 19:51, Jan Beulich wrote
>> >>> On 03.06.15 at 10:07,  wrote:
>>> @@ -650,9 +650,12 @@ 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)) {
>> >+if (load_intel_pstate)
>> >+ret = intel_pstate_init();
>> >+if (!load_intel_pstate)
>> >+ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>
>>I don't see why you need load_intel_pstate here: Simply call the original 
>>function whenever 
>>intel_pstate_init() returns an error.
>
>I plan to change it to:
>if (load_intel_pstate)
>ret = intel_pstate_init();
>if (ret)
>ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>
>This allows the case that the machine supports the intel_pstate driver but 
>people
> just prefer to use the old driver for their own reasons.

But there's no point in using load_intel_pstate here - it should be a variable 
local to
that driver. Simply call the function unconditionally and check the variable 
first thing
inside the function.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-06-04 Thread Wang, Wei W 
On 04/06/2015 09:18, Wei Wang wrote
> On 03/06/2015 19:51, Jan Beulich wrote
> > >>> On 03.06.15 at 10:07,  wrote:
> > > On 26/05/2015 22:02, Jan Beulich wrote
> > >> >>> On 13.05.16 at 09:51,  wrote:
> > >> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > >> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > >> > --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> > >> > +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> > >> > @@ -766,6 +766,8 @@ static struct cpufreq_driver
> > >> > intel_pstate_driver =
> > {
> > >> >  .name = "intel_pstate",
> > >> >  };
> > >> >
> > >> > +int __initdata load_intel_pstate = 0;
> > >>
> > >> static bool_t
> > >
> > > I think we cannot use "static" here, since load_intel_pstate is also
> > > used in cpufreq.c to select which driver to load.
> >
> > Iirc I had also requested to deal with that (as it looks pretty hackish 
> > right
> now).
> 
> I'm not sure about this. Can you please elaborate it - why it looks hackish by
> doing so?
> What is your preferred way to do it? Thanks.
> 
> The following is your another comment on "load_intel_pstate":
> 
> >> @@ -650,9 +650,12 @@ 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)) {
> > >+if (load_intel_pstate)
> > >+ret = intel_pstate_init();
> > >+if (!load_intel_pstate)
> > >+ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> 
> >I don't see why you need load_intel_pstate here: Simply call the
> >original function whenever
> >intel_pstate_init() returns an error.
> 
> I plan to change it to:
>   if (load_intel_pstate)
>   ret = intel_pstate_init();
>   if (ret)
>   ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> 
> This allows the case that the machine supports the intel_pstate driver but
> people just prefer to use the old driver for their own reasons.


Missed one thing, it should be like this:
if (load_intel_pstate)
ret = intel_pstate_init();
if (ret || ! load_intel_pstate)
ret = cpufreq_register_driver(&acpi_cpufreq_driver);
 
 Best,
 Wei

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-06-03 Thread Wang, Wei W 
On 03/06/2015 19:51, Jan Beulich wrote
> >>> On 03.06.15 at 10:07,  wrote:
> > On 26/05/2015 22:02, Jan Beulich wrote
> >> >>> On 13.05.16 at 09:51,  wrote:
> >> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >> > --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> >> > +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> >> > @@ -766,6 +766,8 @@ static struct cpufreq_driver intel_pstate_driver =
> {
> >> >  .name = "intel_pstate",
> >> >  };
> >> >
> >> > +int __initdata load_intel_pstate = 0;
> >>
> >> static bool_t
> >
> > I think we cannot use "static" here, since load_intel_pstate is also
> > used in cpufreq.c to select which driver to load.
> 
> Iirc I had also requested to deal with that (as it looks pretty hackish right 
> now).

I'm not sure about this. Can you please elaborate it - why it looks hackish by 
doing so? 
What is your preferred way to do it? Thanks.

The following is your another comment on "load_intel_pstate":

>> @@ -650,9 +650,12 @@ 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)) {
> >+if (load_intel_pstate)
> >+ret = intel_pstate_init();
> >+if (!load_intel_pstate)
> >+ret = cpufreq_register_driver(&acpi_cpufreq_driver);

>I don't see why you need load_intel_pstate here: Simply call the original 
>function whenever 
>intel_pstate_init() returns an error.

I plan to change it to:
if (load_intel_pstate)
ret = intel_pstate_init();
if (ret)
ret = cpufreq_register_driver(&acpi_cpufreq_driver);

This allows the case that the machine supports the intel_pstate driver but 
people just prefer to use the old driver for their own reasons.

Best,
Wei

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-06-03 Thread Jan Beulich 
>>> On 03.06.15 at 10:07,  wrote:
> On 26/05/2015 22:02, Jan Beulich wrote
>> >>> On 13.05.16 at 09:51,  wrote:
>> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> > --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
>> > +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
>> > @@ -766,6 +766,8 @@ static struct cpufreq_driver intel_pstate_driver = {
>> >  .name = "intel_pstate",
>> >  };
>> >
>> > +int __initdata load_intel_pstate = 0;
>> 
>> static bool_t
> 
> I think we cannot use "static" here, since load_intel_pstate is also used in 
> cpufreq.c to select which driver to load.

Iirc I had also requested to deal with that (as it looks pretty hackish
right now).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-06-03 Thread Wang, Wei W 
On 26/05/2015 22:02, Jan Beulich wrote
> >>> On 13.05.16 at 09:51,  wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> > +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> > @@ -766,6 +766,8 @@ static struct cpufreq_driver intel_pstate_driver = {
> >  .name = "intel_pstate",
> >  };
> >
> > +int __initdata load_intel_pstate = 0;
> 
> static bool_t

I think we cannot use "static" here, since load_intel_pstate is also used in 
cpufreq.c to select which driver to load.

Best,
Wei


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-05-26 Thread Jan Beulich 
>>> On 13.05.16 at 09:51,  wrote:
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -650,9 +650,12 @@ 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)) {
> +if (load_intel_pstate)
> +ret = intel_pstate_init();
> +if (!load_intel_pstate)
> +ret = cpufreq_register_driver(&acpi_cpufreq_driver);

I don't see why you need load_intel_pstate here: Simply call the
original function whenever intel_pstate_init() returns an error.

> --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> @@ -766,6 +766,8 @@ static struct cpufreq_driver intel_pstate_driver = {
>  .name = "intel_pstate",
>  };
>  
> +int __initdata load_intel_pstate = 0;

static bool_t

> @@ -850,3 +856,14 @@ out:
>  xfree(all_cpu_data);
>  return -ENODEV;
>  }
> +
> +static int __init intel_pstate_setup(char *str)
> +{
> +if (!str)
> +return -EINVAL;
> +if (!strcmp(str, "enable"))
> +load_intel_pstate = 1;
> +
> +return 0;
> +}
> +custom_param("intel_pstate", intel_pstate_setup);

This is an ordinary boolean_param().

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

2015-05-13 Thread Wei Wang 
By default, the old P-state driver (acpi-freq) is used. Adding
"intel_pstate=enable" 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.

Signed-off-by: Wei Wang 
---
 xen/arch/x86/acpi/cpufreq/cpufreq.c  |  9 ++---
 xen/arch/x86/acpi/cpufreq/intel_pstate.c | 21 +++--
 xen/include/asm-x86/acpi.h   |  2 ++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c 
b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index fa3678d..f75f356 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -650,9 +650,12 @@ 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)) {
+if (load_intel_pstate)
+ret = intel_pstate_init();
+if (!load_intel_pstate)
+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 052a0d0..c1a8b11 100644
--- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
+++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
@@ -766,6 +766,8 @@ static struct cpufreq_driver intel_pstate_driver = {
 .name = "intel_pstate",
 };
 
+int __initdata load_intel_pstate = 0;
+
 static int intel_pstate_msrs_not_valid(void)
 {
 /* Check that all the msr's we are using are valid. */
@@ -819,10 +821,14 @@ int __init intel_pstate_init(void)
 if (cpuid_ecx(6) & 0x1)
 set_bit(X86_FEATURE_APERFMPERF, &boot_cpu_data.x86_capability);
 
-id = x86_match_cpu(intel_pstate_cpu_ids);
-if (!id)
+if (!load_intel_pstate)
 return -ENODEV;
 
+id = x86_match_cpu(intel_pstate_cpu_ids);
+if (!id) {
+load_intel_pstate = 0;
+return -ENODEV;
+}
 cpu_info = (struct cpu_defaults *)id->driver_data;
 
 copy_pid_params(&cpu_info->pid_policy);
@@ -850,3 +856,14 @@ out:
 xfree(all_cpu_data);
 return -ENODEV;
 }
+
+static int __init intel_pstate_setup(char *str)
+{
+if (!str)
+return -EINVAL;
+if (!strcmp(str, "enable"))
+load_intel_pstate = 1;
+
+return 0;
+}
+custom_param("intel_pstate", intel_pstate_setup);
diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
index 505d7e7..1a97545 100644
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -32,6 +32,8 @@
 #define COMPILER_DEPENDENT_INT64   long long
 #define COMPILER_DEPENDENT_UINT64  unsigned long long
 
+extern int load_intel_pstate;
+
 extern int intel_pstate_init(void);
 
 /*
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel