[PATCH] ARM: alignment: Use is_wide_instruction() to check wide instruction

[PATCH] ARM: alignment: Use is_wide_instruction() to check wide instruction

[Yingjoe Chen]

I first notice the comment is incorrect, then I realize there's another
macro which do exactly the same thing.
Tested with hand written userspace program with a few wide instructions
to make sure this still work as expect.

8<-------------------------------------------------
do_alignment() is using locally added IS_T32() macro to check if an
instruction is a Thumb-2 32 bit instruction. The macro
is_wide_instruction() is doing the same thing, with slightly faster
implementation.
Change to use is_wide_instruction() in do_alignment() and remove
IS_T32().

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 arch/arm/mm/alignment.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 2c0c541..f8e82f5 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -71,10 +71,6 @@
 
 #define BAD_INSTR 	0xdeadc0de
 
-/* Thumb-2 32 bit format per ARMv7 DDI0406A A6.3, either f800h,e800h,f800h */
-#define IS_T32(hi16) \
-	(((hi16) & 0xe000) == 0xe000 && ((hi16) & 0x1800))
-
 static unsigned long ai_user;
 static unsigned long ai_sys;
 static void *ai_sys_last_pc;
@@ -770,7 +766,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		tinstr = __mem_to_opcode_thumb16(tinstr);
 		if (!fault) {
 			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
-			    IS_T32(tinstr)) {
+			    is_wide_instruction(tinstr)) {
 				/* Thumb-2 32-bit */
 				u16 tinst2 = 0;
 				fault = probe_kernel_address(ptr + 1, tinst2);
-- 
1.8.1.1.dirty

[PATCH] ARM: alignment: Use is_wide_instruction() to check wide instruction

[Dave Martin]

On Fri, Apr 10, 2015 at 10:05:41PM +0800, Yingjoe Chen wrote:
> 
> I first notice the comment is incorrect, then I realize there's another
> macro which do exactly the same thing.
> Tested with hand written userspace program with a few wide instructions
> to make sure this still work as expect.
> 
> 8<-------------------------------------------------
> do_alignment() is using locally added IS_T32() macro to check if an
> instruction is a Thumb-2 32 bit instruction. The macro
> is_wide_instruction() is doing the same thing, with slightly faster
> implementation.
> Change to use is_wide_instruction() in do_alignment() and remove
> IS_T32().

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

The existing code is correct, but the comment is indeed wrong.
Consolidating this with one of the other existing macros makes sense.


!__opcode_is_thumb16() could also be used here (or is_wide_instruction()
redefined in terms of it), but that's not essential.

Cheers
---Dave

> 
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  arch/arm/mm/alignment.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 2c0c541..f8e82f5 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -71,10 +71,6 @@
>  
>  #define BAD_INSTR 	0xdeadc0de
>  
> -/* Thumb-2 32 bit format per ARMv7 DDI0406A A6.3, either f800h,e800h,f800h */
> -#define IS_T32(hi16) \
> -	(((hi16) & 0xe000) == 0xe000 && ((hi16) & 0x1800))
> -
>  static unsigned long ai_user;
>  static unsigned long ai_sys;
>  static void *ai_sys_last_pc;
> @@ -770,7 +766,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  		tinstr = __mem_to_opcode_thumb16(tinstr);
>  		if (!fault) {
>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
> -			    IS_T32(tinstr)) {
> +			    is_wide_instruction(tinstr)) {
>  				/* Thumb-2 32-bit */
>  				u16 tinst2 = 0;
>  				fault = probe_kernel_address(ptr + 1, tinst2);
> -- 
> 1.8.1.1.dirty
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: alignment: Use is_wide_instruction() to check wide instruction

[Yingjoe Chen]

On Mon, 2015-04-13 at 13:48 +0100, Dave Martin wrote:
> On Fri, Apr 10, 2015 at 10:05:41PM +0800, Yingjoe Chen wrote:
> > 
> > I first notice the comment is incorrect, then I realize there's another
> > macro which do exactly the same thing.
> > Tested with hand written userspace program with a few wide instructions
> > to make sure this still work as expect.
> > 
> > 8<-------------------------------------------------
> > do_alignment() is using locally added IS_T32() macro to check if an
> > instruction is a Thumb-2 32 bit instruction. The macro
> > is_wide_instruction() is doing the same thing, with slightly faster
> > implementation.
> > Change to use is_wide_instruction() in do_alignment() and remove
> > IS_T32().
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> 
> The existing code is correct, but the comment is indeed wrong.
> Consolidating this with one of the other existing macros makes sense.
> 
> 
> !__opcode_is_thumb16() could also be used here (or is_wide_instruction()
> redefined in terms of it), but that's not essential.

Hi Dave,

Thanks for your review.

I did notice there are __opcode_is_thumb16/32. They need to handle
32bits instruction instead of 16bits only for is_wide_instruction, so I
think using same implementation for them is a different story.

Joe.C

> 
> Cheers
> ---Dave
> 
> > 
> > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> >  arch/arm/mm/alignment.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> > index 2c0c541..f8e82f5 100644
> > --- a/arch/arm/mm/alignment.c
> > +++ b/arch/arm/mm/alignment.c
> > @@ -71,10 +71,6 @@
> >  
> >  #define BAD_INSTR 	0xdeadc0de
> >  
> > -/* Thumb-2 32 bit format per ARMv7 DDI0406A A6.3, either f800h,e800h,f800h */
> > -#define IS_T32(hi16) \
> > -	(((hi16) & 0xe000) == 0xe000 && ((hi16) & 0x1800))
> > -
> >  static unsigned long ai_user;
> >  static unsigned long ai_sys;
> >  static void *ai_sys_last_pc;
> > @@ -770,7 +766,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> >  		tinstr = __mem_to_opcode_thumb16(tinstr);
> >  		if (!fault) {
> >  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
> > -			    IS_T32(tinstr)) {
> > +			    is_wide_instruction(tinstr)) {
> >  				/* Thumb-2 32-bit */
> >  				u16 tinst2 = 0;
> >  				fault = probe_kernel_address(ptr + 1, tinst2);
> > -- 
> > 1.8.1.1.dirty
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH] ARM: alignment: Use is_wide_instruction() to check wide instruction

[Dave Martin]

On Mon, Apr 13, 2015 at 10:47:24PM +0800, Yingjoe Chen wrote:
> On Mon, 2015-04-13 at 13:48 +0100, Dave Martin wrote:
> > On Fri, Apr 10, 2015 at 10:05:41PM +0800, Yingjoe Chen wrote:
> > > 
> > > I first notice the comment is incorrect, then I realize there's another
> > > macro which do exactly the same thing.
> > > Tested with hand written userspace program with a few wide instructions
> > > to make sure this still work as expect.
> > > 
> > > 8<-------------------------------------------------
> > > do_alignment() is using locally added IS_T32() macro to check if an
> > > instruction is a Thumb-2 32 bit instruction. The macro
> > > is_wide_instruction() is doing the same thing, with slightly faster
> > > implementation.
> > > Change to use is_wide_instruction() in do_alignment() and remove
> > > IS_T32().
> > 
> > Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> > 
> > The existing code is correct, but the comment is indeed wrong.
> > Consolidating this with one of the other existing macros makes sense.
> > 
> > 
> > !__opcode_is_thumb16() could also be used here (or is_wide_instruction()
> > redefined in terms of it), but that's not essential.
> 
> Hi Dave,
> 
> Thanks for your review.
> 
> I did notice there are __opcode_is_thumb16/32. They need to handle
> 32bits instruction instead of 16bits only for is_wide_instruction, so I
> think using same implementation for them is a different story.
> 
> Joe.C

No worries.  It can be tidied up some more later if someone finds a good
reason for it.

Cheers
---Dave

[PATCH] ARM: alignment: Use is_wide_instruction() to check wide instruction

[Yingjoe Chen]

On Fri, 2015-04-10 at 22:05 +0800, Yingjoe Chen wrote:
> I first notice the comment is incorrect, then I realize there's another
> macro which do exactly the same thing.
> Tested with hand written userspace program with a few wide instructions
> to make sure this still work as expect.
> 
> 8<-------------------------------------------------
> do_alignment() is using locally added IS_T32() macro to check if an
> instruction is a Thumb-2 32 bit instruction. The macro
> is_wide_instruction() is doing the same thing, with slightly faster
> implementation.
> Change to use is_wide_instruction() in do_alignment() and remove
> IS_T32().
> 


Hi Russell,

Any suggestion about this patch?
Should I put this into your patch system?

Joe.C


> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  arch/arm/mm/alignment.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 2c0c541..f8e82f5 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -71,10 +71,6 @@
>  
>  #define BAD_INSTR 	0xdeadc0de
>  
> -/* Thumb-2 32 bit format per ARMv7 DDI0406A A6.3, either f800h,e800h,f800h */
> -#define IS_T32(hi16) \
> -	(((hi16) & 0xe000) == 0xe000 && ((hi16) & 0x1800))
> -
>  static unsigned long ai_user;
>  static unsigned long ai_sys;
>  static void *ai_sys_last_pc;
> @@ -770,7 +766,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  		tinstr = __mem_to_opcode_thumb16(tinstr);
>  		if (!fault) {
>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
> -			    IS_T32(tinstr)) {
> +			    is_wide_instruction(tinstr)) {
>  				/* Thumb-2 32-bit */
>  				u16 tinst2 = 0;
>  				fault = probe_kernel_address(ptr + 1, tinst2);