[PATCH v2 0/4] MIPS: process: Some fixes and improvements about get_frame_info()

[PATCH v2 0/4] MIPS: process: Some fixes and improvements about get_frame_info()

[Jinyang He]

Not familiar with microMIPS. Not test on microMIPS.

Changelog v2:
- Thanks to Thomas for suggesting that remove and sort header includes.
  [PATCH 1/4] for details.
- Thanks to Jiaxun for test v1 and told me it stuck at loading init.
  I mistakenly thought that r_format.rs and pool32a_format.rs are the same.
  See [PATCH 4/4] for details.

Jinyang He (4):
  MIPS: process: Remove unnecessary headers inclusion
  MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op
  MIPS: Fix get_frame_info() handing of function size
  MIPS: Add is_jr_ra_ins() to end the loop early

 arch/mips/kernel/process.c | 87 ++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 37 deletions(-)

-- 
2.1.0

[PATCH v2 1/4] MIPS: process: Remove unnecessary headers inclusion

[Jinyang He]

Some headers are not necessary, remove them and sort includes.

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
v2:
- Remove useless header inclusion.

 arch/mips/kernel/process.c | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index d7e288f..d737234 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -9,50 +9,31 @@
  * Copyright (C) 2004 Thiemo Seufer
  * Copyright (C) 2013  Imagination Technologies Ltd.
  */
+#include <linux/cpu.h>
 #include <linux/errno.h>
-#include <linux/sched.h>
-#include <linux/sched/debug.h>
-#include <linux/sched/task.h>
-#include <linux/sched/task_stack.h>
-#include <linux/tick.h>
-#include <linux/kernel.h>
-#include <linux/mm.h>
-#include <linux/stddef.h>
-#include <linux/unistd.h>
-#include <linux/export.h>
-#include <linux/ptrace.h>
-#include <linux/mman.h>
-#include <linux/personality.h>
-#include <linux/sys.h>
 #include <linux/init.h>
-#include <linux/completion.h>
 #include <linux/kallsyms.h>
-#include <linux/random.h>
-#include <linux/prctl.h>
+#include <linux/kernel.h>
 #include <linux/nmi.h>
-#include <linux/cpu.h>
+#include <linux/personality.h>
+#include <linux/prctl.h>
+#include <linux/sched.h>
+#include <linux/sched/debug.h>
+#include <linux/sched/task_stack.h>
 
 #include <asm/abi.h>
 #include <asm/asm.h>
-#include <asm/bootinfo.h>
-#include <asm/cpu.h>
 #include <asm/dsemul.h>
 #include <asm/dsp.h>
 #include <asm/fpu.h>
+#include <asm/inst.h>
 #include <asm/irq.h>
-#include <asm/mips-cps.h>
+#include <asm/irq_regs.h>
+#include <asm/isadep.h>
 #include <asm/msa.h>
 #include <asm/mipsregs.h>
 #include <asm/processor.h>
 #include <asm/reg.h>
-#include <linux/uaccess.h>
-#include <asm/io.h>
-#include <asm/elf.h>
-#include <asm/isadep.h>
-#include <asm/inst.h>
-#include <asm/stacktrace.h>
-#include <asm/irq_regs.h>
-#include <asm/exec.h>
 
 #ifdef CONFIG_HOTPLUG_CPU
 void arch_cpu_idle_dead(void)
-- 
2.1.0

Re: [PATCH v2 1/4] MIPS: process: Remove unnecessary headers inclusion

[Huacai Chen]

Reviewed-by: Huacai Chen <chenhuacai@kernel.org>

On Thu, Jan 21, 2021 at 1:44 PM Jinyang He <hejinyang@loongson.cn> wrote:
>
> Some headers are not necessary, remove them and sort includes.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
> v2:
> - Remove useless header inclusion.
>
>  arch/mips/kernel/process.c | 39 ++++++++++-----------------------------
>  1 file changed, 10 insertions(+), 29 deletions(-)
>
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index d7e288f..d737234 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -9,50 +9,31 @@
>   * Copyright (C) 2004 Thiemo Seufer
>   * Copyright (C) 2013  Imagination Technologies Ltd.
>   */
> +#include <linux/cpu.h>
>  #include <linux/errno.h>
> -#include <linux/sched.h>
> -#include <linux/sched/debug.h>
> -#include <linux/sched/task.h>
> -#include <linux/sched/task_stack.h>
> -#include <linux/tick.h>
> -#include <linux/kernel.h>
> -#include <linux/mm.h>
> -#include <linux/stddef.h>
> -#include <linux/unistd.h>
> -#include <linux/export.h>
> -#include <linux/ptrace.h>
> -#include <linux/mman.h>
> -#include <linux/personality.h>
> -#include <linux/sys.h>
>  #include <linux/init.h>
> -#include <linux/completion.h>
>  #include <linux/kallsyms.h>
> -#include <linux/random.h>
> -#include <linux/prctl.h>
> +#include <linux/kernel.h>
>  #include <linux/nmi.h>
> -#include <linux/cpu.h>
> +#include <linux/personality.h>
> +#include <linux/prctl.h>
> +#include <linux/sched.h>
> +#include <linux/sched/debug.h>
> +#include <linux/sched/task_stack.h>
>
>  #include <asm/abi.h>
>  #include <asm/asm.h>
> -#include <asm/bootinfo.h>
> -#include <asm/cpu.h>
>  #include <asm/dsemul.h>
>  #include <asm/dsp.h>
>  #include <asm/fpu.h>
> +#include <asm/inst.h>
>  #include <asm/irq.h>
> -#include <asm/mips-cps.h>
> +#include <asm/irq_regs.h>
> +#include <asm/isadep.h>
>  #include <asm/msa.h>
>  #include <asm/mipsregs.h>
>  #include <asm/processor.h>
>  #include <asm/reg.h>
> -#include <linux/uaccess.h>
> -#include <asm/io.h>
> -#include <asm/elf.h>
> -#include <asm/isadep.h>
> -#include <asm/inst.h>
> -#include <asm/stacktrace.h>
> -#include <asm/irq_regs.h>
> -#include <asm/exec.h>
>
>  #ifdef CONFIG_HOTPLUG_CPU
>  void arch_cpu_idle_dead(void)
> --
> 2.1.0
>

[PATCH v2 2/4] MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op

[Jinyang He]

mm16_r5_format.rt is 5 bits, so directly judge the value if equal or not.
mm_jalr_op requires 7th to 16th bits. These 10 which bits generated by
shifting u_format.uimmediate by 6 may be affected by sign extension.
Thus, take out the 10 bits for comparison.

Without this patch, errors may occur, such as these bits are all ones.

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
 arch/mips/kernel/process.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index d737234..74d7fd8 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -292,8 +292,8 @@ static inline int is_jump_ins(union mips_instruction *ip)
 	 * microMIPS is kind of more fun...
 	 */
 	if (mm_insn_16bit(ip->word >> 16)) {
-		if ((ip->mm16_r5_format.opcode == mm_pool16c_op &&
-		    (ip->mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op))
+		if (ip->mm16_r5_format.opcode == mm_pool16c_op &&
+		    ip->mm16_r5_format.rt == mm_jr16_op)
 			return 1;
 		return 0;
 	}
@@ -305,7 +305,7 @@ static inline int is_jump_ins(union mips_instruction *ip)
 	if (ip->r_format.opcode != mm_pool32a_op ||
 			ip->r_format.func != mm_pool32axf_op)
 		return 0;
-	return ((ip->u_format.uimmediate >> 6) & mm_jalr_op) == mm_jalr_op;
+	return ((ip->u_format.uimmediate >> 6) & GENMASK(9, 0)) == mm_jalr_op;
 #else
 	if (ip->j_format.opcode == j_op)
 		return 1;
-- 
2.1.0

Re: [PATCH v2 2/4] MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op

[Maciej W. Rozycki]

On Thu, 21 Jan 2021, Jinyang He wrote:

> mm16_r5_format.rt is 5 bits, so directly judge the value if equal or not.
> mm_jalr_op requires 7th to 16th bits. These 10 which bits generated by

 The minor opcode extension field is comprised of bits 15:6, not 16:7 as 
your description suggests.  Please be accurate with statements.

> shifting u_format.uimmediate by 6 may be affected by sign extension.

 Why?  The `uimmediate' bit-field member is unsigned for a reason.  No 
sign-extension is made on unsigned data with the right-shift operation.

> Thus, take out the 10 bits for comparison.
> 
> Without this patch, errors may occur, such as these bits are all ones.

 How did you come to this conclusion?

> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index d737234..74d7fd8 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -292,8 +292,8 @@ static inline int is_jump_ins(union mips_instruction *ip)
>  	 * microMIPS is kind of more fun...
>  	 */
>  	if (mm_insn_16bit(ip->word >> 16)) {
> -		if ((ip->mm16_r5_format.opcode == mm_pool16c_op &&
> -		    (ip->mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op))
> +		if (ip->mm16_r5_format.opcode == mm_pool16c_op &&
> +		    ip->mm16_r5_format.rt == mm_jr16_op)
>  			return 1;
>  		return 0;
>  	}

 Code style changes should be submitted on their own as separate patches.

> @@ -305,7 +305,7 @@ static inline int is_jump_ins(union mips_instruction *ip)
>  	if (ip->r_format.opcode != mm_pool32a_op ||
>  			ip->r_format.func != mm_pool32axf_op)
>  		return 0;
> -	return ((ip->u_format.uimmediate >> 6) & mm_jalr_op) == mm_jalr_op;
> +	return ((ip->u_format.uimmediate >> 6) & GENMASK(9, 0)) == mm_jalr_op;

 You've now excluded JALR.HB, JALRS, and JALRS.HB instructions.  The mask 
was there for a reason.  If you can't be bothered to verify microMIPS 
changes say with QEMU, then at the very least please check documentation.  
The intent of this code is clear and these instructions are even spelled 
out explicitly in the comment at the top.

 Thomas, please revert this change as I can see you've already taken it.  
It's plain wrong.

  Maciej

Re: [PATCH v2 2/4] MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op

[Jinyang He]

On 02/08/2021 05:31 AM, Maciej W. Rozycki wrote:

> On Thu, 21 Jan 2021, Jinyang He wrote:
>
>> mm16_r5_format.rt is 5 bits, so directly judge the value if equal or not.
>> mm_jalr_op requires 7th to 16th bits. These 10 which bits generated by
>   The minor opcode extension field is comprised of bits 15:6, not 16:7 as
> your description suggests.  Please be accurate with statements.
>
>> shifting u_format.uimmediate by 6 may be affected by sign extension.
>   Why?  The `uimmediate' bit-field member is unsigned for a reason.  No
> sign-extension is made on unsigned data with the right-shift operation.
>
>> Thus, take out the 10 bits for comparison.
>>
>> Without this patch, errors may occur, such as these bits are all ones.
>   How did you come to this conclusion?
>
>> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
>> index d737234..74d7fd8 100644
>> --- a/arch/mips/kernel/process.c
>> +++ b/arch/mips/kernel/process.c
>> @@ -292,8 +292,8 @@ static inline int is_jump_ins(union mips_instruction *ip)
>>   	 * microMIPS is kind of more fun...
>>   	 */
>>   	if (mm_insn_16bit(ip->word >> 16)) {
>> -		if ((ip->mm16_r5_format.opcode == mm_pool16c_op &&
>> -		    (ip->mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op))
>> +		if (ip->mm16_r5_format.opcode == mm_pool16c_op &&
>> +		    ip->mm16_r5_format.rt == mm_jr16_op)
>>   			return 1;
>>   		return 0;
>>   	}
>   Code style changes should be submitted on their own as separate patches.
>
>> @@ -305,7 +305,7 @@ static inline int is_jump_ins(union mips_instruction *ip)
>>   	if (ip->r_format.opcode != mm_pool32a_op ||
>>   			ip->r_format.func != mm_pool32axf_op)
>>   		return 0;
>> -	return ((ip->u_format.uimmediate >> 6) & mm_jalr_op) == mm_jalr_op;
>> +	return ((ip->u_format.uimmediate >> 6) & GENMASK(9, 0)) == mm_jalr_op;
>   You've now excluded JALR.HB, JALRS, and JALRS.HB instructions.  The mask
> was there for a reason.  If you can't be bothered to verify microMIPS
> changes say with QEMU, then at the very least please check documentation.
> The intent of this code is clear and these instructions are even spelled
> out explicitly in the comment at the top.
It's my fault. :-(

How amazing the opcode design is!

Thanks,
Jinyang

>   Thomas, please revert this change as I can see you've already taken it.
> It's plain wrong.
>
>    Maciej

Re: [PATCH v2 2/4] MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op

[Thomas Bogendoerfer]

On Sun, Feb 07, 2021 at 10:31:38PM +0100, Maciej W. Rozycki wrote:
> On Thu, 21 Jan 2021, Jinyang He wrote:
> 
> > mm16_r5_format.rt is 5 bits, so directly judge the value if equal or not.
> > mm_jalr_op requires 7th to 16th bits. These 10 which bits generated by
> 
>  The minor opcode extension field is comprised of bits 15:6, not 16:7 as 
> your description suggests.  Please be accurate with statements.
> 
> > shifting u_format.uimmediate by 6 may be affected by sign extension.
> 
>  Why?  The `uimmediate' bit-field member is unsigned for a reason.  No 
> sign-extension is made on unsigned data with the right-shift operation.
> 
> > Thus, take out the 10 bits for comparison.
> > 
> > Without this patch, errors may occur, such as these bits are all ones.
> 
>  How did you come to this conclusion?
> 
> > diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> > index d737234..74d7fd8 100644
> > --- a/arch/mips/kernel/process.c
> > +++ b/arch/mips/kernel/process.c
> > @@ -292,8 +292,8 @@ static inline int is_jump_ins(union mips_instruction *ip)
> >  	 * microMIPS is kind of more fun...
> >  	 */
> >  	if (mm_insn_16bit(ip->word >> 16)) {
> > -		if ((ip->mm16_r5_format.opcode == mm_pool16c_op &&
> > -		    (ip->mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op))
> > +		if (ip->mm16_r5_format.opcode == mm_pool16c_op &&
> > +		    ip->mm16_r5_format.rt == mm_jr16_op)
> >  			return 1;
> >  		return 0;
> >  	}
> 
>  Code style changes should be submitted on their own as separate patches.
> 
> > @@ -305,7 +305,7 @@ static inline int is_jump_ins(union mips_instruction *ip)
> >  	if (ip->r_format.opcode != mm_pool32a_op ||
> >  			ip->r_format.func != mm_pool32axf_op)
> >  		return 0;
> > -	return ((ip->u_format.uimmediate >> 6) & mm_jalr_op) == mm_jalr_op;
> > +	return ((ip->u_format.uimmediate >> 6) & GENMASK(9, 0)) == mm_jalr_op;
> 
>  You've now excluded JALR.HB, JALRS, and JALRS.HB instructions.  The mask 
> was there for a reason.  If you can't be bothered to verify microMIPS 
> changes say with QEMU, then at the very least please check documentation.  
> The intent of this code is clear and these instructions are even spelled 
> out explicitly in the comment at the top.
> 
>  Thomas, please revert this change as I can see you've already taken it.  
> It's plain wrong.

It's now reverted in mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

[PATCH v2 3/4] MIPS: Fix get_frame_info() handing of function size

[Jinyang He]

[1]: Commit b6c7a324df37b ("MIPS: Fix get_frame_info() handling of
                            microMIPS function size")
[2]: Commit 2b424cfc69728 ("MIPS: Remove function size check in
                            get_frame_info()")

First patch added a constant to check the number of iterations against.
Second patch fixed the situation that info->func_size is zero.

However, func_size member became useless after the second commit. Without
ip_end, the get frame_size operation may be out of range although KALLSYMS
enabled. Thus, check func_size first. Then make ip_end be the sum of ip
and a constant (512) if func_size is equal to 0. Otherwise make ip_end be
the sum of ip and func_size.

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
 arch/mips/kernel/process.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 74d7fd8..adf29f7 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -371,10 +371,8 @@ static inline int is_sp_move_ins(union mips_instruction *ip, int *frame_size)
 static int get_frame_info(struct mips_frame_info *info)
 {
 	bool is_mmips = IS_ENABLED(CONFIG_CPU_MICROMIPS);
-	union mips_instruction insn, *ip;
-	const unsigned int max_insns = 128;
+	union mips_instruction insn, *ip, *ip_end;
 	unsigned int last_insn_size = 0;
-	unsigned int i;
 	bool saw_jump = false;
 
 	info->pc_offset = -1;
@@ -384,7 +382,9 @@ static int get_frame_info(struct mips_frame_info *info)
 	if (!ip)
 		goto err;
 
-	for (i = 0; i < max_insns; i++) {
+	ip_end = (void *)ip + (info->func_size ? info->func_size : 512);
+
+	while (ip < ip_end) {
 		ip = (void *)ip + last_insn_size;
 
 		if (is_mmips && mm_insn_16bit(ip->halfword[0])) {
-- 
2.1.0

[PATCH v2 4/4] MIPS: Add is_jr_ra_ins() to end the loop early

[Jinyang He]

For those leaf functions, they are likely to have no stack operations.
Add is_jr_ra_ins() to determine whether jr ra has been touched before
the frame_size is found. Without this patch, the get frame_size operation
may be out of range and get the frame_size from the next nested function.

There is no POOL32A format in uapi/asm/inst.h, so some bits here use the
format of r_format instead.
e.g.
---------------------------------------------------------------------
|    format      |  31:26  | 25:21 | 20:16 |    15:6    |    5:0    |
-----------------+---------+-------+-------+------------+------------
| pool32a_format | pool32a |  rt   |  rs   |   jalrc    | pool32axf |
-----------------+---------+-------+-------+------------+------------
|    r_format    |  opcode |  rs   |  rt   | rd:5, re:5 |    func   |
---------------------------------------------------------------------

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
v2:
- microMIPS: ip->r_format.rs -> ip->r_format.rt

 arch/mips/kernel/process.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index adf29f7..592f02e 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -186,6 +186,36 @@ struct mips_frame_info {
 #define J_TARGET(pc,target)	\
 		(((unsigned long)(pc) & 0xf0000000) | ((target) << 2))
 
+static inline int is_jr_ra_ins(union mips_instruction *ip)
+{
+#ifdef CONFIG_CPU_MICROMIPS
+	/*
+	 * jr16 ra
+	 * jr ra
+	 */
+	if (mm_insn_16bit(ip->word >> 16)) {
+		if (ip->mm16_r5_format.opcode == mm_pool16c_op &&
+		    ip->mm16_r5_format.rt == mm_jr16_op &&
+		    ip->mm16_r5_format.imm == 31)
+			return 1;
+		return 0;
+	}
+
+	if (ip->r_format.opcode == mm_pool32a_op &&
+	    ip->r_format.func == mm_pool32axf_op &&
+	    ((ip->u_format.uimmediate >> 6) & GENMASK(9, 0)) == mm_jalr_op &&
+	    ip->r_format.rt == 31)
+		return 1;
+	return 0;
+#else
+	if (ip->r_format.opcode == spec_op &&
+	    ip->r_format.func == jr_op &&
+	    ip->r_format.rs == 31)
+		return 1;
+	return 0;
+#endif
+}
+
 static inline int is_ra_save_ins(union mips_instruction *ip, int *poff)
 {
 #ifdef CONFIG_CPU_MICROMIPS
@@ -398,7 +428,9 @@ static int get_frame_info(struct mips_frame_info *info)
 			last_insn_size = 4;
 		}
 
-		if (!info->frame_size) {
+		if (is_jr_ra_ins(ip)) {
+			break;
+		} else if (!info->frame_size) {
 			is_sp_move_ins(&insn, &info->frame_size);
 			continue;
 		} else if (!saw_jump && is_jump_ins(ip)) {
-- 
2.1.0

Re: [PATCH v2 0/4] MIPS: process: Some fixes and improvements about get_frame_info()

[Thomas Bogendoerfer]

On Thu, Jan 21, 2021 at 01:31:34PM +0800, Jinyang He wrote:
> Not familiar with microMIPS. Not test on microMIPS.
> 
> Changelog v2:
> - Thanks to Thomas for suggesting that remove and sort header includes.
>   [PATCH 1/4] for details.
> - Thanks to Jiaxun for test v1 and told me it stuck at loading init.
>   I mistakenly thought that r_format.rs and pool32a_format.rs are the same.
>   See [PATCH 4/4] for details.
> 
> Jinyang He (4):
>   MIPS: process: Remove unnecessary headers inclusion
>   MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op
>   MIPS: Fix get_frame_info() handing of function size
>   MIPS: Add is_jr_ra_ins() to end the loop early
> 
>  arch/mips/kernel/process.c | 87 ++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 37 deletions(-)

I've re-added linux/random.h and asm/mips-cps.h in your first patch and
applied all four patches to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]