powerpc: fix xmon disassembler for little-endian

Submitted by Philippe Bergheaud on Dec. 2, 2013, 9:10 a.m.

Details

Message ID 1385975412-25869-1-git-send-email-felix@linux.vnet.ibm.com
State Accepted, archived
Commit 72eceef67abbe596a4e93ee79e08d9e6c35430ae
Delegated to: Benjamin Herrenschmidt
Headers show

Commit Message

Philippe Bergheaud Dec. 2, 2013, 9:10 a.m.
This patch fixes the disassembler of the powerpc kernel debugger xmon,
for little-endian.

Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
 arch/powerpc/xmon/xmon.c |    4 ++++
 1 file changed, 4 insertions(+)

Comments

Tom Musta Dec. 2, 2013, 2:05 p.m.
On 12/2/2013 3:10 AM, Philippe Bergheaud wrote:
> This patch fixes the disassembler of the powerpc kernel debugger xmon,
> for little-endian.
> 
> Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
> ---
>  arch/powerpc/xmon/xmon.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index af9d346..6c27804 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -171,7 +171,11 @@ extern void xmon_leave(void);
>  #define REG		"%.8lx"
>  #endif
>  
> +#ifdef __LITTLE_ENDIAN__
> +#define GETWORD(v)	(((v)[3] << 24) + ((v)[2] << 16) + ((v)[1] << 8) + (v)[0])
> +#else
>  #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
> +#endif
>  
>  #define isxdigit(c)	(('0' <= (c) && (c) <= '9') \
>  			 || ('a' <= (c) && (c) <= 'f') \
> 

Philippe:  Wouldn't it be better to just do a 32-bit load and let the endianness be worked out
by the hardware?  i.e.

#define GETWORD(v) (*(u32 *)v)


Tom
Philippe Bergheaud Dec. 4, 2013, 1:45 p.m.
Tom Musta wrote:
> On 12/2/2013 3:10 AM, Philippe Bergheaud wrote:
> 
>>This patch fixes the disassembler of the powerpc kernel debugger xmon,
>>for little-endian.
>>
>>Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
>>---
>> arch/powerpc/xmon/xmon.c |    4 ++++
>> 1 file changed, 4 insertions(+)
>>
>>diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>>index af9d346..6c27804 100644
>>--- a/arch/powerpc/xmon/xmon.c
>>+++ b/arch/powerpc/xmon/xmon.c
>>@@ -171,7 +171,11 @@ extern void xmon_leave(void);
>> #define REG		"%.8lx"
>> #endif
>> 
>>+#ifdef __LITTLE_ENDIAN__
>>+#define GETWORD(v)	(((v)[3] << 24) + ((v)[2] << 16) + ((v)[1] << 8) + (v)[0])
>>+#else
>> #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
>>+#endif
>> 
>> #define isxdigit(c)	(('0' <= (c) && (c) <= '9') \
>> 			 || ('a' <= (c) && (c) <= 'f') \
>>
> 
> 
> Philippe:  Wouldn't it be better to just do a 32-bit load and let the endianness be worked out
> by the hardware?  i.e.
> 
> #define GETWORD(v) (*(u32 *)v)
Yes, your alternative is better.
Wouldn't it narrow the scope of the macro to aligned words on POWER7?
I think that all references to GETWORD operate on aligned words anyway.

Philippe
Benjamin Herrenschmidt Dec. 5, 2013, 4:39 a.m.
On Wed, 2013-12-04 at 14:45 +0100, Philippe Bergheaud wrote:

> >>+#ifdef __LITTLE_ENDIAN__
> >>+#define GETWORD(v)	(((v)[3] << 24) + ((v)[2] << 16) + ((v)[1] << 8) + (v)[0])
> >>+#else
> >> #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
> >>+#endif
> >> 
> >> #define isxdigit(c)	(('0' <= (c) && (c) <= '9') \
> >> 			 || ('a' <= (c) && (c) <= 'f') \
> >>
> > 
> > 
> > Philippe:  Wouldn't it be better to just do a 32-bit load and let the endianness be worked out
> > by the hardware?  i.e.
> > 
> > #define GETWORD(v) (*(u32 *)v)
> Yes, your alternative is better.
> Wouldn't it narrow the scope of the macro to aligned words on POWER7?
> I think that all references to GETWORD operate on aligned words anyway.

Well, xmon has to be robust ... as long as you are *certain* that even
with crap entry state it won't try to access unaligned boundaries then
go for it but we aren't looking at performance here.

Cheers,
Ben.
Philippe Bergheaud Dec. 5, 2013, 8:50 a.m.
Benjamin Herrenschmidt wrote:
> On Wed, 2013-12-04 at 14:45 +0100, Philippe Bergheaud wrote:
> 
> 
>>>>+#ifdef __LITTLE_ENDIAN__
>>>>+#define GETWORD(v)	(((v)[3] << 24) + ((v)[2] << 16) + ((v)[1] << 8) + (v)[0])
>>>>+#else
>>>>#define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
>>>>+#endif
>>>>
>>>>#define isxdigit(c)	(('0' <= (c) && (c) <= '9') \
>>>>			 || ('a' <= (c) && (c) <= 'f') \
>>>>
>>>
>>>
>>>Philippe:  Wouldn't it be better to just do a 32-bit load and let the endianness be worked out
>>>by the hardware?  i.e.
>>>
>>>#define GETWORD(v) (*(u32 *)v)
>>
>>Yes, your alternative is better.
>>Wouldn't it narrow the scope of the macro to aligned words on POWER7?
>>I think that all references to GETWORD operate on aligned words anyway.
> 
> 
> Well, xmon has to be robust ... as long as you are *certain* that even
> with crap entry state it won't try to access unaligned boundaries then
> go for it but we aren't looking at performance here.
Thank you Tom and Ben.  We are definitely not looking at performance here.
I prefer to stay on the safe side, and leave the original patch untouched.

Philippe

Patch hide | download patch | download mbox

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index af9d346..6c27804 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -171,7 +171,11 @@  extern void xmon_leave(void);
 #define REG		"%.8lx"
 #endif
 
+#ifdef __LITTLE_ENDIAN__
+#define GETWORD(v)	(((v)[3] << 24) + ((v)[2] << 16) + ((v)[1] << 8) + (v)[0])
+#else
 #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
+#endif
 
 #define isxdigit(c)	(('0' <= (c) && (c) <= '9') \
 			 || ('a' <= (c) && (c) <= 'f') \