(no subject)

[Andres Freund]

Hi,

After 3.14 I noticed that perf started to only support software
events.

The reason - found after a some staring deathmatches because the
patches look innocent - is a typo in __flip_bit() introduced by
22085a66c2fab6cf9b9393c056a3600a6b4735de which only takes effect after
c0a639ad0bc6b178b46996bd1f821a04643e2bde. Due to the typo no msr
writes via either msr_clear/set_bit actually write back the new value.
That then causes cpuid limits not to be lifted causing 'my' perf and
very likely some other problems.

There's also a typo in the former commit which makes it impossible to
actually use the MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro correctly.

Regards,

Andres Freund

[PATCH 1/2] x86: Fix typo preventing msr_set/clear_bit from having an effect

[Andres Freund]

Due to a typo the msr accessor function introduced in
22085a66c2fab6cf9b9393c056a3600a6b4735de didn't have any lasting
effects because they accidentally wrote the old value back.

After c0a639ad0bc6b178b46996bd1f821a04643e2bde this at the very least
this causes cpuid limits not to be lifted on some cpus leading to
missing capabilities for those.

Signed-off-by: Andres Freund <andres@anarazel.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/lib/msr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index db9db44..4362373 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -76,7 +76,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
 	if (m1.q == m.q)
 		return 0;
 
-	err = msr_write(msr, &m);
+	err = msr_write(msr, &m1);
 	if (err)
 		return err;
 
-- 
2.0.0.rc2.4.g1dc51c6.dirty

[PATCH 2/2] x86: Fix typo in MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro

[Andres Freund]

The spuriously added semicolon didn't have any effect because the
macro isn't currently in use.

c0a639ad0bc6b178b46996bd1f821a04643e2bde

Signed-off-by: Andres Freund <andres@anarazel.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/uapi/asm/msr-index.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index c827ace..fcf2b3a 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -384,7 +384,7 @@
 #define MSR_IA32_MISC_ENABLE_MWAIT_BIT			18
 #define MSR_IA32_MISC_ENABLE_MWAIT			(1ULL << MSR_IA32_MISC_ENABLE_MWAIT_BIT)
 #define MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT		22
-#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT);
+#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT)
 #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE_BIT		23
 #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE		(1ULL << MSR_IA32_MISC_ENABLE_XTPR_DISABLE_BIT)
 #define MSR_IA32_MISC_ENABLE_XD_DISABLE_BIT		34
-- 
2.0.0.rc2.4.g1dc51c6.dirty

Re: [PATCH 1/2] x86: Fix typo preventing msr_set/clear_bit from having an effect

[Borislav Petkov]

On Fri, May 09, 2014 at 03:29:16AM +0200, Andres Freund wrote:
> Due to a typo the msr accessor function introduced in
> 22085a66c2fab6cf9b9393c056a3600a6b4735de didn't have any lasting
> effects because they accidentally wrote the old value back.
> 
> After c0a639ad0bc6b178b46996bd1f821a04643e2bde this at the very least
> this causes cpuid limits not to be lifted on some cpus leading to
> missing capabilities for those.
> 
> Signed-off-by: Andres Freund <andres@anarazel.de>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/lib/msr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
> index db9db44..4362373 100644
> --- a/arch/x86/lib/msr.c
> +++ b/arch/x86/lib/msr.c
> @@ -76,7 +76,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
>  	if (m1.q == m.q)
>  		return 0;
>  
> -	err = msr_write(msr, &m);
> +	err = msr_write(msr, &m1);
>  	if (err)
>  		return err;

Good catch, thanks, and sorry about the screwup! :-[

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86: Fix typo in MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro

[Borislav Petkov]

On Fri, May 09, 2014 at 03:29:17AM +0200, Andres Freund wrote:
> The spuriously added semicolon didn't have any effect because the
> macro isn't currently in use.
> 
> c0a639ad0bc6b178b46996bd1f821a04643e2bde
> 
> Signed-off-by: Andres Freund <andres@anarazel.de>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/uapi/asm/msr-index.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> index c827ace..fcf2b3a 100644
> --- a/arch/x86/include/uapi/asm/msr-index.h
> +++ b/arch/x86/include/uapi/asm/msr-index.h
> @@ -384,7 +384,7 @@
>  #define MSR_IA32_MISC_ENABLE_MWAIT_BIT			18
>  #define MSR_IA32_MISC_ENABLE_MWAIT			(1ULL << MSR_IA32_MISC_ENABLE_MWAIT_BIT)
>  #define MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT		22
> -#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT);
> +#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT)
>  #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE_BIT		23
>  #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE		(1ULL << MSR_IA32_MISC_ENABLE_XTPR_DISABLE_BIT)
>  #define MSR_IA32_MISC_ENABLE_XD_DISABLE_BIT		34

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86: Fix typo in MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro

[Borislav Petkov]

On Fri, May 09, 2014 at 09:57:30AM +0200, Borislav Petkov wrote:
> > diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> > index c827ace..fcf2b3a 100644
> > --- a/arch/x86/include/uapi/asm/msr-index.h
> > +++ b/arch/x86/include/uapi/asm/msr-index.h
> > @@ -384,7 +384,7 @@
> >  #define MSR_IA32_MISC_ENABLE_MWAIT_BIT			18
> >  #define MSR_IA32_MISC_ENABLE_MWAIT			(1ULL << MSR_IA32_MISC_ENABLE_MWAIT_BIT)
> >  #define MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT		22
> > -#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT);
> > +#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT)

Btw, we should probably be catching typos like that with checkpatch.
Here's an initial version for single-line macros:

---
From: Borislav Petkov <bp@suse.de>
Subject: [PATCH] checkpatch: Catch trailing semicolons in macro definitions

This currently checks only single-line macro definitions. I.e.,

$ git show c0a639ad0bc6b178b46996bd1f821a04643e2bde | ./scripts/checkpatch.pl -

...
WARNING: Trailing semicolon at macro definition
+#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID       (1ULL << MSR_BIT_LIMIT_CPUID);

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 scripts/checkpatch.pl | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb2160489d..04929c20c9b5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3313,6 +3313,11 @@ sub process {
 			}
 		}
 
+		if ($line =~ /^\+\s*#\s*define\s.*;\s*$/) {
+			WARN("TRAILING_SEMICOLON",
+			     "Trailing semicolon at macro definition\n" . $herecurr);
+		}
+
 # check for multiple assignments
 		if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
 			CHK("MULTIPLE_ASSIGNMENTS",
-- 
1.9.0

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86: Fix typo preventing msr_set/clear_bit from having an effect

[Andres Freund]

Hi,

On 2014-05-09 09:54:52 +0200, Borislav Petkov wrote:
> On Fri, May 09, 2014 at 03:29:16AM +0200, Andres Freund wrote:
> > Due to a typo the msr accessor function introduced in
> > 22085a66c2fab6cf9b9393c056a3600a6b4735de didn't have any lasting
> > effects because they accidentally wrote the old value back.
> > 
> > After c0a639ad0bc6b178b46996bd1f821a04643e2bde this at the very least
> > this causes cpuid limits not to be lifted on some cpus leading to
> > missing capabilities for those.
> > 
> > Signed-off-by: Andres Freund <andres@anarazel.de>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: H. Peter Anvin <hpa@linux.intel.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/x86/lib/msr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> Good catch, thanks, and sorry about the screwup! :-[
> 
> Acked-by: Borislav Petkov <bp@suse.de>

It got caught before the release, so all is well... Actually rather
surprised it only causes relatively minor problems; i.e. still boots.

Did I CC the correct people for this to get picked up for 3.15 or to I
need to do anything else?

Greetings,

Andres Freund

Re: [PATCH 1/2] x86: Fix typo preventing msr_set/clear_bit from having an effect

[Borislav Petkov]

On Fri, May 09, 2014 at 12:39:42PM +0200, Andres Freund wrote:
> It got caught before the release, so all is well...

Yeah.

> Actually rather surprised it only causes relatively minor problems;
> i.e. still boots.

Well, I did switch only a couple of sites in intel.c, maybe you got
lucky... (if that can be called getting lucky at all :-P).

> Did I CC the correct people for this to get picked up for 3.15 or to I
> need to do anything else?

Yes, you did, it looks good.

Thanks!

-- 
Regards/Gruss,
    Boris.

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

[tip:x86/urgent] x86: Fix typo preventing msr_set/ clear_bit from having an effect

[tip-bot for Andres Freund]

Commit-ID:  722a0d22d028bd74061cf582de1764884e73674f
Gitweb:     http://git.kernel.org/tip/722a0d22d028bd74061cf582de1764884e73674f
Author:     Andres Freund <andres@anarazel.de>
AuthorDate: Fri, 9 May 2014 03:29:16 +0200
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Fri, 9 May 2014 08:42:32 -0700

x86: Fix typo preventing msr_set/clear_bit from having an effect

Due to a typo the msr accessor function introduced in
22085a66c2fab6cf9b9393c056a3600a6b4735de didn't have any lasting
effects because they accidentally wrote the old value back.

After c0a639ad0bc6b178b46996bd1f821a04643e2bde this at the very least
this causes cpuid limits not to be lifted on some cpus leading to
missing capabilities for those.

Signed-off-by: Andres Freund <andres@anarazel.de>
Link: http://lkml.kernel.org/r/1399598957-7011-2-git-send-email-andres@anarazel.de
Cc: Borislav Petkov <bp@suse.de>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/lib/msr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index db9db44..4362373 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -76,7 +76,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
 	if (m1.q == m.q)
 		return 0;
 
-	err = msr_write(msr, &m);
+	err = msr_write(msr, &m1);
 	if (err)
 		return err;
 

[tip:x86/urgent] x86: Fix typo in MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro

[tip-bot for Andres Freund]

Commit-ID:  c45f77364ba060395b7eff1bf45e6c537f913380
Gitweb:     http://git.kernel.org/tip/c45f77364ba060395b7eff1bf45e6c537f913380
Author:     Andres Freund <andres@anarazel.de>
AuthorDate: Fri, 9 May 2014 03:29:17 +0200
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Fri, 9 May 2014 08:42:47 -0700

x86: Fix typo in MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro

The spuriously added semicolon didn't have any effect because the
macro isn't currently in use.

c0a639ad0bc6b178b46996bd1f821a04643e2bde

Signed-off-by: Andres Freund <andres@anarazel.de>
Link: http://lkml.kernel.org/r/1399598957-7011-3-git-send-email-andres@anarazel.de
Cc: Borislav Petkov <bp@suse.de>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/uapi/asm/msr-index.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index c827ace..fcf2b3a 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -384,7 +384,7 @@
 #define MSR_IA32_MISC_ENABLE_MWAIT_BIT			18
 #define MSR_IA32_MISC_ENABLE_MWAIT			(1ULL << MSR_IA32_MISC_ENABLE_MWAIT_BIT)
 #define MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT		22
-#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT);
+#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT)
 #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE_BIT		23
 #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE		(1ULL << MSR_IA32_MISC_ENABLE_XTPR_DISABLE_BIT)
 #define MSR_IA32_MISC_ENABLE_XD_DISABLE_BIT		34

Re: [PATCH 2/2] x86: Fix typo in MSR_IA32_MISC_ENABLE_LIMIT_CPUID macro

[Joe Perches]

On Fri, 2014-05-09 at 12:33 +0200, Borislav Petkov wrote:
> On Fri, May 09, 2014 at 09:57:30AM +0200, Borislav Petkov wrote:
> > > diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
[]
> > > -#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT);
> > > +#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID		(1ULL << MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT)
> 
> Btw, we should probably be catching typos like that with checkpatch.
> Here's an initial version for single-line macros:
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3313,6 +3313,11 @@ sub process {
>  			}
>  		}
>  
> +		if ($line =~ /^\+\s*#\s*define\s.*;\s*$/) {
> +			WARN("TRAILING_SEMICOLON",
> +			     "Trailing semicolon at macro definition\n" . $herecurr);
> +		}
> +

Seems sensible.

I suggest moving the test next to the other macro
definition tests for things like unnecessary semicolon
around "do {... while (0)" around line 3800.

Extending the test to find multiline forms like

#define	foo	\
	bar;

would also be trivially done there.

[PATCH] checkpatch: Warn on #defines ending in semicolon

[Joe Perches]

Using a #define ending in a semicolon is poor style
and can lead to unexpected code paths being executed.

Warn on uses of these #define types:

	#define foo[(...)] bar;
	#define foo[(...)]	\
		bar;

Original-patch-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb216..3b5651a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3782,6 +3782,17 @@ sub process {
 					WARN("DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON",
 					     "do {} while (0) macros should not be semicolon terminated\n" . "$herectx");
 				}
+			} elsif ($dstat =~ /^\+\s*#\s*define\s+$Ident.*;\s*$/) {
+				$ctx =~ s/\n*$//;
+				my $cnt = statement_rawlines($ctx);
+				my $herectx = $here . "\n";
+
+				for (my $n = 0; $n < $cnt; $n++) {
+					$herectx .= raw_line($linenr, $n) . "\n";
+				}
+
+				WARN("TRAILING_SEMICOLON",
+				     "macros should not use a trailing semicolon\n" . "$herectx");
 			}
 		}
 

Re: [PATCH] checkpatch: Warn on #defines ending in semicolon

[Thomas Gleixner]

On Fri, 9 May 2014, Joe Perches wrote:

> Using a #define ending in a semicolon is poor style

It's not poor style it's simply a bug. Just because it 

> can lead to unexpected code paths being executed.

Thanks,

	tglx

Re: [PATCH] checkpatch: Warn on #defines ending in semicolon

[Joe Perches]

On Fri, 2014-05-09 at 23:47 +0200, Thomas Gleixner wrote:
> On Fri, 9 May 2014, Joe Perches wrote:
> 
> > Using a #define ending in a semicolon is poor style
> 
> It's not poor style it's simply a bug. Just because it 
> 
> > can lead to unexpected code paths being executed.

I think it's still a (bad) style issue.

I remember a discussion with a developer that
wanted all macros to end in a semicolon so he
could write COBOL like C.

ick.

cheers, Joe