Skip to contentSkip to conversation

powerpc: Decode correct MSR bits in oops output

PATCH In review Anton Blanchard 5 participants 9 messages Nov 25, 2011 – Nov 28, 2011 PowerPC

lore
Share & tools
lore

Anton Blanchard

author
#0 lore raw

PATCH 1/1 powerpc: Decode correct MSR bits in oops output

On a 64bit book3s machine I have an oops from a system reset that claims the book3e CE bit was set:

MSR: 8000000000021032 <ME,CE,IR,DR> CR: 24004082 XER: 00000010

On a book3s machine system reset sets IBM bit 46 and 47 depending on the power saving mode. Separate the definitions by type and for completeness add the rest of the bits in.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/process.c +19 -3
Index: linux-build/arch/powerpc/kernel/process.c
===================================================================
@@ -584,16 +584,32 @@ static struct regbit {
 	unsigned long bit;
 	const char *name;
 } msr_bits[] = {
+#if defined(CONFIG_PPC64) && !defined(CONFIG_BOOKE)
+	{MSR_SF,	"SF"},
+	{MSR_HV,	"HV"},
+#endif
+	{MSR_VEC,	"VEC"},
+	{MSR_VSX,	"VSX"},
+#ifdef CONFIG_BOOKE
+	{MSR_CE,	"CE"},
+#endif
 	{MSR_EE,	"EE"},
 	{MSR_PR,	"PR"},
 	{MSR_FP,	"FP"},
-	{MSR_VEC,	"VEC"},
-	{MSR_VSX,	"VSX"},
 	{MSR_ME,	"ME"},
-	{MSR_CE,	"CE"},
+#ifdef CONFIG_BOOKE
 	{MSR_DE,	"DE"},
+#else
+	{MSR_SE,	"SE"},
+	{MSR_BE,	"BE"},
+#endif
 	{MSR_IR,	"IR"},
 	{MSR_DR,	"DR"},
+	{MSR_PMM,	"PMM"},
+#ifndef CONFIG_BOOKE
+	{MSR_RI,	"RI"},
+	{MSR_LE,	"LE"},
+#endif
 	{0,		NULL}
 };
#1 lore raw
Quoted 10 lines · Anton Blanchard

=20
On a 64bit book3s machine I have an oops from a system reset that claims the book3e CE bit was set:
=20
MSR: 8000000000021032 <ME,CE,IR,DR> CR: 24004082 XER: 00000010 =20
On a book3s machine system reset sets IBM bit 46 and 47 depending on the power saving mode. Separate the definitions by type and for completeness add the rest of the bits in.
=20

Quoted 5 lines
Signed-off-by Anton Blanchard <[email protected]>

---
=20
Index: linux-build/arch/powerpc/kernel/process.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Quoted 1 line

--- linux-build.orig/arch/powerpc/kernel/process.c 2011-11-25 =

13:22:24.294919094 +1100

Quoted 1 line

+++ linux-build/arch/powerpc/kernel/process.c 2011-11-25 =

13:36:23.213834524 +1100

Quoted 21 lines
arch/powerpc/kernel/process.c +10 -3
@@ -584,16 +584,32 @@ static struct regbit {
 	unsigned long bit;
 	const char *name;
 } msr_bits[] =3D {
+#if defined(CONFIG_PPC64) && !defined(CONFIG_BOOKE)
+	{MSR_SF,	"SF"},
+	{MSR_HV,	"HV"},
+#endif
+	{MSR_VEC,	"VEC"},
+	{MSR_VSX,	"VSX"},
+#ifdef CONFIG_BOOKE
+	{MSR_CE,	"CE"},
+#endif
 	{MSR_EE,	"EE"},
 	{MSR_PR,	"PR"},
 	{MSR_FP,	"FP"},
-	{MSR_VEC,	"VEC"},
-	{MSR_VSX,	"VSX"},
 	{MSR_ME,	"ME"},
-	{MSR_CE,	"CE"},
+#ifdef CONFIG_BOOKE

arch/powerpc/kernel/process.c static struct regbit { · line 610

Quoted 10 lines
arch/powerpc/kernel/process.c +7 -0
 	{MSR_DE,	"DE"},
+#else
+	{MSR_SE,	"SE"},
+	{MSR_BE,	"BE"},
+#endif
 	{MSR_IR,	"IR"},
 	{MSR_DR,	"DR"},
+	{MSR_PMM,	"PMM"},
+#ifndef CONFIG_BOOKE
+	{MSR_RI,	"RI"},

We have 'RI' on some BOOKE so lets allow for it to be decoded

arch/powerpc/kernel/process.c static struct regbit { · line 614

Quoted 4 lines
arch/powerpc/kernel/process.c +2 -0
+	{MSR_LE,	"LE"},
+#endif
 	{0,		NULL}
 };

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

- k=

#2 lore raw

On Mon, Nov 28, 2011 at 11:04 AM, Kumar Gala <[email protected]> wr= ote:

Quoted 13 lines · Anton Blanchard
Quoted 11 lines

On a 64bit book3s machine I have an oops from a system reset that claims the book3e CE bit was set:

MSR: 8000000000021032 <ME,CE,IR,DR> =A0CR: 24004082 =A0XER: 00000010

On a book3s machine system reset sets IBM bit 46 and 47 depending on the power saving mode. Separate the definitions by type and for completeness add the rest of the bits in.

Quoted 5 lines
Quoted 5 lines
Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux-build/arch/powerpc/kernel/process.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Quoted 1 line
Quoted 1 line

--- linux-build.orig/arch/powerpc/kernel/process.c =A0 =A02011-11-25 13:=

22:24.294919094 +1100

Quoted 1 line
Quoted 1 line

+++ linux-build/arch/powerpc/kernel/process.c 2011-11-25 13:36:23.213834=

524 +1100

Quoted 34 lines
Quoted 21 lines
(quoted diff) +10 -3
@@ -584,16 +584,32 @@ static struct regbit {
 =A0 =A0 =A0 unsigned long bit;
 =A0 =A0 =A0 const char *name;
 } msr_bits[] =3D {
+#if defined(CONFIG_PPC64) && !defined(CONFIG_BOOKE)
+ =A0 =A0 {MSR_SF, =A0 =A0 =A0 =A0"SF"},
+ =A0 =A0 {MSR_HV, =A0 =A0 =A0 =A0"HV"},
+#endif
+ =A0 =A0 {MSR_VEC, =A0 =A0 =A0 "VEC"},
+ =A0 =A0 {MSR_VSX, =A0 =A0 =A0 "VSX"},
+#ifdef CONFIG_BOOKE
+ =A0 =A0 {MSR_CE, =A0 =A0 =A0 =A0"CE"},
+#endif
 =A0 =A0 =A0 {MSR_EE, =A0 =A0 =A0 =A0"EE"},
 =A0 =A0 =A0 {MSR_PR, =A0 =A0 =A0 =A0"PR"},
 =A0 =A0 =A0 {MSR_FP, =A0 =A0 =A0 =A0"FP"},
- =A0 =A0 {MSR_VEC, =A0 =A0 =A0 "VEC"},
- =A0 =A0 {MSR_VSX, =A0 =A0 =A0 "VSX"},
 =A0 =A0 =A0 {MSR_ME, =A0 =A0 =A0 =A0"ME"},
- =A0 =A0 {MSR_CE, =A0 =A0 =A0 =A0"CE"},
+#ifdef CONFIG_BOOKE

Review static struct regbit { · line 610

Quoted 10 lines
(quoted diff) +7 -0
 =A0 =A0 =A0 {MSR_DE, =A0 =A0 =A0 =A0"DE"},
+#else
+ =A0 =A0 {MSR_SE, =A0 =A0 =A0 =A0"SE"},
+ =A0 =A0 {MSR_BE, =A0 =A0 =A0 =A0"BE"},
+#endif
 =A0 =A0 =A0 {MSR_IR, =A0 =A0 =A0 =A0"IR"},
 =A0 =A0 =A0 {MSR_DR, =A0 =A0 =A0 =A0"DR"},
+ =A0 =A0 {MSR_PMM, =A0 =A0 =A0 "PMM"},
+#ifndef CONFIG_BOOKE
+ =A0 =A0 {MSR_RI, =A0 =A0 =A0 =A0"RI"},

We have 'RI' on some BOOKE so lets allow for it to be decoded

Quoted 8 lines

arch/powerpc/kernel/process.c static struct regbit { · line 614

Quoted 4 lines
arch/powerpc/kernel/process.c +2 -0
+ =A0 =A0 {MSR_LE, =A0 =A0 =A0 =A0"LE"},
+#endif
 =A0 =A0 =A0 {0, =A0 =A0 =A0 =A0 =A0 =A0 NULL}
 };

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

Those don't exist on 4xx, so CONFIG_BOOKE doesn't seem appropriate.

josh

#3 lore raw
Quoted 7 lines · Josh Boyer
Quoted 4 lines · Kumar Gala

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

Those don't exist on 4xx, so CONFIG_BOOKE doesn't seem appropriate.

They're defined by book3e of ISA 2.06.

Not all bits are going to exist on all CPUs -- does 4xx use these bits to mean something different?

-Scott

#4 lore raw
Quoted 10 lines · Scott Wood
Quoted 7 lines · Josh Boyer
Quoted 4 lines · Kumar Gala

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

Those don't exist on 4xx, so CONFIG_BOOKE doesn't seem appropriate.

They're defined by book3e of ISA 2.06.

4xx existed before that, sadly (as did CONFIG_BOOKE).

Quoted 2 lines

Not all bits are going to exist on all CPUs -- does 4xx use these bits to mean something different?

No, marked as reserved. However, given the patch shows up in human readable output, I don't think we want reserved bits being decoded and showing up inadvertently.

Could introduce BOOK3E_32 to cover cases like this.

josh

#5 lore raw
Quoted 7 lines · Josh Boyer
Quoted 6 lines · Scott Wood
Quoted 5 lines · Josh Boyer
Quoted 4 lines · Kumar Gala

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

PMM is not just BookE, and is already present in the patch.

RI is present on e500mc (despite being reserved in book3e), so might not want to stick that inside #ifndef CONFIG_BOOKE.

Quoted 6 lines
Quoted 2 lines

Not all bits are going to exist on all CPUs -- does 4xx use these bits to mean something different?

No, marked as reserved. However, given the patch shows up in human readable output, I don't think we want reserved bits being decoded and showing up inadvertently.

Do the bits ever actually get set on 4xx (documented or otherwise), or is this a theoretical concern?

If 4xx must be excluded, use something like:
#if defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)

Do we also need to patch out things like MSR_VEC at runtime, in case it randomly shows up on a pre-Altivec CPU?

Quoted 1 line

Could introduce BOOK3E_32 to cover cases like this.

Why _32? These bits apply to 64-bit as well. MSR_CM is only for 64-bit.

UCLE and PMM are present on pre-2.06 e500 cores as well.

-Scott

#6 lore raw

On Mon, Nov 28, 2011 at 3:04 PM, Scott Wood <[email protected]> wrote= :

Quoted 2 lines · Josh Boyer
Quoted 1 line

On Mon, Nov 28, 2011 at 2:30 PM, Scott Wood <[email protected]> wr=

ote:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines · Josh Boyer
Quoted 1 line

On Mon, Nov 28, 2011 at 11:04 AM, Kumar Gala <[email protected]=

Quoted 17 lines · g> wrote:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

PMM is not just BookE, and is already present in the patch.

RI is present on e500mc (despite being reserved in book3e), so might not want to stick that inside #ifndef CONFIG_BOOKE.

Quoted 6 lines
Quoted 2 lines

Not all bits are going to exist on all CPUs -- does 4xx use these bits to mean something different?

No, marked as reserved. =A0However, given the patch shows up in human readable output, I don't think we want reserved bits being decoded and showing up inadvertently.

Quoted 5 lines

Do the bits ever actually get set on 4xx (documented or otherwise), or is this a theoretical concern?

If 4xx must be excluded, use something like:
#if defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)

I was going for something a bit simpler. Basically, CONFIG_BOOKE should be treated as a legacy Kconfig variable that has nothing to do with ISA 2.06 because it existed before that and is used by things that aren't compliant with 2.06 (both FSL and 4xx). If we use it for things that show up on these non-compliant CPUs, we'll have to continually add the !defined(WHATEVER) and that just gets ugly over time.

Quoted 6 lines

Do we also need to patch out things like MSR_VEC at runtime, in case it randomly shows up on a pre-Altivec CPU?

Quoted 1 line

Could introduce BOOK3E_32 to cover cases like this.

Why _32? =A0These bits apply to 64-bit as well. =A0MSR_CM is only for 64-=

bit.

Because CONFIG_BOOK3E depeonds on PPC_BOOK3E_64. So either that dependency needs to go so it's selectable elsewhere, or a similarly intended PPC_BOOK3E_32 needs to get created. Or something.

Quoted 1 line

UCLE and PMM are present on pre-2.06 e500 cores as well.

Sigh. Maybe there is no way to get un-ugly.

josh

#7 lore raw
Quoted 9 lines · Josh Boyer
Quoted 4 lines · Scott Wood
Quoted 1 line · Josh Boyer

Could introduce BOOK3E_32 to cover cases like this.

Why _32? These bits apply to 64-bit as well. MSR_CM is only for 64-bit.

Because CONFIG_BOOK3E depeonds on PPC_BOOK3E_64. So either that dependency needs to go so it's selectable elsewhere, or a similarly intended PPC_BOOK3E_32 needs to get created. Or something.

I think that dependency should go, in any case. We already have PPC_BOOK3E_64 for places that need to depend on that, and we don't want to end up having this all over the place:

#if defined(CONFIG_PPC_BOOK3E_32) || defined(CONFIG_PPC_BOOK3E_64)

Quoted 3 lines
Quoted 1 line

UCLE and PMM are present on pre-2.06 e500 cores as well.

Sigh. Maybe there is no way to get un-ugly.

If we drop the 64-bit dependency, we could do this for UCLE if it really needs to be omitted from a 4xx kernel:

#if defined(CONFIG_PPC_BOOK3E) || defined(CONFIG_E500)

PMM is not just a BookE thing, so if 4xx really needs to exclude it, #ifndef CONFIG_4xx is the way to go.

I wouldn't bother unless 4xx is known to set these bits, though.

For GS and CM CONFIG_PPC_BOOK3E is OK, once 32-bit e500mc/e5500 kernels start selecting it.

-Scott

#8 lore raw
In reply to Kumar Gala
Quoted 5 lines · Kumar Gala

arch/powerpc/kernel/process.c static struct regbit { · line 610

Quoted 2 lines
arch/powerpc/kernel/process.c +2 -0
+#ifndef CONFIG_BOOKE
+	{MSR_RI,	"RI"},

We have 'RI' on some BOOKE so lets allow for it to be decoded

Quoted 8 lines
Quoted 4 lines
arch/powerpc/kernel/process.c +2 -0
+	{MSR_LE,	"LE"},
+#endif

	{0,		NULL}
};

Since you're fixing this can you add the following for CONFIG_BOOKE:

MSR_GS, MSR_UCLE, MSR_PMM, MSR_CM

Please send a followup patch.

Cheers,
Ben.