[v2] powerpc/tm: Print 64-bits MSR

Message ID 1533648900-7933-1-git-send-email-leitao@debian.org
State New
Headers show
Series
  • [v2] powerpc/tm: Print 64-bits MSR
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-ppc32 success Test build-ppc32 on branch next
snowpatch_ozlabs/build-ppc64e success Test build-ppc64e on branch next
snowpatch_ozlabs/build-ppc64be success Test build-ppc64be on branch next
snowpatch_ozlabs/build-ppc64le success Test build-ppc64le on branch next
snowpatch_ozlabs/checkpatch warning Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Breno Leitao Aug. 7, 2018, 1:35 p.m.
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(-)

Comments

Segher Boessenkool Aug. 7, 2018, 3:50 p.m. | #1
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
Christophe LEROY Aug. 7, 2018, 5:15 p.m. | #2
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);
>   		}
>   	}
>
Breno Leitao Aug. 7, 2018, 6:41 p.m. | #3
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);
Christophe LEROY Aug. 7, 2018, 6:57 p.m. | #4
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);
Breno Leitao Aug. 8, 2018, 3:50 p.m. | #5
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!

Patch

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);
 		}
 	}