[RFC] perf/x86/intel: Disable check_msr for real hw

[RFC] perf/x86/intel: Disable check_msr for real hw

[Jiri Olsa]

hi,
the HPE server can do POST tracing and have enabled LBR
tracing during the boot, which makes check_msr fail falsly.

It looks like check_msr code was added only to check on guests
MSR access, would it be then ok to disable check_msr for real
hardware? (as in patch below)

We could also check if LBR tracing is enabled and make
appropriate checks, but this change is simpler ;-)

ideas? thanks,
jirka


---
 arch/x86/events/intel/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 71001f005bfe..1194ae7e1992 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -20,6 +20,7 @@
 #include <asm/intel-family.h>
 #include <asm/apic.h>
 #include <asm/cpu_device_id.h>
+#include <asm/hypervisor.h>
 
 #include "../perf_event.h"
 
@@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
 {
 	u64 val_old, val_new, val_tmp;
 
+	/*
+	 * Disable the check for real HW, so we don't
+	 * mess up with potentionaly enabled regs.
+	 */
+	if (hypervisor_is_type(X86_HYPER_NATIVE))
+		return true;
+
 	/*
 	 * Read the current value, change it and read it back to see if it
 	 * matches, this is needed to detect certain hardware emulators
-- 
2.21.0

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 01:28:53PM +0200, Jiri Olsa wrote:
> hi,
> the HPE server can do POST tracing and have enabled LBR
> tracing during the boot, which makes check_msr fail falsly.
> 
> It looks like check_msr code was added only to check on guests
> MSR access, would it be then ok to disable check_msr for real
> hardware? (as in patch below)
> 
> We could also check if LBR tracing is enabled and make
> appropriate checks, but this change is simpler ;-)
> 
> ideas? thanks,
> jirka
> 
> 
> ---
>  arch/x86/events/intel/core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 71001f005bfe..1194ae7e1992 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -20,6 +20,7 @@
>  #include <asm/intel-family.h>
>  #include <asm/apic.h>
>  #include <asm/cpu_device_id.h>
> +#include <asm/hypervisor.h>
>  
>  #include "../perf_event.h"
>  
> @@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
>  {
>  	u64 val_old, val_new, val_tmp;
>  
> +	/*
> +	 * Disable the check for real HW, so we don't
> +	 * mess up with potentionaly enabled regs.
> +	 */
> +	if (hypervisor_is_type(X86_HYPER_NATIVE))
> +		return true;

Yeah, I think that works, or !boot_cpu_has(X86_FEATURE_HYPERVISOR).

>  	/*
>  	 * Read the current value, change it and read it back to see if it
>  	 * matches, this is needed to detect certain hardware emulators
> -- 
> 2.21.0
> 

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Liang, Kan]

On 6/14/2019 7:28 AM, Jiri Olsa wrote:
> hi,
> the HPE server can do POST tracing and have enabled LBR
> tracing during the boot, which makes check_msr fail falsly.
> 
> It looks like check_msr code was added only to check on guests
> MSR access, would it be then ok to disable check_msr for real
> hardware? (as in patch below)

Yes, the check_msr patch was to fix a bug report in guest.
I didn't get similar bug report for real hardware.
I think it should be OK to disable it for real hardware.

Thanks,
Kan

> 
> We could also check if LBR tracing is enabled and make
> appropriate checks, but this change is simpler ;-)
> 
> ideas? thanks,
> jirka
> 
> 
> ---
>   arch/x86/events/intel/core.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 71001f005bfe..1194ae7e1992 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -20,6 +20,7 @@
>   #include <asm/intel-family.h>
>   #include <asm/apic.h>
>   #include <asm/cpu_device_id.h>
> +#include <asm/hypervisor.h>
>   
>   #include "../perf_event.h"
>   
> @@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
>   {
>   	u64 val_old, val_new, val_tmp;
>   
> +	/*
> +	 * Disable the check for real HW, so we don't
> +	 * mess up with potentionaly enabled regs.
> +	 */
> +	if (hypervisor_is_type(X86_HYPER_NATIVE))
> +		return true;
> +
>   	/*
>   	 * Read the current value, change it and read it back to see if it
>   	 * matches, this is needed to detect certain hardware emulators
> 

[PATCH] perf/x86/intel: Disable check_msr for real hw

[Jiri Olsa]

On Fri, Jun 14, 2019 at 09:45:21AM -0400, Liang, Kan wrote:
> 
> 
> On 6/14/2019 7:28 AM, Jiri Olsa wrote:
> > hi,
> > the HPE server can do POST tracing and have enabled LBR
> > tracing during the boot, which makes check_msr fail falsly.
> > 
> > It looks like check_msr code was added only to check on guests
> > MSR access, would it be then ok to disable check_msr for real
> > hardware? (as in patch below)
> 
> Yes, the check_msr patch was to fix a bug report in guest.
> I didn't get similar bug report for real hardware.
> I think it should be OK to disable it for real hardware.
> 

thanks for confirmation, attaching the full patch

thanks,
jirka


---
Tom Vaden reported false failure of check_msr function, because
some servers can do POST tracing and enable LBR tracing during
the boot.

Kan confirmed that check_msr patch was to fix a bug report in
guest, so it's ok to disable it for real HW.

Cc: Kan Liang <kan.liang@intel.com>
Reported-by: Tom Vaden <tom.vaden@hpe.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/events/intel/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 71001f005bfe..1194ae7e1992 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -20,6 +20,7 @@
 #include <asm/intel-family.h>
 #include <asm/apic.h>
 #include <asm/cpu_device_id.h>
+#include <asm/hypervisor.h>
 
 #include "../perf_event.h"
 
@@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
 {
 	u64 val_old, val_new, val_tmp;
 
+	/*
+	 * Disable the check for real HW, so we don't
+	 * mess up with potentionaly enabled regs.
+	 */
+	if (hypervisor_is_type(X86_HYPER_NATIVE))
+		return true;
+
 	/*
 	 * Read the current value, change it and read it back to see if it
 	 * matches, this is needed to detect certain hardware emulators
-- 
2.21.0

Re: [PATCH] perf/x86/intel: Disable check_msr for real hw

[Vaden, Tom (HPE Server OS Architecture)]

On 6/16/19 10:13 AM, Jiri Olsa wrote:
> On Fri, Jun 14, 2019 at 09:45:21AM -0400, Liang, Kan wrote:
>>
>>
>> On 6/14/2019 7:28 AM, Jiri Olsa wrote:
>>> hi,
>>> the HPE server can do POST tracing and have enabled LBR
>>> tracing during the boot, which makes check_msr fail falsly.
>>>
>>> It looks like check_msr code was added only to check on guests
>>> MSR access, would it be then ok to disable check_msr for real
>>> hardware? (as in patch below)
>>
>> Yes, the check_msr patch was to fix a bug report in guest.
>> I didn't get similar bug report for real hardware.
>> I think it should be OK to disable it for real hardware.
>>
> 
> thanks for confirmation, attaching the full patch
> 
> thanks,
> jirka
> 
> 
> ---
> Tom Vaden reported false failure of check_msr function, because
> some servers can do POST tracing and enable LBR tracing during
> the boot.
> 
> Kan confirmed that check_msr patch was to fix a bug report in
> guest, so it's ok to disable it for real HW.
> 
> Cc: Kan Liang <kan.liang@intel.com>
> Reported-by: Tom Vaden <tom.vaden@hpe.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Thanks for handling the patch.

Acked-by: Tom Vaden <tom.vaden@hpe.com>

> ---
>   arch/x86/events/intel/core.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 71001f005bfe..1194ae7e1992 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -20,6 +20,7 @@
>   #include <asm/intel-family.h>
>   #include <asm/apic.h>
>   #include <asm/cpu_device_id.h>
> +#include <asm/hypervisor.h>
>   
>   #include "../perf_event.h"
>   
> @@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
>   {
>   	u64 val_old, val_new, val_tmp;
>   
> +	/*
> +	 * Disable the check for real HW, so we don't
> +	 * mess up with potentionaly enabled regs.
> +	 */
> +	if (hypervisor_is_type(X86_HYPER_NATIVE))
> +		return true;
> +
>   	/*
>   	 * Read the current value, change it and read it back to see if it
>   	 * matches, this is needed to detect certain hardware emulators
> 

[tip:perf/core] perf/x86/intel: Disable check_msr for real HW

[tip-bot for Jiri Olsa]

Commit-ID:  d0e1a507bdc761a14906f03399d933ea639a1756
Gitweb:     https://git.kernel.org/tip/d0e1a507bdc761a14906f03399d933ea639a1756
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Sun, 16 Jun 2019 16:13:13 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 17 Jun 2019 12:36:24 +0200

perf/x86/intel: Disable check_msr for real HW

Tom Vaden reported false failure of the check_msr() function, because
some servers can do POST tracing and enable LBR tracing during
bootup.

Kan confirmed that check_msr patch was to fix a bug report in
guest, so it's ok to disable it for real HW.

Reported-by: Tom Vaden <tom.vaden@hpe.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Tom Vaden <tom.vaden@hpe.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Liang Kan <kan.liang@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20190616141313.GD2500@krava
[ Readability edits. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 5e6ae481dee7..bda450ff51ee 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -20,6 +20,7 @@
 #include <asm/intel-family.h>
 #include <asm/apic.h>
 #include <asm/cpu_device_id.h>
+#include <asm/hypervisor.h>
 
 #include "../perf_event.h"
 
@@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
 {
 	u64 val_old, val_new, val_tmp;
 
+	/*
+	 * Disable the check for real HW, so we don't
+	 * mess with potentionaly enabled registers:
+	 */
+	if (hypervisor_is_type(X86_HYPER_NATIVE))
+		return true;
+
 	/*
 	 * Read the current value, change it and read it back to see if it
 	 * matches, this is needed to detect certain hardware emulators

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Andi Kleen]

On Fri, Jun 14, 2019 at 01:28:53PM +0200, Jiri Olsa wrote:
> hi,
> the HPE server can do POST tracing and have enabled LBR
> tracing during the boot, which makes check_msr fail falsly.
> 
> It looks like check_msr code was added only to check on guests
> MSR access, would it be then ok to disable check_msr for real
> hardware? (as in patch below)
> 
> We could also check if LBR tracing is enabled and make
> appropriate checks, but this change is simpler ;-)
> 
> ideas? thanks,
> jirka

Sorry for the late comment. I see this patch has been merged now.

Unfortunately I don't think it's a good idea. The problem 
is that the hypervisor flags are only set for a few hypervisors
that Linux knows about. But in practice there are many more
Hypervisors around that will not cause these flags to be set.
But these are still likely to miss MSRs.

The other hypervisors are relatively obscure, but eventually
someone will hit problems.

-Andi

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Jiri Olsa]

On Fri, Jun 21, 2019 at 10:48:25AM -0700, Andi Kleen wrote:
> On Fri, Jun 14, 2019 at 01:28:53PM +0200, Jiri Olsa wrote:
> > hi,
> > the HPE server can do POST tracing and have enabled LBR
> > tracing during the boot, which makes check_msr fail falsly.
> > 
> > It looks like check_msr code was added only to check on guests
> > MSR access, would it be then ok to disable check_msr for real
> > hardware? (as in patch below)
> > 
> > We could also check if LBR tracing is enabled and make
> > appropriate checks, but this change is simpler ;-)
> > 
> > ideas? thanks,
> > jirka
> 
> Sorry for the late comment. I see this patch has been merged now.
> 
> Unfortunately I don't think it's a good idea. The problem 
> is that the hypervisor flags are only set for a few hypervisors
> that Linux knows about. But in practice there are many more
> Hypervisors around that will not cause these flags to be set.
> But these are still likely to miss MSRs.
> 
> The other hypervisors are relatively obscure, but eventually
> someone will hit problems.

any idea if there's any other flag/way we could use to detect those?

adding few virtualization folks to the loop
and attaching the original patch

thanks,
jirka


---
Tom Vaden reported false failure of check_msr function, because
some servers can do POST tracing and enable LBR tracing during
the boot.

Kan confirmed that check_msr patch was to fix a bug report in
guest, so it's ok to disable it for real HW.

Cc: Kan Liang <kan.liang@intel.com>
Reported-by: Tom Vaden <tom.vaden@hpe.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/events/intel/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 71001f005bfe..1194ae7e1992 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -20,6 +20,7 @@
 #include <asm/intel-family.h>
 #include <asm/apic.h>
 #include <asm/cpu_device_id.h>
+#include <asm/hypervisor.h>
 
 #include "../perf_event.h"
 
@@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
 {
 	u64 val_old, val_new, val_tmp;
 
+	/*
+	 * Disable the check for real HW, so we don't
+	 * mess up with potentionaly enabled regs.
+	 */
+	if (hypervisor_is_type(X86_HYPER_NATIVE))
+		return true;
+
 	/*
 	 * Read the current value, change it and read it back to see if it
 	 * matches, this is needed to detect certain hardware emulators
-- 
2.21.0

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Vaden, Tom (HPE Server OS Architecture)]

On 6/23/19 6:40 PM, Jiri Olsa wrote:
> On Fri, Jun 21, 2019 at 10:48:25AM -0700, Andi Kleen wrote:
>> On Fri, Jun 14, 2019 at 01:28:53PM +0200, Jiri Olsa wrote:
>>> hi,
>>> the HPE server can do POST tracing and have enabled LBR
>>> tracing during the boot, which makes check_msr fail falsly.
>>>
>>> It looks like check_msr code was added only to check on guests
>>> MSR access, would it be then ok to disable check_msr for real
>>> hardware? (as in patch below)
>>>
>>> We could also check if LBR tracing is enabled and make
>>> appropriate checks, but this change is simpler ;-)
>>>
>>> ideas? thanks,
>>> jirka
>>
>> Sorry for the late comment. I see this patch has been merged now.
>>
>> Unfortunately I don't think it's a good idea. The problem
>> is that the hypervisor flags are only set for a few hypervisors
>> that Linux knows about. But in practice there are many more
>> Hypervisors around that will not cause these flags to be set.
>> But these are still likely to miss MSRs.
>>
>> The other hypervisors are relatively obscure, but eventually
>> someone will hit problems.
> 
> any idea if there's any other flag/way we could use to detect those?
> 
> adding few virtualization folks to the loop
> and attaching the original patch
> 
> thanks,
> jirka
> 
Would it be reasonable to change the sense of the original patch in 
commit 4550931 to only enable the check for the set of "certain hardware 
emulators" and leave the check otherwise disabled by default? I'm 
assuming that set is known (and small)?

> 
> ---
> Tom Vaden reported false failure of check_msr function, because
> some servers can do POST tracing and enable LBR tracing during
> the boot.
> 
> Kan confirmed that check_msr patch was to fix a bug report in
> guest, so it's ok to disable it for real HW.
> 
> Cc: Kan Liang <kan.liang@intel.com>
> Reported-by: Tom Vaden <tom.vaden@hpe.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   arch/x86/events/intel/core.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 71001f005bfe..1194ae7e1992 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -20,6 +20,7 @@
>   #include <asm/intel-family.h>
>   #include <asm/apic.h>
>   #include <asm/cpu_device_id.h>
> +#include <asm/hypervisor.h>
>   
>   #include "../perf_event.h"
>   
> @@ -4050,6 +4051,13 @@ static bool check_msr(unsigned long msr, u64 mask)
>   {
>   	u64 val_old, val_new, val_tmp;
>   
> +	/*
> +	 * Disable the check for real HW, so we don't
> +	 * mess up with potentionaly enabled regs.
> +	 */
> +	if (hypervisor_is_type(X86_HYPER_NATIVE))
> +		return true;
> +
>   	/*
>   	 * Read the current value, change it and read it back to see if it
>   	 * matches, this is needed to detect certain hardware emulators
> 

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Paolo Bonzini]

On 24/06/19 00:40, Jiri Olsa wrote:
> On Fri, Jun 21, 2019 at 10:48:25AM -0700, Andi Kleen wrote:
>> On Fri, Jun 14, 2019 at 01:28:53PM +0200, Jiri Olsa wrote:
>>> hi,
>>> the HPE server can do POST tracing and have enabled LBR
>>> tracing during the boot, which makes check_msr fail falsly.
>>>
>>> It looks like check_msr code was added only to check on guests
>>> MSR access, would it be then ok to disable check_msr for real
>>> hardware? (as in patch below)
>>>
>>> We could also check if LBR tracing is enabled and make
>>> appropriate checks, but this change is simpler ;-)
>>>
>>> ideas? thanks,
>>> jirka
>>
>> Sorry for the late comment. I see this patch has been merged now.
>>
>> Unfortunately I don't think it's a good idea. The problem 
>> is that the hypervisor flags are only set for a few hypervisors
>> that Linux knows about. But in practice there are many more
>> Hypervisors around that will not cause these flags to be set.
>> But these are still likely to miss MSRs.
>>
>> The other hypervisors are relatively obscure, but eventually
>> someone will hit problems.
> 
> any idea if there's any other flag/way we could use to detect those?

There's no silver bullet, the best is boot_cpu_has(X86_FEATURE_HYPERVISOR).

Paolo

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Andi Kleen]

> > The other hypervisors are relatively obscure, but eventually
> > someone will hit problems.
> 
> any idea if there's any other flag/way we could use to detect those?

I'm not aware of a generic way to detect any hypervisor unfortunately.

There are hypervisor reserved cpuid ranges, in theory you could
probe the existence of those. But there might be always some which
don't have extra CPUIDs.

> 
> adding few virtualization folks to the loop
> and attaching the original patch
> 
> thanks,
> jirka
> 
> 
> ---
> Tom Vaden reported false failure of check_msr function, because
> some servers can do POST tracing and enable LBR tracing during
> the boot.

Just to understand the original problem, the LBR registers
get locked somehow? It would be reasonable to not use LBRs
in this case. We just need to make sure everything 
else is still probed.

-Andi

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Jiri Olsa]

On Mon, Jun 24, 2019 at 09:46:17AM -0700, Andi Kleen wrote:
> > > The other hypervisors are relatively obscure, but eventually
> > > someone will hit problems.
> > 
> > any idea if there's any other flag/way we could use to detect those?
> 
> I'm not aware of a generic way to detect any hypervisor unfortunately.
> 
> There are hypervisor reserved cpuid ranges, in theory you could
> probe the existence of those. But there might be always some which
> don't have extra CPUIDs.
> 
> > 
> > adding few virtualization folks to the loop
> > and attaching the original patch
> > 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > Tom Vaden reported false failure of check_msr function, because
> > some servers can do POST tracing and enable LBR tracing during
> > the boot.
> 
> Just to understand the original problem, the LBR registers
> get locked somehow? It would be reasonable to not use LBRs
> in this case. We just need to make sure everything 
> else is still probed.

Tom, plz correctme if I'm wrongm but AFAIK because the LBR tracing is
enabled during the boot the lbr_from/lbr_to registers will fail the
check_msr 'val_new != val_tmp' check

if there's no good way to detect this, maybe we add boot option
to disable the check_msr check

jirka

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Andi Kleen]

> Tom, plz correctme if I'm wrongm but AFAIK because the LBR tracing is
> enabled during the boot the lbr_from/lbr_to registers will fail the
> check_msr 'val_new != val_tmp' check

Ok this should be handleable. It should be enough to check
the ctrl register, if that working likely we don't need
to check the data registers which might be changing.

-Andi

Re: [RFC] perf/x86/intel: Disable check_msr for real hw

[Jiri Olsa]

On Mon, Jun 24, 2019 at 11:38:06AM -0700, Andi Kleen wrote:
> > Tom, plz correctme if I'm wrongm but AFAIK because the LBR tracing is
> > enabled during the boot the lbr_from/lbr_to registers will fail the
> > check_msr 'val_new != val_tmp' check
> 
> Ok this should be handleable. It should be enough to check
> the ctrl register, if that working likely we don't need
> to check the data registers which might be changing.

if the control register check is enough, we should be ok then,
I'll send new version

thanks,
jirka