| Message ID | 1533648900-7933-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 51303113e32fd92d327b3c441c45e235642fa69c |
| Headers | show |
| Series | [v2] powerpc/tm: Print 64-bits MSR | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
On Tue, Aug 07, 2018 at 10:35:00AM -0300, Breno Leitao wrote: > On a kernel TM Bad thing program exception, the Machine State Register > (MSR) is not being properly displayed. The exception code dumps a 32-bits > value but MSR is a 64 bits register for all platforms that have HTM > enabled. > > This patch dumps the MSR value as a 64-bits value instead of 32 bits. In > order to do so, the 'reason' variable could not be used, since it trimmed > MSR to 32-bits (int). So maybe reason should be a long instead of an int? Segher
Le 07/08/2018 à 15:35, Breno Leitao a écrit : > On a kernel TM Bad thing program exception, the Machine State Register > (MSR) is not being properly displayed. The exception code dumps a 32-bits > value but MSR is a 64 bits register for all platforms that have HTM > enabled. > > This patch dumps the MSR value as a 64-bits value instead of 32 bits. In > order to do so, the 'reason' variable could not be used, since it trimmed > MSR to 32-bits (int). reason is not always regs->msr, see get_reason(), allthough in your case it is. I think it would be better to change 'reason' to 'unsigned long' instead of replacing it by regs->msr for the printk. Christophe > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > arch/powerpc/kernel/traps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index 0e17dcb48720..cd561fd89532 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -1402,7 +1402,7 @@ void program_check_exception(struct pt_regs *regs) > goto bail; > } else { > printk(KERN_EMERG "Unexpected TM Bad Thing exception " > - "at %lx (msr 0x%x)\n", regs->nip, reason); > + "at %lx (msr 0x%lx)\n", regs->nip, regs->msr); > die("Unrecoverable exception", regs, SIGABRT); > } > } >
Hi, On 08/07/2018 02:15 PM, Christophe LEROY wrote: > Le 07/08/2018 à 15:35, Breno Leitao a écrit : >> On a kernel TM Bad thing program exception, the Machine State Register >> (MSR) is not being properly displayed. The exception code dumps a 32-bits >> value but MSR is a 64 bits register for all platforms that have HTM >> enabled. >> >> This patch dumps the MSR value as a 64-bits value instead of 32 bits. In >> order to do so, the 'reason' variable could not be used, since it trimmed >> MSR to 32-bits (int). > > reason is not always regs->msr, see get_reason(), allthough in your case it is. > > I think it would be better to change 'reason' to 'unsigned long' instead of > replacing it by regs->msr for the printk. That was my initial approach, but this code seems to run on 32 bits system, and I do not want to change the whole 'reason' bit width without having a 32 bits to test, at least. Also, it is a bit weird doing something as: printk("....(msr 0x%lx)....", reason); I personally think that the follow code is much more readable: printk(".... (msr 0x%lx)...", regs->msr);
Breno Leitao <leitao@debian.org> a écrit : > Hi, > > On 08/07/2018 02:15 PM, Christophe LEROY wrote: >> Le 07/08/2018 à 15:35, Breno Leitao a écrit : >>> On a kernel TM Bad thing program exception, the Machine State Register >>> (MSR) is not being properly displayed. The exception code dumps a 32-bits >>> value but MSR is a 64 bits register for all platforms that have HTM >>> enabled. >>> >>> This patch dumps the MSR value as a 64-bits value instead of 32 bits. In >>> order to do so, the 'reason' variable could not be used, since it trimmed >>> MSR to 32-bits (int). >> >> reason is not always regs->msr, see get_reason(), allthough in your >> case it is. >> >> I think it would be better to change 'reason' to 'unsigned long' instead of >> replacing it by regs->msr for the printk. > > That was my initial approach, but this code seems to run on 32 bits system, > and I do not want to change the whole 'reason' bit width without having a 32 > bits to test, at least. But 'unsigned long' is still 32 bits on ppc32, so it makes no difference with 'unsigned int' And I will test it for you if needed Christophe > > Also, it is a bit weird doing something as: > > printk("....(msr 0x%lx)....", reason); > > I personally think that the follow code is much more readable: > > printk(".... (msr 0x%lx)...", regs->msr);
Hi Leroy, On 08/07/2018 03:57 PM, LEROY Christophe wrote: > Breno Leitao <leitao@debian.org> a écrit : >> On 08/07/2018 02:15 PM, Christophe LEROY wrote: >>> Le 07/08/2018 à 15:35, Breno Leitao a écrit : >>> I think it would be better to change 'reason' to 'unsigned long' instead of >>> replacing it by regs->msr for the printk. >> >> That was my initial approach, but this code seems to run on 32 bits system, >> and I do not want to change the whole 'reason' bit width without having a 32 >> bits to test, at least. > > But 'unsigned long' is still 32 bits on ppc32, so it makes no difference with > 'unsigned int' > And I will test it for you if needed Cool, I really appreciate it, and I would definitely need it once I have a more intrusive HTM patchset I am working on. Regarding this one, I think the change is so simple as-is that I would prefer to continue with the v2 patch, if you do not mind. Thank you!
On Tue, 2018-08-07 at 13:35:00 UTC, Breno Leitao wrote: > On a kernel TM Bad thing program exception, the Machine State Register > (MSR) is not being properly displayed. The exception code dumps a 32-bits > value but MSR is a 64 bits register for all platforms that have HTM > enabled. > > This patch dumps the MSR value as a 64-bits value instead of 32 bits. In > order to do so, the 'reason' variable could not be used, since it trimmed > MSR to 32-bits (int). > > Signed-off-by: Breno Leitao <leitao@debian.org> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/51303113e32fd92d327b3c441c45e2 cheers
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index 0e17dcb48720..cd561fd89532 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -1402,7 +1402,7 @@ void program_check_exception(struct pt_regs *regs) goto bail; } else { printk(KERN_EMERG "Unexpected TM Bad Thing exception " - "at %lx (msr 0x%x)\n", regs->nip, reason); + "at %lx (msr 0x%lx)\n", regs->nip, regs->msr); die("Unrecoverable exception", regs, SIGABRT); } }
On a kernel TM Bad thing program exception, the Machine State Register (MSR) is not being properly displayed. The exception code dumps a 32-bits value but MSR is a 64 bits register for all platforms that have HTM enabled. This patch dumps the MSR value as a 64-bits value instead of 32 bits. In order to do so, the 'reason' variable could not be used, since it trimmed MSR to 32-bits (int). Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/traps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)