[PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Venkatesh Srinivas]

CPUs which should support the RAPL counters according to
Family/Model/Stepping may still issue #GP when attempting to access
the RAPL MSRs. This may happen when Linux is running under KVM and
we are passing-through host F/M/S data, for example. Use rdmsrl_safe
to first access the RAPL_POWER_UNIT MSR; if this fails, do not
attempt to use this PMU.

Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
---
 arch/x86/kernel/cpu/perf_event_intel_rapl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
index 5ad35ad..95700e5 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
@@ -511,6 +511,7 @@ static int rapl_cpu_prepare(int cpu)
 	struct rapl_pmu *pmu = per_cpu(rapl_pmu, cpu);
 	int phys_id = topology_physical_package_id(cpu);
 	u64 ms;
+	u64 msr_rapl_power_unit_bits;
 
 	if (pmu)
 		return 0;
@@ -518,6 +519,9 @@ static int rapl_cpu_prepare(int cpu)
 	if (phys_id < 0)
 		return -1;
 
+	if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
+		return -1;
+
 	pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
 	if (!pmu)
 		return -1;
@@ -531,8 +535,7 @@ static int rapl_cpu_prepare(int cpu)
 	 *
 	 * we cache in local PMU instance
 	 */
-	rdmsrl(MSR_RAPL_POWER_UNIT, pmu->hw_unit);
-	pmu->hw_unit = (pmu->hw_unit >> 8) & 0x1FULL;
+	pmu->hw_unit = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 	pmu->pmu = &rapl_pmu_class;
 
 	/*
@@ -649,7 +652,9 @@ static int __init rapl_pmu_init(void)
 	get_online_cpus();
 
 	for_each_online_cpu(cpu) {
-		rapl_cpu_prepare(cpu);
+		ret = rapl_cpu_prepare(cpu);
+		if (ret)
+			goto out;
 		rapl_cpu_init(cpu);
 	}
 
@@ -672,6 +677,7 @@ static int __init rapl_pmu_init(void)
 		hweight32(rapl_cntr_mask),
 		ktime_to_ms(pmu->timer_interval));
 
+out:
 	put_online_cpus();
 
 	return 0;
-- 
1.9.0.279.gdc9e3eb

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Peter Zijlstra]

On Thu, Mar 13, 2014 at 12:36:26PM -0700, Venkatesh Srinivas wrote:
> CPUs which should support the RAPL counters according to
> Family/Model/Stepping may still issue #GP when attempting to access
> the RAPL MSRs. This may happen when Linux is running under KVM and
> we are passing-through host F/M/S data, for example. Use rdmsrl_safe
> to first access the RAPL_POWER_UNIT MSR; if this fails, do not
> attempt to use this PMU.

This kvm thing is getting tedious :-(

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Andi Kleen]

On Fri, Mar 14, 2014 at 09:44:29AM +0100, Peter Zijlstra wrote:
> On Thu, Mar 13, 2014 at 12:36:26PM -0700, Venkatesh Srinivas wrote:
> > CPUs which should support the RAPL counters according to
> > Family/Model/Stepping may still issue #GP when attempting to access
> > the RAPL MSRs. This may happen when Linux is running under KVM and
> > we are passing-through host F/M/S data, for example. Use rdmsrl_safe
> > to first access the RAPL_POWER_UNIT MSR; if this fails, do not
> > attempt to use this PMU.
> 
> This kvm thing is getting tedious :-(

It sounds bogus to me too, VMs are supposed to return 0 
on reading unknown MSRs, no #GP.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Venkatesh Srinivas]

On Fri, Mar 14, 2014 at 6:56 AM, Andi Kleen <ak@linux.intel.com> wrote:
> On Fri, Mar 14, 2014 at 09:44:29AM +0100, Peter Zijlstra wrote:
>> On Thu, Mar 13, 2014 at 12:36:26PM -0700, Venkatesh Srinivas wrote:
>> > CPUs which should support the RAPL counters according to
>> > Family/Model/Stepping may still issue #GP when attempting to access
>> > the RAPL MSRs. This may happen when Linux is running under KVM and
>> > we are passing-through host F/M/S data, for example. Use rdmsrl_safe
>> > to first access the RAPL_POWER_UNIT MSR; if this fails, do not
>> > attempt to use this PMU.
>>
>> This kvm thing is getting tedious :-(
>
> It sounds bogus to me too, VMs are supposed to return 0
> on reading unknown MSRs, no #GP.

The Intel ISR section for RDMSR seems to say: "Specifying a reserved
or unimplemented
MSR address in ECX will also cause a general protection exception".

>From a guest's perspective, MSR_RAPL_POWER_UNIT is unimplemented; kvm matches
this behavior.

Thanks,
-- vs;

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Andi Kleen]

> The Intel ISR section for RDMSR seems to say: "Specifying a reserved
> or unimplemented
> MSR address in ECX will also cause a general protection exception".
> 
> From a guest's perspective, MSR_RAPL_POWER_UNIT is unimplemented; kvm matches
> this behavior.

MSRs are model specific and defined per model number. If you report a model
number you're expected to implement the MSRs defined for that model number.

AFAIK Xen just reports 0 for unknown MSRs (and I'm surprised KVM doesn't too)

I would suggest to fix KVM.

-Andi 

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[David Ahern]

On 3/14/14, 10:17 AM, Andi Kleen wrote:
>> The Intel ISR section for RDMSR seems to say: "Specifying a reserved
>> or unimplemented
>> MSR address in ECX will also cause a general protection exception".
>>
>>  From a guest's perspective, MSR_RAPL_POWER_UNIT is unimplemented; kvm matches
>> this behavior.
>
> MSRs are model specific and defined per model number. If you report a model
> number you're expected to implement the MSRs defined for that model number.
>
> AFAIK Xen just reports 0 for unknown MSRs (and I'm surprised KVM doesn't too)
>
> I would suggest to fix KVM.

I believe ignore_msrs parameter to kvm handles that.

David

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Venkatesh Srinivas]

On Fri, Mar 14, 2014 at 10:57:58AM -0600, David Ahern wrote:
>On 3/14/14, 10:17 AM, Andi Kleen wrote:
>>>The Intel ISR section for RDMSR seems to say: "Specifying a reserved
>>>or unimplemented
>>>MSR address in ECX will also cause a general protection exception".
>>>
>>> From a guest's perspective, MSR_RAPL_POWER_UNIT is unimplemented; kvm matches
>>>this behavior.
>>
>>MSRs are model specific and defined per model number. If you report a model
>>number you're expected to implement the MSRs defined for that model number.
>>
>>AFAIK Xen just reports 0 for unknown MSRs (and I'm surprised KVM doesn't too)
>>
>>I would suggest to fix KVM.
>
>I believe ignore_msrs parameter to kvm handles that.
>
>David

Hi,

cc-ing the virtualization mailing list for more detail on the kvm
default for ignore_msrs (it defaults off).

1) Just returning 0 for unsupported MSRs is not workable -- 0 may be a
    meaningful value for an MSR. RDMSR/WRMSR already have a mechanism
    for out-of-band errors, #GP.

2) #GP has been KVM's default behavior for quite some time. Even if we
    believe changing KVM's default is appropriate, Linux w/ the RAPL PMU
    code enabled will fail to boot on existing KVM versions. W/ this
    change, Linux will boot on prior KVM versions.

Thanks,
-- vs;

[tip:perf/core] perf/x86/intel: Use rdmsrl_safe() when initializing RAPL PMU

[tip-bot for Venkatesh Srinivas]

Commit-ID:  24223657806a0ebd0ae5c9caaf7b021091889cf2
Gitweb:     http://git.kernel.org/tip/24223657806a0ebd0ae5c9caaf7b021091889cf2
Author:     Venkatesh Srinivas <venkateshs@google.com>
AuthorDate: Thu, 13 Mar 2014 12:36:26 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 18 Apr 2014 12:14:26 +0200

perf/x86/intel: Use rdmsrl_safe() when initializing RAPL PMU

CPUs which should support the RAPL counters according to
Family/Model/Stepping may still issue #GP when attempting to access
the RAPL MSRs. This may happen when Linux is running under KVM and
we are passing-through host F/M/S data, for example. Use rdmsrl_safe
to first access the RAPL_POWER_UNIT MSR; if this fails, do not
attempt to use this PMU.

Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1394739386-22260-1-git-send-email-venkateshs@google.com
Cc: zheng.z.yan@intel.com
Cc: eranian@google.com
Cc: ak@linux.intel.com
Cc: linux-kernel@vger.kernel.org
[ The patch also silently fixes another bug: rapl_pmu_init() didn't handle the memory alloc failure case previously. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_intel_rapl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
index 4b9a9e9..7c87424 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
@@ -535,6 +535,7 @@ static int rapl_cpu_prepare(int cpu)
 	struct rapl_pmu *pmu = per_cpu(rapl_pmu, cpu);
 	int phys_id = topology_physical_package_id(cpu);
 	u64 ms;
+	u64 msr_rapl_power_unit_bits;
 
 	if (pmu)
 		return 0;
@@ -542,6 +543,9 @@ static int rapl_cpu_prepare(int cpu)
 	if (phys_id < 0)
 		return -1;
 
+	if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
+		return -1;
+
 	pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
 	if (!pmu)
 		return -1;
@@ -555,8 +559,7 @@ static int rapl_cpu_prepare(int cpu)
 	 *
 	 * we cache in local PMU instance
 	 */
-	rdmsrl(MSR_RAPL_POWER_UNIT, pmu->hw_unit);
-	pmu->hw_unit = (pmu->hw_unit >> 8) & 0x1FULL;
+	pmu->hw_unit = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 	pmu->pmu = &rapl_pmu_class;
 
 	/*
@@ -677,7 +680,9 @@ static int __init rapl_pmu_init(void)
 	cpu_notifier_register_begin();
 
 	for_each_online_cpu(cpu) {
-		rapl_cpu_prepare(cpu);
+		ret = rapl_cpu_prepare(cpu);
+		if (ret)
+			goto out;
 		rapl_cpu_init(cpu);
 	}
 
@@ -700,6 +705,7 @@ static int __init rapl_pmu_init(void)
 		hweight32(rapl_cntr_mask),
 		ktime_to_ms(pmu->timer_interval));
 
+out:
 	cpu_notifier_register_done();
 
 	return 0;

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Stephane Eranian]

On Thu, Mar 13, 2014 at 8:36 PM, Venkatesh Srinivas
<venkateshs@google.com> wrote:
> CPUs which should support the RAPL counters according to
> Family/Model/Stepping may still issue #GP when attempting to access
> the RAPL MSRs. This may happen when Linux is running under KVM and
> we are passing-through host F/M/S data, for example. Use rdmsrl_safe
> to first access the RAPL_POWER_UNIT MSR; if this fails, do not
> attempt to use this PMU.
>
> Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
> ---
>  arch/x86/kernel/cpu/perf_event_intel_rapl.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
> index 5ad35ad..95700e5 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
> @@ -511,6 +511,7 @@ static int rapl_cpu_prepare(int cpu)
>         struct rapl_pmu *pmu = per_cpu(rapl_pmu, cpu);
>         int phys_id = topology_physical_package_id(cpu);
>         u64 ms;
> +       u64 msr_rapl_power_unit_bits;
>
>         if (pmu)
>                 return 0;
> @@ -518,6 +519,9 @@ static int rapl_cpu_prepare(int cpu)
>         if (phys_id < 0)
>                 return -1;
>
> +       if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
> +               return -1;
> +
I have a problem with this patch on native hardware. This
rdmsrl_safe() systematically
fails when I know the MSR is perfectly valid on the CPU. Consequently, RAPL PMU
is disabled when I tried on IvyBridge and Haswell CPUs.

I don't know the internals of rdmsrl_safe(). Maybe it is invoked too
early in the boot process.

Please fix this.

>         pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
>         if (!pmu)
>                 return -1;
> @@ -531,8 +535,7 @@ static int rapl_cpu_prepare(int cpu)
>          *
>          * we cache in local PMU instance
>          */
> -       rdmsrl(MSR_RAPL_POWER_UNIT, pmu->hw_unit);
> -       pmu->hw_unit = (pmu->hw_unit >> 8) & 0x1FULL;
> +       pmu->hw_unit = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
>         pmu->pmu = &rapl_pmu_class;
>
>         /*
> @@ -649,7 +652,9 @@ static int __init rapl_pmu_init(void)
>         get_online_cpus();
>
>         for_each_online_cpu(cpu) {
> -               rapl_cpu_prepare(cpu);
> +               ret = rapl_cpu_prepare(cpu);
> +               if (ret)
> +                       goto out;
>                 rapl_cpu_init(cpu);
>         }
>
> @@ -672,6 +677,7 @@ static int __init rapl_pmu_init(void)
>                 hweight32(rapl_cntr_mask),
>                 ktime_to_ms(pmu->timer_interval));
>
> +out:
>         put_online_cpus();
>
>         return 0;
> --
> 1.9.0.279.gdc9e3eb
>

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Peter Zijlstra]

On Wed, Apr 23, 2014 at 04:31:32PM +0200, Stephane Eranian wrote:
> On Thu, Mar 13, 2014 at 8:36 PM, Venkatesh Srinivas
> <venkateshs@google.com> wrote:
> > CPUs which should support the RAPL counters according to
> > Family/Model/Stepping may still issue #GP when attempting to access
> > the RAPL MSRs. This may happen when Linux is running under KVM and
> > we are passing-through host F/M/S data, for example. Use rdmsrl_safe
> > to first access the RAPL_POWER_UNIT MSR; if this fails, do not
> > attempt to use this PMU.
> >
> > Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
> > ---
> >  arch/x86/kernel/cpu/perf_event_intel_rapl.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
> > index 5ad35ad..95700e5 100644
> > --- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
> > @@ -511,6 +511,7 @@ static int rapl_cpu_prepare(int cpu)
> >         struct rapl_pmu *pmu = per_cpu(rapl_pmu, cpu);
> >         int phys_id = topology_physical_package_id(cpu);
> >         u64 ms;
> > +       u64 msr_rapl_power_unit_bits;
> >
> >         if (pmu)
> >                 return 0;
> > @@ -518,6 +519,9 @@ static int rapl_cpu_prepare(int cpu)
> >         if (phys_id < 0)
> >                 return -1;
> >
> > +       if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
> > +               return -1;
> > +
> I have a problem with this patch on native hardware. This
> rdmsrl_safe() systematically
> fails when I know the MSR is perfectly valid on the CPU. Consequently, RAPL PMU
> is disabled when I tried on IvyBridge and Haswell CPUs.
> 
> I don't know the internals of rdmsrl_safe(). Maybe it is invoked too
> early in the boot process.

Weird; so the way it works is that it adds an exception table entry for
the wrmsr instruction, so when the wrmsr generates a fault due to being
an invalid msr the fault handler looks at the exception table, and finds
the entry, which instructs it to continue execution at the error path
and return -EFAULT.

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Stephane Eranian]

On Wed, Apr 23, 2014 at 4:45 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Apr 23, 2014 at 04:31:32PM +0200, Stephane Eranian wrote:
>> On Thu, Mar 13, 2014 at 8:36 PM, Venkatesh Srinivas
>> <venkateshs@google.com> wrote:
>> > CPUs which should support the RAPL counters according to
>> > Family/Model/Stepping may still issue #GP when attempting to access
>> > the RAPL MSRs. This may happen when Linux is running under KVM and
>> > we are passing-through host F/M/S data, for example. Use rdmsrl_safe
>> > to first access the RAPL_POWER_UNIT MSR; if this fails, do not
>> > attempt to use this PMU.
>> >
>> > Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
>> > ---
>> >  arch/x86/kernel/cpu/perf_event_intel_rapl.c | 12 +++++++++---
>> >  1 file changed, 9 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
>> > index 5ad35ad..95700e5 100644
>> > --- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
>> > +++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
>> > @@ -511,6 +511,7 @@ static int rapl_cpu_prepare(int cpu)
>> >         struct rapl_pmu *pmu = per_cpu(rapl_pmu, cpu);
>> >         int phys_id = topology_physical_package_id(cpu);
>> >         u64 ms;
>> > +       u64 msr_rapl_power_unit_bits;
>> >
>> >         if (pmu)
>> >                 return 0;
>> > @@ -518,6 +519,9 @@ static int rapl_cpu_prepare(int cpu)
>> >         if (phys_id < 0)
>> >                 return -1;
>> >
>> > +       if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
>> > +               return -1;
>> > +
>> I have a problem with this patch on native hardware. This
>> rdmsrl_safe() systematically
>> fails when I know the MSR is perfectly valid on the CPU. Consequently, RAPL PMU
>> is disabled when I tried on IvyBridge and Haswell CPUs.
>>
>> I don't know the internals of rdmsrl_safe(). Maybe it is invoked too
>> early in the boot process.
>
> Weird; so the way it works is that it adds an exception table entry for
> the wrmsr instruction, so when the wrmsr generates a fault due to being
> an invalid msr the fault handler looks at the exception table, and finds
> the entry, which instructs it to continue execution at the error path
> and return -EFAULT.
>
On your machine, booted with 3.15-rc2, do you have /sys/devices/power?
If not, and you have at least an SNB, you should have RAPL and that
RAPL_UNIT MSR.

Proof is that if you read that MSR using /dev/cpu/msr it works just fine:
# modprobe msr
# rdmsr 0x606
a1003

So something is broken somewhere.

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Peter Zijlstra]

On Wed, Apr 23, 2014 at 04:49:55PM +0200, Stephane Eranian wrote:
> On your machine, booted with 3.15-rc2, do you have /sys/devices/power?
> If not, and you have at least an SNB, you should have RAPL and that
> RAPL_UNIT MSR.
> 
> Proof is that if you read that MSR using /dev/cpu/msr it works just fine:
> # modprobe msr
> # rdmsr 0x606
> a1003
> 
> So something is broken somewhere.

I've not got SNB+ class hardware (for testing). But I believe you that
there's something funny, I just do not understanding how *msr_safe()
could cause this.

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Stephane Eranian]

On Wed, Apr 23, 2014 at 5:09 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Apr 23, 2014 at 04:49:55PM +0200, Stephane Eranian wrote:
>> On your machine, booted with 3.15-rc2, do you have /sys/devices/power?
>> If not, and you have at least an SNB, you should have RAPL and that
>> RAPL_UNIT MSR.
>>
>> Proof is that if you read that MSR using /dev/cpu/msr it works just fine:
>> # modprobe msr
>> # rdmsr 0x606
>> a1003
>>
>> So something is broken somewhere.
>
> I've not got SNB+ class hardware (for testing). But I believe you that
> there's something funny, I just do not understanding how *msr_safe()
> could cause this.

Or the logic of the test in rapl_cpu_prepare() is wrong, maybe?
Wouldn't rdmsrl_safe() return 0 on success?

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Borislav Petkov]

On Wed, Apr 23, 2014 at 05:14:33PM +0200, Stephane Eranian wrote:
> Wouldn't rdmsrl_safe() return 0 on success?

Yes.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Stephane Eranian]

On Wed, Apr 23, 2014 at 5:16 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Apr 23, 2014 at 05:14:33PM +0200, Stephane Eranian wrote:
>> Wouldn't rdmsrl_safe() return 0 on success?
>
> Yes.
>
then the if() test is wrong:
if (!rdmsrl_safe())
   return -1;

Should be:
if (rdmsrl_safe())
  return -1;

Or am I missing something?


> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Borislav Petkov]

On Wed, Apr 23, 2014 at 05:18:29PM +0200, Stephane Eranian wrote:
> On Wed, Apr 23, 2014 at 5:16 PM, Borislav Petkov <bp@alien8.de> wrote:
> > On Wed, Apr 23, 2014 at 05:14:33PM +0200, Stephane Eranian wrote:
> >> Wouldn't rdmsrl_safe() return 0 on success?
> >
> > Yes.
> >
> then the if() test is wrong:
> if (!rdmsrl_safe())
>    return -1;
> 
> Should be:
> if (rdmsrl_safe())
>   return -1;
> 
> Or am I missing something?

Yeah, that is wrong:

        if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
                return -1;

On error we return -EIO, on success 0. Just remove the "!".


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Stephane Eranian]

On Wed, Apr 23, 2014 at 5:35 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Apr 23, 2014 at 05:18:29PM +0200, Stephane Eranian wrote:
>> On Wed, Apr 23, 2014 at 5:16 PM, Borislav Petkov <bp@alien8.de> wrote:
>> > On Wed, Apr 23, 2014 at 05:14:33PM +0200, Stephane Eranian wrote:
>> >> Wouldn't rdmsrl_safe() return 0 on success?
>> >
>> > Yes.
>> >
>> then the if() test is wrong:
>> if (!rdmsrl_safe())
>>    return -1;
>>
>> Should be:
>> if (rdmsrl_safe())
>>   return -1;
>>
>> Or am I missing something?
>
> Yeah, that is wrong:
>
>         if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
>                 return -1;
>
> On error we return -EIO, on success 0. Just remove the "!".
>
Yeah, will post a patch ASAP. A comment describing return values
where rdmsrl_safe() is defined would maybe help avoid problems in the future.
Thanks.

>
> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --

Re: [PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Borislav Petkov]

On Wed, Apr 23, 2014 at 05:44:30PM +0200, Stephane Eranian wrote:
> A comment describing return values where rdmsrl_safe() is defined
> would maybe help avoid problems in the future.

If it helps, sure. But please as a separate patch and to x86 guys.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH] perf/x86/intel: Use rdmsrl_safe when initializing RAPL PMU.

[Venkatesh Srinivas]

CPUs which should support the RAPL counters according to
Family/Model/Stepping may still issue #GP when attempting to access
the RAPL MSRs. This may happen when Linux is running under KVM and
we are passing-through host F/M/S data, for example. Use rdmsrl_safe
to first access the RAPL_POWER_UNIT MSR; if this fails, do not
attempt to use this PMU.

Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
---
 arch/x86/kernel/cpu/perf_event_intel_rapl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
index 5ad35ad..95700e5 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
@@ -511,6 +511,7 @@ static int rapl_cpu_prepare(int cpu)
 	struct rapl_pmu *pmu = per_cpu(rapl_pmu, cpu);
 	int phys_id = topology_physical_package_id(cpu);
 	u64 ms;
+	u64 msr_rapl_power_unit_bits;
 
 	if (pmu)
 		return 0;
@@ -518,6 +519,9 @@ static int rapl_cpu_prepare(int cpu)
 	if (phys_id < 0)
 		return -1;
 
+	if (!rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
+		return -1;
+
 	pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
 	if (!pmu)
 		return -1;
@@ -531,8 +535,7 @@ static int rapl_cpu_prepare(int cpu)
 	 *
 	 * we cache in local PMU instance
 	 */
-	rdmsrl(MSR_RAPL_POWER_UNIT, pmu->hw_unit);
-	pmu->hw_unit = (pmu->hw_unit >> 8) & 0x1FULL;
+	pmu->hw_unit = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 	pmu->pmu = &rapl_pmu_class;
 
 	/*
@@ -649,7 +652,9 @@ static int __init rapl_pmu_init(void)
 	get_online_cpus();
 
 	for_each_online_cpu(cpu) {
-		rapl_cpu_prepare(cpu);
+		ret = rapl_cpu_prepare(cpu);
+		if (ret)
+			goto out;
 		rapl_cpu_init(cpu);
 	}
 
@@ -672,6 +677,7 @@ static int __init rapl_pmu_init(void)
 		hweight32(rapl_cntr_mask),
 		ktime_to_ms(pmu->timer_interval));
 
+out:
 	put_online_cpus();
 
 	return 0;
-- 
1.9.0.279.gdc9e3eb