Commit 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
added support for unwinding the stack on microMIPS, which has a mix of
16 and 32bit sized instructions. Unfortunately a lot of the code
introduced had bugs which prevented it working correctly in all cases.
This series aims to address those issues. The series also provides
additional fixup to some changes made in v4.11, which also aimed to
address errors in commit 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned
access support.").
These patches have been tested on qemu M14Kc micromips and tested for
regression on ci40, Boston, Octeon III & malta.
For ease of backport, I have opted to just fix the issues with the
existing code to start off with, despite the violations of the code
style in the areas being fixed. The final patch in the series refactors
the is_sp_move_ins function so that it compiles with the coding
standard.
This series is based on v4.13-rc4.
Changes in v3:
- Remove hack which attempted to cope with 16bit instructions
- New patch to fix detection of addiusp instruction
- Deal with special behaviour of addiusp immediates 0x0,0x1,0x1fe & 0x1ff
- New patch to fix big endian systems
Changes in v2:
- Keep locals in reverse christmas tree order
- Replace conditional with xor and subtract
- Refactor is_sp_move_ins to interpret immediate inline.
Matt Redfearn (6):
MIPS: Handle non word sized instructions when examining frame
MIPS: microMIPS: Fix detection of addiusp instruction
MIPS: microMIPS: Fix decoding of addiusp instruction
MIPS: microMIPS: Fix decoding of swsp16 instruction
MIPS: Stacktrace: Fix microMIPS stack unwinding on big endian systems
MIPS: Refactor handling of stack pointer in get_frame_info
arch/mips/include/uapi/asm/inst.h | 2 +-
arch/mips/kernel/process.c | 80 ++++++++++++++++++++-------------------
2 files changed, 43 insertions(+), 39 deletions(-)
--
2.7.4
Commit 34c2f668d0f6b ("MIPS: microMIPS: Add unaligned access support.")
added fairly broken support for handling 16bit microMIPS instructions in
get_frame_info(). It adjusts the instruction pointer by 16bits in the
case of a 16bit sp move instruction, but not any other 16bit
instruction.
Commit b6c7a324df37 ("MIPS: Fix get_frame_info() handling of microMIPS
function size") goes some way to fixing get_frame_info() to iterate over
microMIPS instuctions, but the instruction pointer is still manipulated
using a postincrement, and is of union mips_instruction type. Since the
union is sized to the largest member (a word), but microMIPS
instructions are a mix of halfword and word sizes, the function does not
always iterate correctly, ending up misaligned with the instruction
stream and interpreting it incorrectly.
Since the instruction modifying the stack pointer is usually the first
in the function, that one is usually handled correctly. But the
instruction which saves the return address to the sp is some variable
number of instructions into the frame and is frequently missed due to
not being on a word boundary, leading to incomplete walking of the
stack.
Fix this by incrementing the instruction pointer based on the size of
the previously decoded instruction (& remove the hack introduced by
commit 34c2f668d0f6b ("MIPS: microMIPS: Add unaligned access support.")
which adjusts the instruction pointer in the case of a 16bit sp move
instruction, but not any other).
Fixes: 34c2f668d0f6b ("MIPS: microMIPS: Add unaligned access support.")
Signed-off-by: Matt Redfearn <[email protected]>
---
Changes in v3:
- Remove hack which attempted to cope with 16bit instructions
Changes in v2:
- Keep locals in reverse christmas tree order
arch/mips/kernel/process.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 5351e1f3950d..5950ecf469e9 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -348,6 +348,7 @@ static int get_frame_info(struct mips_frame_info *info)
bool is_mmips = IS_ENABLED(CONFIG_CPU_MICROMIPS);
union mips_instruction insn, *ip, *ip_end;
const unsigned int max_insns = 128;
+ unsigned int last_insn_size = 0;
unsigned int i;
info->pc_offset = -1;
@@ -359,15 +360,19 @@ static int get_frame_info(struct mips_frame_info *info)
ip_end = (void *)ip + info->func_size;
- for (i = 0; i < max_insns && ip < ip_end; i++, ip++) {
+ for (i = 0; i < max_insns && ip < ip_end; i++) {
+ ip = (void *)ip + last_insn_size;
if (is_mmips && mm_insn_16bit(ip->halfword[0])) {
insn.halfword[0] = 0;
insn.halfword[1] = ip->halfword[0];
+ last_insn_size = 2;
} else if (is_mmips) {
insn.halfword[0] = ip->halfword[1];
insn.halfword[1] = ip->halfword[0];
+ last_insn_size = 4;
} else {
insn.word = ip->word;
+ last_insn_size = 4;
}
if (is_jump_ins(&insn))
@@ -389,8 +394,6 @@ static int get_frame_info(struct mips_frame_info *info)
tmp = (ip->halfword[0] >> 1);
info->frame_size = -(signed short)(tmp & 0xf);
}
- ip = (void *) &ip->halfword[1];
- ip--;
} else
#endif
info->frame_size = - ip->i_format.simmediate;
--
2.7.4
When the immediate encoded in the instruction is accessed, it is sign
extended due to being a signed value being assigned to a signed integer.
The ISA specifies that this operation is an unsigned operation.
The sign extension leads us to incorrectly decode:
801e9c8e: cbf1 sw ra,68(sp)
As having an immediate of 1073741809.
Since the instruction format does not specify signed/unsigned, and this
is currently the only location to use this instuction format, change it
to an unsigned immediate.
Fixes: bb9bc4689b9c ("MIPS: Calculate microMIPS ra properly when unwinding the stack")
Suggested-by: Paul Burton <[email protected]>
Signed-off-by: Matt Redfearn <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
Changes in v3: None
Changes in v2: None
arch/mips/include/uapi/asm/inst.h | 2 +-
arch/mips/kernel/process.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/mips/include/uapi/asm/inst.h b/arch/mips/include/uapi/asm/inst.h
index d61897535926..6abea5183d7c 100644
--- a/arch/mips/include/uapi/asm/inst.h
+++ b/arch/mips/include/uapi/asm/inst.h
@@ -981,7 +981,7 @@ struct mm16_r3_format { /* Load from global pointer format */
struct mm16_r5_format { /* Load/store from stack pointer format */
__BITFIELD_FIELD(unsigned int opcode : 6,
__BITFIELD_FIELD(unsigned int rt : 5,
- __BITFIELD_FIELD(signed int simmediate : 5,
+ __BITFIELD_FIELD(unsigned int imm : 5,
__BITFIELD_FIELD(unsigned int : 16, /* Ignored */
;))))
};
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 288a79bd0e72..6fa726b0be01 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -214,7 +214,7 @@ static inline int is_ra_save_ins(union mips_instruction *ip, int *poff)
if (ip->mm16_r5_format.rt != 31)
return 0;
- *poff = ip->mm16_r5_format.simmediate;
+ *poff = ip->mm16_r5_format.imm;
*poff = (*poff << 2) / sizeof(ulong);
return 1;
--
2.7.4
Commit 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
added handling of microMIPS instructions to manipulate the stack
pointer. The code that was added violates code style rules with long
lines caused by lots of nested conditionals.
The added code interprets (inline) any known stack pointer manipulation
instruction to find the stack frame size. Handling the microMIPS cases
added quite a bit of complication to this function.
Refactor is_sp_move_ins to perform the interpretation of the immediate
as the instruction manipulating the stack pointer is found. This reduces
the amount of indentation required in get_frame_info, and more closely
matches the operation of is_ra_save_ins.
Suggested-by: Maciej W. Rozycki <[email protected]>
Signed-off-by: Matt Redfearn <[email protected]>
---
Changes in v3: None
Changes in v2:
- Refactor is_sp_move_ins to interpret immediate inline.
arch/mips/kernel/process.c | 61 +++++++++++++++++++++++-----------------------
1 file changed, 30 insertions(+), 31 deletions(-)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index ba7d5f73c8b0..26e2cdcd0057 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -313,9 +313,11 @@ static inline int is_jump_ins(union mips_instruction *ip)
#endif
}
-static inline int is_sp_move_ins(union mips_instruction *ip)
+static inline int is_sp_move_ins(union mips_instruction *ip, int *frame_size)
{
#ifdef CONFIG_CPU_MICROMIPS
+ unsigned short tmp;
+
/*
* addiusp -imm
* addius5 sp,-imm
@@ -325,20 +327,39 @@ static inline int is_sp_move_ins(union mips_instruction *ip)
* microMIPS is not more fun...
*/
if (mm_insn_16bit(ip->word >> 16)) {
- return (ip->mm16_r3_format.opcode == mm_pool16d_op &&
- ip->mm16_r3_format.simmediate & mm_addiusp_func) ||
- (ip->mm16_r5_format.opcode == mm_pool16d_op &&
- ip->mm16_r5_format.rt == 29);
+ if (ip->mm16_r3_format.opcode == mm_pool16d_op &&
+ ip->mm16_r3_format.simmediate & mm_addiusp_func) {
+ tmp = ip->mm_b0_format.simmediate >> 1;
+ tmp = ((tmp & 0x1ff) ^ 0x100) - 0x100;
+ if ((tmp + 2) < 4) /* 0x0,0x1,0x1fe,0x1ff are special */
+ tmp ^= 0x100;
+ *frame_size = -(signed short)(tmp << 2);
+ return 1;
+ }
+ if (ip->mm16_r5_format.opcode == mm_pool16d_op &&
+ ip->mm16_r5_format.rt == 29) {
+ tmp = ip->mm16_r5_format.imm >> 1;
+ *frame_size = -(signed short)(tmp & 0xf);
+ return 1;
+ }
+ return 0;
}
- return ip->mm_i_format.opcode == mm_addiu32_op &&
- ip->mm_i_format.rt == 29 && ip->mm_i_format.rs == 29;
+ if (ip->mm_i_format.opcode == mm_addiu32_op &&
+ ip->mm_i_format.rt == 29 && ip->mm_i_format.rs == 29) {
+ *frame_size = -ip->i_format.simmediate;
+ return 1;
+ }
#else
/* addiu/daddiu sp,sp,-imm */
if (ip->i_format.rs != 29 || ip->i_format.rt != 29)
return 0;
- if (ip->i_format.opcode == addiu_op || ip->i_format.opcode == daddiu_op)
+
+ if (ip->i_format.opcode == addiu_op ||
+ ip->i_format.opcode == daddiu_op) {
+ *frame_size = -ip->i_format.simmediate;
return 1;
+ }
#endif
return 0;
}
@@ -377,29 +398,7 @@ static int get_frame_info(struct mips_frame_info *info)
break;
if (!info->frame_size) {
- if (is_sp_move_ins(&insn))
- {
-#ifdef CONFIG_CPU_MICROMIPS
- if (mm_insn_16bit(insn.word >> 16))
- {
- unsigned short tmp;
-
- if (ip->mm16_r3_format.simmediate & mm_addiusp_func)
- {
- tmp = ip->mm_b0_format.simmediate >> 1;
- tmp = ((tmp & 0x1ff) ^ 0x100) - 0x100;
- /* 0x0,0x1,0x1fe,0x1ff are special */
- if ((tmp + 2) < 4)
- tmp ^= 0x100;
- info->frame_size = -(signed short)(tmp << 2);
- } else {
- tmp = (ip->mm16_r5_format.imm >> 1);
- info->frame_size = -(signed short)(tmp & 0xf);
- }
- } else
-#endif
- info->frame_size = - ip->i_format.simmediate;
- }
+ is_sp_move_ins(&insn, &info->frame_size);
continue;
}
if (info->pc_offset == -1 &&
--
2.7.4
The stack unwinding code uses the mips_instuction union to decode the
instructions it finds. That union uses the __BITFIELD_FIELD macro to
reorder depending on endianness. The stack unwinding code always places
16bit instructions in halfword 1 of the union. This makes the union
accesses correct for little endian systems. Similarly, 32bit
instructions are reordered such that they are correct for little endian
systems. This handling leaves unwinding the stack on big endian systems
broken, as the mips_instruction union will then look for the fields in
the wrong halfword.
To fix this, use a logical shift to place the 16bit instruction into the
correct position in the word field of the union. Use the same shifting
to order the 2 halfwords of 32bit instuctions. Then replace accesses to
the halfword with accesses to the shifted word.
In the case of the ADDIUS5 instruction, switch to using the
mm16_r5_format union member to avoid the need for a 16bit shift.
Fixes: 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
Signed-off-by: Matt Redfearn <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
Changes in v3:
New patch to fix big endian systems
Changes in v2: None
arch/mips/kernel/process.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 6fa726b0be01..ba7d5f73c8b0 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -208,7 +208,7 @@ static inline int is_ra_save_ins(union mips_instruction *ip, int *poff)
*
* microMIPS is way more fun...
*/
- if (mm_insn_16bit(ip->halfword[1])) {
+ if (mm_insn_16bit(ip->word >> 16)) {
switch (ip->mm16_r5_format.opcode) {
case mm_swsp16_op:
if (ip->mm16_r5_format.rt != 31)
@@ -287,7 +287,7 @@ static inline int is_jump_ins(union mips_instruction *ip)
*
* microMIPS is kind of more fun...
*/
- if (mm_insn_16bit(ip->halfword[1])) {
+ 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))
return 1;
@@ -324,7 +324,7 @@ static inline int is_sp_move_ins(union mips_instruction *ip)
*
* microMIPS is not more fun...
*/
- if (mm_insn_16bit(ip->halfword[1])) {
+ if (mm_insn_16bit(ip->word >> 16)) {
return (ip->mm16_r3_format.opcode == mm_pool16d_op &&
ip->mm16_r3_format.simmediate & mm_addiusp_func) ||
(ip->mm16_r5_format.opcode == mm_pool16d_op &&
@@ -363,12 +363,10 @@ static int get_frame_info(struct mips_frame_info *info)
for (i = 0; i < max_insns && ip < ip_end; i++) {
ip = (void *)ip + last_insn_size;
if (is_mmips && mm_insn_16bit(ip->halfword[0])) {
- insn.halfword[0] = 0;
- insn.halfword[1] = ip->halfword[0];
+ insn.word = ip->halfword[0] << 16;
last_insn_size = 2;
} else if (is_mmips) {
- insn.halfword[0] = ip->halfword[1];
- insn.halfword[1] = ip->halfword[0];
+ insn.word = ip->halfword[0] << 16 | ip->halfword[1];
last_insn_size = 4;
} else {
insn.word = ip->word;
@@ -382,7 +380,7 @@ static int get_frame_info(struct mips_frame_info *info)
if (is_sp_move_ins(&insn))
{
#ifdef CONFIG_CPU_MICROMIPS
- if (mm_insn_16bit(ip->halfword[0]))
+ if (mm_insn_16bit(insn.word >> 16))
{
unsigned short tmp;
@@ -395,7 +393,7 @@ static int get_frame_info(struct mips_frame_info *info)
tmp ^= 0x100;
info->frame_size = -(signed short)(tmp << 2);
} else {
- tmp = (ip->halfword[0] >> 1);
+ tmp = (ip->mm16_r5_format.imm >> 1);
info->frame_size = -(signed short)(tmp & 0xf);
}
} else
--
2.7.4
Commit 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
added handling of microMIPS instructions to manipulate the stack
pointer. Unfortunately the decoding of the addiusp instruction was
incorrect, and performed a left shift by 2 bits to the raw immediate,
rather than decoding the immediate and then performing the shift, as
documented in the ISA.
This led to incomplete stack traces, due to incorrect frame sizes being
calculated. For example the instruction:
801faee0 <do_sys_poll>:
801faee0: 4e25 addiu sp,sp,-952
As decoded by objdump, would be interpreted by the existing code as
having manipulated the stack pointer by +1096.
Fix this by changing the order of decoding the immediate and applying
the left shift. Also change to accessing the instuction through the
union to avoid the endianness problem of accesing halfword[0], which
will fail on big endian systems.
Cope with the special behaviour of immediates 0x0, 0x1, 0x1fe and 0x1ff
by XORing with 0x100 again if mod(immediate) < 4. This logic was tested
with the following test code:
int main(int argc, char **argv)
{
unsigned int enc;
int imm;
for (enc = 0; enc < 512; ++enc) {
int tmp = enc << 2;
imm = -(signed short)(tmp | ((tmp & 0x100) ? 0xfe00 : 0));
unsigned short tmp = enc;
tmp = (tmp ^ 0x100) - 0x100;
if ((unsigned short)(tmp + 2) < 4)
tmp ^= 0x100;
imm = -(signed short)(tmp << 2);
printf("%#x\t%d\t->\t(%#x\t%d)\t%#x\t%d\n",
enc, enc,
(short)tmp, (short)tmp,
imm, imm);
}
return EXIT_SUCCESS;
}
Which generates the table:
input encoding -> tmp (matching manual) frame size
-----------------------------------------------------------------------
0 0 -> (0x100 256) 0xfffffc00 -1024
0x1 1 -> (0x101 257) 0xfffffbfc -1028
0x2 2 -> (0x2 2) 0xfffffff8 -8
0x3 3 -> (0x3 3) 0xfffffff4 -12
...
0xfe 254 -> (0xfe 254) 0xfffffc08 -1016
0xff 255 -> (0xff 255) 0xfffffc04 -1020
0x100 256 -> (0xffffff00 -256) 0x400 1024
0x101 257 -> (0xffffff01 -255) 0x3fc 1020
...
0x1fc 508 -> (0xfffffffc -4) 0x10 16
0x1fd 509 -> (0xfffffffd -3) 0xc 12
0x1fe 510 -> (0xfffffefe -258) 0x408 1032
0x1ff 511 -> (0xfffffeff -257) 0x404 1028
Thanks to James Hogan for the test code & verifying the logic.
Fixes: 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
Suggested-by: James Hogan <[email protected]>
Signed-off-by: Matt Redfearn <[email protected]>
---
Changes in v3:
- Deal with special behaviour of addiusp immediates 0x0,0x1,0x1fe & 0x1ff
Changes in v2:
- Replace conditional with xor and subtract
arch/mips/kernel/process.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 9d38faf01055..288a79bd0e72 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -386,10 +386,14 @@ static int get_frame_info(struct mips_frame_info *info)
{
unsigned short tmp;
- if (ip->halfword[0] & mm_addiusp_func)
+ if (ip->mm16_r3_format.simmediate & mm_addiusp_func)
{
- tmp = (((ip->halfword[0] >> 1) & 0x1ff) << 2);
- info->frame_size = -(signed short)(tmp | ((tmp & 0x100) ? 0xfe00 : 0));
+ tmp = ip->mm_b0_format.simmediate >> 1;
+ tmp = ((tmp & 0x1ff) ^ 0x100) - 0x100;
+ /* 0x0,0x1,0x1fe,0x1ff are special */
+ if ((tmp + 2) < 4)
+ tmp ^= 0x100;
+ info->frame_size = -(signed short)(tmp << 2);
} else {
tmp = (ip->halfword[0] >> 1);
info->frame_size = -(signed short)(tmp & 0xf);
--
2.7.4
The addiusp instruction uses the pool16d opcode, with bit 0 of the
immediate set. The test for the addiusp opcode erroneously did a logical
and of the immediate with mm_addiusp_func, which has value 1, so this
test always passes when the immediate is non-zero.
Fix the test by replacing the logical and with a bitwise and.
Fixes: 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
Signed-off-by: Matt Redfearn <[email protected]>
---
Changes in v3:
- New patch to fix detection of addiusp instruction
Changes in v2: None
arch/mips/kernel/process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 5950ecf469e9..9d38faf01055 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -326,7 +326,7 @@ static inline int is_sp_move_ins(union mips_instruction *ip)
*/
if (mm_insn_16bit(ip->halfword[1])) {
return (ip->mm16_r3_format.opcode == mm_pool16d_op &&
- ip->mm16_r3_format.simmediate && mm_addiusp_func) ||
+ ip->mm16_r3_format.simmediate & mm_addiusp_func) ||
(ip->mm16_r5_format.opcode == mm_pool16d_op &&
ip->mm16_r5_format.rt == 29);
}
--
2.7.4