2013-03-20 16:17:36

by Dirk Brandewie

[permalink] [raw]
Subject: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

From: Dirk Brandewie <[email protected]>

Some VMs seem to try to implement some MSRs but not all the registers
the driver needs. Check to make sure all the MSR that we need are
available. If any of the required MSRs are not available refuse to
load.

Signed-off-by: Dirk Brandewie <[email protected]>
---
drivers/cpufreq/intel_pstate.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index f6dd1e7..cd9c5f4 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -752,6 +752,29 @@ static struct cpufreq_driver intel_pstate_driver = {

static int __initdata no_load;

+static int intel_pstate_msrs_not_valid(void)
+{
+ /* Check that all the msr's we are using are valid. */
+ u64 aperf, mperf, tmp;
+
+ rdmsrl(MSR_IA32_APERF, aperf);
+ rdmsrl(MSR_IA32_MPERF, mperf);
+
+ if (!intel_pstate_min_pstate() ||
+ !intel_pstate_max_pstate() ||
+ !intel_pstate_turbo_pstate())
+ return -ENODEV;
+
+ rdmsrl(MSR_IA32_APERF, tmp);
+ if (!(tmp - aperf))
+ return -ENODEV;
+
+ rdmsrl(MSR_IA32_MPERF, tmp);
+ if (!(tmp - mperf))
+ return -ENODEV;
+
+ return 0;
+}
static int __init intel_pstate_init(void)
{
int cpu, rc = 0;
@@ -764,6 +787,9 @@ static int __init intel_pstate_init(void)
if (!id)
return -ENODEV;

+ if (intel_pstate_msrs_not_valid())
+ return -ENODEV;
+
pr_info("Intel P-state driver initializing.\n");

all_cpu_data = vmalloc(sizeof(void *) * num_possible_cpus());
--
1.7.7.6


2013-03-20 18:21:27

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On Wednesday, March 20, 2013 09:17:24 AM [email protected] wrote:
> From: Dirk Brandewie <[email protected]>
>
> Some VMs seem to try to implement some MSRs but not all the registers
> the driver needs. Check to make sure all the MSR that we need are
> available. If any of the required MSRs are not available refuse to
> load.
>
> Signed-off-by: Dirk Brandewie <[email protected]>

Is this needed for v3.9? Any pointers to bug reports etc.?

Rafael


> ---
> drivers/cpufreq/intel_pstate.c | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f6dd1e7..cd9c5f4 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -752,6 +752,29 @@ static struct cpufreq_driver intel_pstate_driver = {
>
> static int __initdata no_load;
>
> +static int intel_pstate_msrs_not_valid(void)
> +{
> + /* Check that all the msr's we are using are valid. */
> + u64 aperf, mperf, tmp;
> +
> + rdmsrl(MSR_IA32_APERF, aperf);
> + rdmsrl(MSR_IA32_MPERF, mperf);
> +
> + if (!intel_pstate_min_pstate() ||
> + !intel_pstate_max_pstate() ||
> + !intel_pstate_turbo_pstate())
> + return -ENODEV;
> +
> + rdmsrl(MSR_IA32_APERF, tmp);
> + if (!(tmp - aperf))
> + return -ENODEV;
> +
> + rdmsrl(MSR_IA32_MPERF, tmp);
> + if (!(tmp - mperf))
> + return -ENODEV;
> +
> + return 0;
> +}
> static int __init intel_pstate_init(void)
> {
> int cpu, rc = 0;
> @@ -764,6 +787,9 @@ static int __init intel_pstate_init(void)
> if (!id)
> return -ENODEV;
>
> + if (intel_pstate_msrs_not_valid())
> + return -ENODEV;
> +
> pr_info("Intel P-state driver initializing.\n");
>
> all_cpu_data = vmalloc(sizeof(void *) * num_possible_cpus());
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2013-03-20 18:36:25

by Dirk Brandewie

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On 03/20/2013 11:28 AM, Rafael J. Wysocki wrote:
> On Wednesday, March 20, 2013 09:17:24 AM [email protected] wrote:
>> From: Dirk Brandewie <[email protected]>
>>
>> Some VMs seem to try to implement some MSRs but not all the registers
>> the driver needs. Check to make sure all the MSR that we need are
>> available. If any of the required MSRs are not available refuse to
>> load.
>>
>> Signed-off-by: Dirk Brandewie <[email protected]>
>
> Is this needed for v3.9? Any pointers to bug reports etc.?
>

Sorry I saw right after I sent the mail that the bug report was missing

https://bugzilla.redhat.com/show_bug.cgi?id=922923
Reported-by: Josh Stone <[email protected]>

Would you like me to spin the patch?

> Rafael
>
>
>> ---
>> drivers/cpufreq/intel_pstate.c | 26 ++++++++++++++++++++++++++
>> 1 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index f6dd1e7..cd9c5f4 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -752,6 +752,29 @@ static struct cpufreq_driver intel_pstate_driver = {
>>
>> static int __initdata no_load;
>>
>> +static int intel_pstate_msrs_not_valid(void)
>> +{
>> + /* Check that all the msr's we are using are valid. */
>> + u64 aperf, mperf, tmp;
>> +
>> + rdmsrl(MSR_IA32_APERF, aperf);
>> + rdmsrl(MSR_IA32_MPERF, mperf);
>> +
>> + if (!intel_pstate_min_pstate() ||
>> + !intel_pstate_max_pstate() ||
>> + !intel_pstate_turbo_pstate())
>> + return -ENODEV;
>> +
>> + rdmsrl(MSR_IA32_APERF, tmp);
>> + if (!(tmp - aperf))
>> + return -ENODEV;
>> +
>> + rdmsrl(MSR_IA32_MPERF, tmp);
>> + if (!(tmp - mperf))
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>> static int __init intel_pstate_init(void)
>> {
>> int cpu, rc = 0;
>> @@ -764,6 +787,9 @@ static int __init intel_pstate_init(void)
>> if (!id)
>> return -ENODEV;
>>
>> + if (intel_pstate_msrs_not_valid())
>> + return -ENODEV;
>> +
>> pr_info("Intel P-state driver initializing.\n");
>>
>> all_cpu_data = vmalloc(sizeof(void *) * num_possible_cpus());
>>

2013-03-21 00:00:48

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On Wednesday, March 20, 2013 11:28:49 AM Dirk Brandewie wrote:
> On 03/20/2013 11:28 AM, Rafael J. Wysocki wrote:
> > On Wednesday, March 20, 2013 09:17:24 AM [email protected] wrote:
> >> From: Dirk Brandewie <[email protected]>
> >>
> >> Some VMs seem to try to implement some MSRs but not all the registers
> >> the driver needs. Check to make sure all the MSR that we need are
> >> available. If any of the required MSRs are not available refuse to
> >> load.
> >>
> >> Signed-off-by: Dirk Brandewie <[email protected]>
> >
> > Is this needed for v3.9? Any pointers to bug reports etc.?
> >
>
> Sorry I saw right after I sent the mail that the bug report was missing
>
> https://bugzilla.redhat.com/show_bug.cgi?id=922923
> Reported-by: Josh Stone <[email protected]>
>
> Would you like me to spin the patch?

No, thanks, this information is sufficient.

Thanks,
Rafael


> >> ---
> >> drivers/cpufreq/intel_pstate.c | 26 ++++++++++++++++++++++++++
> >> 1 files changed, 26 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >> index f6dd1e7..cd9c5f4 100644
> >> --- a/drivers/cpufreq/intel_pstate.c
> >> +++ b/drivers/cpufreq/intel_pstate.c
> >> @@ -752,6 +752,29 @@ static struct cpufreq_driver intel_pstate_driver = {
> >>
> >> static int __initdata no_load;
> >>
> >> +static int intel_pstate_msrs_not_valid(void)
> >> +{
> >> + /* Check that all the msr's we are using are valid. */
> >> + u64 aperf, mperf, tmp;
> >> +
> >> + rdmsrl(MSR_IA32_APERF, aperf);
> >> + rdmsrl(MSR_IA32_MPERF, mperf);
> >> +
> >> + if (!intel_pstate_min_pstate() ||
> >> + !intel_pstate_max_pstate() ||
> >> + !intel_pstate_turbo_pstate())
> >> + return -ENODEV;
> >> +
> >> + rdmsrl(MSR_IA32_APERF, tmp);
> >> + if (!(tmp - aperf))
> >> + return -ENODEV;
> >> +
> >> + rdmsrl(MSR_IA32_MPERF, tmp);
> >> + if (!(tmp - mperf))
> >> + return -ENODEV;
> >> +
> >> + return 0;
> >> +}
> >> static int __init intel_pstate_init(void)
> >> {
> >> int cpu, rc = 0;
> >> @@ -764,6 +787,9 @@ static int __init intel_pstate_init(void)
> >> if (!id)
> >> return -ENODEV;
> >>
> >> + if (intel_pstate_msrs_not_valid())
> >> + return -ENODEV;
> >> +
> >> pr_info("Intel P-state driver initializing.\n");
> >>
> >> all_cpu_data = vmalloc(sizeof(void *) * num_possible_cpus());
> >>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2013-03-21 03:20:12

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On Wed, Mar 20, 2013 at 9:47 PM, <[email protected]> wrote:
> From: Dirk Brandewie <[email protected]>
>
> Some VMs seem to try to implement some MSRs but not all the registers
> the driver needs. Check to make sure all the MSR that we need are
> available. If any of the required MSRs are not available refuse to
> load.
>
> Signed-off-by: Dirk Brandewie <[email protected]>
> ---
> drivers/cpufreq/intel_pstate.c | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f6dd1e7..cd9c5f4 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -752,6 +752,29 @@ static struct cpufreq_driver intel_pstate_driver = {
>
> static int __initdata no_load;
>
> +static int intel_pstate_msrs_not_valid(void)
> +{
> + /* Check that all the msr's we are using are valid. */
> + u64 aperf, mperf, tmp;
> +
> + rdmsrl(MSR_IA32_APERF, aperf);
> + rdmsrl(MSR_IA32_MPERF, mperf);
> +
> + if (!intel_pstate_min_pstate() ||
> + !intel_pstate_max_pstate() ||
> + !intel_pstate_turbo_pstate())
> + return -ENODEV;
> +
> + rdmsrl(MSR_IA32_APERF, tmp);
> + if (!(tmp - aperf))
> + return -ENODEV;
> +
> + rdmsrl(MSR_IA32_MPERF, tmp);
> + if (!(tmp - mperf))
> + return -ENODEV;
> +
> + return 0;
> +}

Add blank line here.

> static int __init intel_pstate_init(void)
> {
> int cpu, rc = 0;
> @@ -764,6 +787,9 @@ static int __init intel_pstate_init(void)
> if (!id)
> return -ENODEV;
>
> + if (intel_pstate_msrs_not_valid())
> + return -ENODEV;
> +
> pr_info("Intel P-state driver initializing.\n");
>
> all_cpu_data = vmalloc(sizeof(void *) * num_possible_cpus());

Acked-by: Viresh Kumar <[email protected]>

2013-03-22 00:33:32

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On Thursday, March 21, 2013 01:08:03 AM Rafael J. Wysocki wrote:
> On Wednesday, March 20, 2013 11:28:49 AM Dirk Brandewie wrote:
> > On 03/20/2013 11:28 AM, Rafael J. Wysocki wrote:
> > > On Wednesday, March 20, 2013 09:17:24 AM [email protected] wrote:
> > >> From: Dirk Brandewie <[email protected]>
> > >>
> > >> Some VMs seem to try to implement some MSRs but not all the registers
> > >> the driver needs. Check to make sure all the MSR that we need are
> > >> available. If any of the required MSRs are not available refuse to
> > >> load.
> > >>
> > >> Signed-off-by: Dirk Brandewie <[email protected]>
> > >
> > > Is this needed for v3.9? Any pointers to bug reports etc.?
> > >
> >
> > Sorry I saw right after I sent the mail that the bug report was missing
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=922923
> > Reported-by: Josh Stone <[email protected]>
> >
> > Would you like me to spin the patch?
>
> No, thanks, this information is sufficient.

Applied to linux-pm.git/bleeding-edge and will be moved to linux-next after
build testing.

Thanks,
Rafael


> > >> ---
> > >> drivers/cpufreq/intel_pstate.c | 26 ++++++++++++++++++++++++++
> > >> 1 files changed, 26 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > >> index f6dd1e7..cd9c5f4 100644
> > >> --- a/drivers/cpufreq/intel_pstate.c
> > >> +++ b/drivers/cpufreq/intel_pstate.c
> > >> @@ -752,6 +752,29 @@ static struct cpufreq_driver intel_pstate_driver = {
> > >>
> > >> static int __initdata no_load;
> > >>
> > >> +static int intel_pstate_msrs_not_valid(void)
> > >> +{
> > >> + /* Check that all the msr's we are using are valid. */
> > >> + u64 aperf, mperf, tmp;
> > >> +
> > >> + rdmsrl(MSR_IA32_APERF, aperf);
> > >> + rdmsrl(MSR_IA32_MPERF, mperf);
> > >> +
> > >> + if (!intel_pstate_min_pstate() ||
> > >> + !intel_pstate_max_pstate() ||
> > >> + !intel_pstate_turbo_pstate())
> > >> + return -ENODEV;
> > >> +
> > >> + rdmsrl(MSR_IA32_APERF, tmp);
> > >> + if (!(tmp - aperf))
> > >> + return -ENODEV;
> > >> +
> > >> + rdmsrl(MSR_IA32_MPERF, tmp);
> > >> + if (!(tmp - mperf))
> > >> + return -ENODEV;
> > >> +
> > >> + return 0;
> > >> +}
> > >> static int __init intel_pstate_init(void)
> > >> {
> > >> int cpu, rc = 0;
> > >> @@ -764,6 +787,9 @@ static int __init intel_pstate_init(void)
> > >> if (!id)
> > >> return -ENODEV;
> > >>
> > >> + if (intel_pstate_msrs_not_valid())
> > >> + return -ENODEV;
> > >> +
> > >> pr_info("Intel P-state driver initializing.\n");
> > >>
> > >> all_cpu_data = vmalloc(sizeof(void *) * num_possible_cpus());
> > >>
> >
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2013-03-22 15:06:57

by Dave Jones

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On Fri, Mar 22, 2013 at 01:40:49AM +0100, Rafael J. Wysocki wrote:
> On Thursday, March 21, 2013 01:08:03 AM Rafael J. Wysocki wrote:
> > On Wednesday, March 20, 2013 11:28:49 AM Dirk Brandewie wrote:
> > > On 03/20/2013 11:28 AM, Rafael J. Wysocki wrote:
> > > > On Wednesday, March 20, 2013 09:17:24 AM [email protected] wrote:
> > > >> From: Dirk Brandewie <[email protected]>
> > > >>
> > > >> Some VMs seem to try to implement some MSRs but not all the registers
> > > >> the driver needs. Check to make sure all the MSR that we need are
> > > >> available. If any of the required MSRs are not available refuse to
> > > >> load.
> > > >>
> > > >> Signed-off-by: Dirk Brandewie <[email protected]>
> > > >
> > > > Is this needed for v3.9? Any pointers to bug reports etc.?
> > > >
> > >
> > > Sorry I saw right after I sent the mail that the bug report was missing
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=922923
> > > Reported-by: Josh Stone <[email protected]>
> > >
> > > Would you like me to spin the patch?
> >
> > No, thanks, this information is sufficient.
>
> Applied to linux-pm.git/bleeding-edge and will be moved to linux-next after
> build testing.

'next' seems to be imply you're targetting this for 3.10 ?
In case it wasn't clear, this is needed for 3.9.

Dave

2013-03-22 23:59:28

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Add function to check that all MSR's are valid

On Friday, March 22, 2013 11:06:47 AM Dave Jones wrote:
> On Fri, Mar 22, 2013 at 01:40:49AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, March 21, 2013 01:08:03 AM Rafael J. Wysocki wrote:
> > > On Wednesday, March 20, 2013 11:28:49 AM Dirk Brandewie wrote:
> > > > On 03/20/2013 11:28 AM, Rafael J. Wysocki wrote:
> > > > > On Wednesday, March 20, 2013 09:17:24 AM [email protected] wrote:
> > > > >> From: Dirk Brandewie <[email protected]>
> > > > >>
> > > > >> Some VMs seem to try to implement some MSRs but not all the registers
> > > > >> the driver needs. Check to make sure all the MSR that we need are
> > > > >> available. If any of the required MSRs are not available refuse to
> > > > >> load.
> > > > >>
> > > > >> Signed-off-by: Dirk Brandewie <[email protected]>
> > > > >
> > > > > Is this needed for v3.9? Any pointers to bug reports etc.?
> > > > >
> > > >
> > > > Sorry I saw right after I sent the mail that the bug report was missing
> > > >
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=922923
> > > > Reported-by: Josh Stone <[email protected]>
> > > >
> > > > Would you like me to spin the patch?
> > >
> > > No, thanks, this information is sufficient.
> >
> > Applied to linux-pm.git/bleeding-edge and will be moved to linux-next after
> > build testing.
>
> 'next' seems to be imply you're targetting this for 3.10 ?
> In case it wasn't clear, this is needed for 3.9.

I'm going to push this for 3.9 after it has spent a couple of days in
linux-next just in case. So 3.9-rc5 is the target.

Thanks,
Rafael


--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.