MIPS: microMIPS: Refactor mips16 get_frame_info support
PATCH v2 1/1 MIPS: microMIPS: Refactor mips16 get_frame_info support
Current get_frame_info implementation works on word boundary, this can lead to alignment and endian issues in microMIPS/MIPS16 mode, due to:
1. MIPS16 instructions can be 16bit or 32bit
2. MIPS16 32bit instructions may not be on the word boundary
3. MIPS16 instructions are placed in 32-bit memory element, in endian-dependent order.
Example:
insn1 = 16bit => word1, hword[0]
insn2 = 32bit => word1, hword[1], word2, hword[0]
insn3 = 16bit => word2, hword[1]
Big Endian
hword[0] hword[1] hword[0] hword[1]
+-------------+-------------+-------------+-------------+
| insn1 | insn2 | insn2' | insn3 |
+-------------+-------------+-------------+-------------+
31 word1 0 31 word2 0
Little Endian
hword[1] hword[0] hword[1] hword[0]
+-------------+-------------+-------------+-------------+
| insn2 | insn1 | insn3 | insn2' |
+-------------+-------------+-------------+-------------+
31 word1 0 31 word2 0This patch refactors microMIPS get_frame_info by implementing fetch_instruction() to fetch instructions on word boundary, and MIPS16_fetch_halfword() to assemble halfwords into 16bit/32bit MIPS16 instructions for further processing.
This patch also fixes sibling call handling and schedule_mfi initialization for microMIPS.
--- arch/mips/include/asm/cpu-features.h | 7 + arch/mips/kernel/process.c | 322 ++++++++++++++++++++-------------- 2 files changed, 198 insertions(+), 131 deletions(-)
arch/mips/include/asm/cpu-features.h +7 -0
@@ -101,6 +101,13 @@#ifndef cpu_has_mmips#define cpu_has_mmips (cpu_data[0].options & MIPS_CPU_MICROMIPS)#endif +#ifndef kernel_uses_mmips +# ifdef CONFIG_CPU_MICROMIPS +# define kernel_uses_mmips 1 +# else +# define kernel_uses_mmips 0 +# endif +#endif #ifndef cpu_has_vtag_icache#define cpu_has_vtag_icache (cpu_data[0].icache.flags & MIPS_CACHE_VTAG)#endif
arch/mips/kernel/process.c +191 -131
372 lines not on this page · raw message
Signature · 1 line
1.7.10.2 (Apple Git-33)
Hi Tony,
Thanks for doing this work. A couple of notes below. Many are
preexisting problems, but fiddling with this code is a good opportunity to get it right.
Quoted 28 lines · Tony Wu
Current
get_frame_infoimplementation works on word boundary, this can lead to alignment and endian issues in microMIPS/MIPS16 mode, due to:1. MIPS16 instructions can be 16bit or 32bit
2. MIPS16 32bit instructions may not be on the word boundary
3. MIPS16 instructions are placed in 32-bit memory element, in endian-dependent order.Example:
insn1 = 16bit => word1, hword[0] insn2 = 32bit => word1, hword[1], word2, hword[0] insn3 = 16bit => word2, hword[1] Big Endian hword[0] hword[1] hword[0] hword[1] +-------------+-------------+-------------+-------------+ | insn1 | insn2 | insn2' | insn3 | +-------------+-------------+-------------+-------------+ 31 word1 0 31 word2 0 Little Endian hword[1] hword[0] hword[1] hword[0] +-------------+-------------+-------------+-------------+ | insn2 | insn1 | insn3 | insn2' | +-------------+-------------+-------------+-------------+ 31 word1 0 31 word2 0
Quoted 4 lines
This patch refactors microMIPS
get_frame_infoby implementingfetch_instruction()to fetch instructions on word boundary, andMIPS16_fetch_halfword()to assemble halfwords into 16bit/32bit MIPS16 instructions for further processing.
I think you're overcomplicating things a bit. In the MIPS16 and
microMIPS modes instructions are simply sequences of 16-bit halfwords (unlike the standard MIPS mode where instructions are 32-bit words), each of which is stored in memory using the bus endianness. Then in the MIPS16 mode instructions are either one or two 16-bit halfwords and in the microMIPS mode they are any between one and three 16-bit halfwords (the POOL48 major opcode for the latters; we may ignore them until we support the 64-bit microMIPS mode). I think it's easier to see them this way, and also likely to process them.
Also I suggest to avoid referring to the MIPS16 mode except perhaps the
first sentence, because none of this code actually handles MIPS16 instructions. If you want to refer to both MIPS16 and microMIPS instruction sets in a generic way, then you may use "compressed ISA mode instructions" or suchlike.
I hope you're not confusing the two instruction sets.
Quoted 39 lines
arch/mips/kernel/process.c +0
-29@@ -211,124 +211,168 @@ struct mips_frame_info {int pc_offset; }; -#define J_TARGET (pc,target) \ - (((unsigned long)(pc) & 0xf0000000) | ((target) << 2)) -static inline int is_ra_save_ins (union mips_instruction *ip) { -#ifdef CONFIG_CPU_MICROMIPS - union mips_instruction mmi; - - /* - * swsp ra,offset - * swm16 reglist,offset(sp) - * swm32 reglist,offset(sp) - * sw32 ra,offset(sp) - * jradiussp - NOT SUPPORTED - * - * microMIPS is way more fun... - */ - if (mm_insn_16bit(ip->halfword[0])) { - mmi.word = (ip->halfword[0] << 16); - return ((mmi.mm16_r5_format.opcode == mm_swsp16_op && - mmi.mm16_r5_format.rt == 31) || - (mmi.mm16_m_format.opcode == mm_pool16c_op && - mmi.mm16_m_format.func == mm_swm16_op)); - } - else { - mmi.halfword[0] = ip->halfword[1]; - mmi.halfword[1] = ip->halfword[0]; - return ((mmi.mm_m_format.opcode == mm_pool32b_op && - mmi.mm_m_format.rd > 9 && - mmi.mm_m_format.base == 29 && - mmi.mm_m_format.func == mm_swm32_func) ||
arch/mips/kernel/process.c struct mips_frame_info { · line 222
Quoted 10 lines
arch/mips/kernel/process.c +7
-3- (mmi.i_format.opcode == mm_sw32_op && - mmi.i_format.rs ==29 && - mmi.i_format.rt == 31)); +if (kernel_uses_mmips) { +/* +* swsp ra,offset +* swm16 reglist,offset(sp) +* swm32 reglist,offset(sp) +* sw32 ra,offset(sp) +* jradiussp - NOT SUPPORTED
The instruction is called JRADDIUSP, however it's irrelevant here as it's
an SP restore instruction. I think this reference can be removed.
arch/mips/kernel/process.c struct mips_frame_info { · line 231
Quoted 9 lines
arch/mips/kernel/process.c +9
-0+* +* microMIPS is way more fun... +*/ + return ((ip->mm16_r5_format.opcode == mm_swsp16_op && + ip->mm16_r5_format.rt == 31) || + (ip->mm16_m_format.opcode == mm_pool16c_op && + ip->mm16_m_format.func == mm_swm16_op) || + /* 32bit instruction */ + (ip->mm_m_format.opcode == mm_pool32b_op &&
This:
arch/mips/kernel/process.c struct mips_frame_info { · line 232
Quoted 1 line
arch/mips/kernel/process.c +1
-0+ ip->mm_m_format.rd > 9 &&
should be:
ip->mm_m_format.rd >= 16 &&
I believe -- codes 10-15 are reserved and from the current encoding it's clear it's bit #4 that denotes $ra.
arch/mips/kernel/process.c struct mips_frame_info { · line 235
Quoted 3 lines
arch/mips/kernel/process.c +3
-0+ ip->mm_m_format.base ==29 && + ip->mm_m_format.func == mm_swm32_func) || + (ip->i_format.opcode == mm_sw32_op &&
This should be:
(ip->mm_i_format.opcode == mm_sw32_op &&
I believe, and consequently this:
arch/mips/kernel/process.c struct mips_frame_info { · line 236
Quoted 1 line
arch/mips/kernel/process.c +1
-0+ ip->i_format.rs == 29 && ip->i_format.rt == 31));
should be:
ip->mm_i_format.rs == 29 && ip->mm_i_format.rt == 31));
-- note that rs (aka base) and rt are swapped between the standard MIPS and the microMIPS instruction format, so the above change is not even remotely cosmetical.
arch/mips/kernel/process.c struct mips_frame_info { · line 239
Quoted 3 lines
arch/mips/kernel/process.c +3
-0+ }else { +/* sw / sd $ra, offset($sp) */ + return (ip->i_format.opcode == sw_op || ip->i_format.opcode == sd_op) &&
Oversize line here.
Quoted 39 lines
arch/mips/kernel/process.c +2
-32+ ip->i_format.rs ==29 && + ip->i_format.rt ==31 ; } -#else -/* sw / sd $ra, offset($sp) */ -return (ip->i_format.opcode == sw_op || ip->i_format.opcode == sd_op) && - ip->i_format.rs ==29 && - ip->i_format.rt ==31 ; -#endif }static inline int is_jump_ins (union mips_instruction *ip) { -#ifdef CONFIG_CPU_MICROMIPS -/* -* jr16,jrc,jalr16,jalr16 -* jal -* jalr/jr,jalr.hb/jr.hb,jalrs,jalrs.hb -* jraddiusp - NOT SUPPORTED -* -* microMIPS is kind of more fun... -*/ -union mips_instruction mmi; - - mmi.word = (ip->halfword[0 ] <<16 ); - -if ((mmi.mm16_r5_format.opcode == mm_pool16c_op && - (mmi.mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op) || - ip->j_format.opcode == mm_jal32_op) -return 1 ; -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); -#else -if (ip->j_format.opcode == j_op) -return 1 ; -if (ip->j_format.opcode == jal_op) -return 1 ;
arch/mips/kernel/process.c struct mips_frame_info { · line 252
Quoted 10 lines
arch/mips/kernel/process.c +6
-4-if (ip->r_format.opcode != spec_op) -return 0 ; -return ip->r_format.func == jalr_op || ip->r_format.func == jr_op; -#endif +if (kernel_uses_mmips) { +/* +* jr16,jrc,jalr16,jalr16 +* jal +* jalr/jr,jalr.hb/jr.hb,jalrs,jalrs.hb +* jraddiusp - NOT SUPPORTED
I think that JRADDIUSP should really be supported here, GCC is pretty
keen on using this instruction where possible.
arch/mips/kernel/process.c struct mips_frame_info { · line 256
Quoted 4 lines
arch/mips/kernel/process.c +4
-0+* +* microMIPS is kind of more fun... +*/ + if ((ip->mm16_r5_format.opcode == mm_pool16c_op &&
This:
arch/mips/kernel/process.c struct mips_frame_info { · line 257
Quoted 1 line
arch/mips/kernel/process.c +1
-0+ (ip->mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op))
should be:
(ip->mm16_r5_format.rt & ~0x3) == mm_jr16_op))
I believe (and also mm_jr16_op is wrongly defined as 0x18 instead of 0xc; mm_jrc_op, mm_jalr16_op and mm_jalrs16_op are likewise wrong -- all need to be right-shifted by one bit).
arch/mips/kernel/process.c struct mips_frame_info { · line 260
Quoted 3 lines
arch/mips/kernel/process.c +3
-0+return 1 ; + + if (ip->j_format.opcode == mm_jal32_op ||
This:
ip->j_format.opcode == mm_jals32_op ||
is missing here.
arch/mips/kernel/process.c struct mips_frame_info { · line 266
Quoted 6 lines
arch/mips/kernel/process.c +6
-0+ ip->j_format.opcode == mm_j32_op) +return 1 ; + +if (ip->r_format.opcode != mm_pool32a_op || + ip->r_format.func != mm_pool32axf_op) +return 0 ;
I think this:
arch/mips/kernel/process.c struct mips_frame_info { · line 267
Quoted 1 line
arch/mips/kernel/process.c +1
-0+return (((ip->u_format.uimmediate >>6 ) & mm_jalr_op) == mm_jalr_op);
should be fully decoded (and oversize line fixed too):
return (((ip->u_format.uimmediate >> 6) & ~0x14) == mm_jalr_op);
arch/mips/kernel/process.c struct mips_frame_info { · line 275
Quoted 8 lines
arch/mips/kernel/process.c +8
-0+ }else { +if (ip->j_format.opcode == j_op) +return 1 ; +if (ip->j_format.opcode == jal_op) +return 1 ; +if (ip->r_format.opcode != spec_op) +return 0 ; +return ip->r_format.func == jalr_op || ip->r_format.func == jr_op;
Oversize line here.
Quoted 18 lines
arch/mips/kernel/process.c +1
-13+ } }static inline int is_sp_move_ins (union mips_instruction *ip) { -#ifdef CONFIG_CPU_MICROMIPS -/* -* addiusp -imm -* addius5 sp,-imm -* addiu32 sp,sp,-imm -* jradiussp - NOT SUPPORTED -* -* microMIPS is not more fun... -*/ -if (mm_insn_16bit (ip->halfword[0 ])) { -union mips_instruction mmi; - - mmi.word = (ip->halfword[0 ] <<16 );
arch/mips/kernel/process.c struct mips_frame_info { · line 286
Quoted 10 lines
arch/mips/kernel/process.c +6
-4-return ((mmi.mm16_r3_format.opcode == mm_pool16d_op && - mmi.mm16_r3_format.simmediate && mm_addiusp_func) || - (mmi.mm16_r5_format.opcode == mm_pool16d_op && - mmi.mm16_r5_format.rt ==29 )); +if (kernel_uses_mmips) { +/* +* addiusp -imm +* addius5 sp,-imm +* addiu32 sp,sp,-imm +* jradiussp - NOT SUPPORTED
Again, typo here, but should this instruction should be supported? It
can't have a negative immediate argument, it's a function return instruction, so I think the reference should be removed.
Quoted 18 lines
arch/mips/kernel/process.c +16
-1+* +* microMIPS is not more fun... +*/ +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 ) || +/* 32bit instruction */ + (ip->mm_i_format.opcode == mm_addiu32_op && + ip->mm_i_format.rt ==29 && ip->mm_i_format.rs ==29 )); + }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) +return 1 ; } - return (ip->mm_i_format.opcode == mm_addiu32_op &&
arch/mips/kernel/process.c struct mips_frame_info { · line 305
Quoted 10 lines
arch/mips/kernel/process.c +0
-8- ip->mm_i_format.rt == 29 && ip->mm_i_format.rs == 29); -#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) -return 1 ; -#endif return 0 ; }
I think for readability's sake it would make sense to rewrite these three
for a consistent return style, i.e. either "if (foo) return 1; if (bar) return 1;" etc. or "return foo || bar;" etc., and not a mixture of both.
Quoted 18 lines
arch/mips/kernel/process.c +18
-0+/* +* A few fun facts on MIPS16 +* +* 1. MIPS16 instructions are either 16-bit or 32-bit in length +* 2. MIPS16 32-bit instruction may not be on word boundary +* 3. MIPS16 instructions are placed in 32-bit memory element, in +* endian-dependent order. +* +* Big Endian +* hword[0] hword[1] hword[0] hword[1] +* +-------------+-------------+-------------+-------------+ +* | insn1 | insn2 | insn2' | insn3 | +* +-------------+-------------+-------------+-------------+ +* 31 word1 0 31 word2 0 +* +* Little Endian +* hword[1] hword[0] hword[1] hword[0] +* +-------------+-------------+-------------+-------------+
arch/mips/kernel/process.c struct mips_frame_info { · line 334
Quoted 10 lines
arch/mips/kernel/process.c +10
-0+* | insn2 | insn1 | insn3 | insn2' | +* +-------------+-------------+-------------+-------------+ +* 31 word1 0 31 word2 0 +* +* In MIPS16 mode, MIPS16_fetch_halfword does the followings: +* +* 1. fetch word from word-aligned address +* 2. access the fetched word using halfword (defeat endian issue) +* 3. assemble 16/32 bit MIPS16 instruction from halfword(s) +*/
So my note at the beginning also applies here. Please rewrite the
comment and reshape the figures so they refer to 16-bit halfwords rather than 32-bit words for compressed ISA modes.
arch/mips/kernel/process.c struct mips_frame_info { · line 335
Quoted 1 line
arch/mips/kernel/process.c +1
-0+static void MIPS16_fetch_halfword(union mips_instruction **ip,
Linux doesn't use capital letters in function names, and having my note
above in mind please make this mmips_fetch_halfword or suchlike.
Quoted 17 lines
arch/mips/kernel/process.c +17
-0+ unsigned short *this_halfword, + unsigned short *prev_halfword) +{ +if (*prev_halfword) { + *this_halfword = *prev_halfword; + *prev_halfword =0 ; + }else { +/* advance pointer to next word */ + *this_halfword = (*ip)->halfword[0 ]; + *prev_halfword = (*ip)->halfword[1 ]; + *ip +=1 ; + } +} + +static void fetch_instruction (union mips_instruction **ip, +union mips_instruction *mi, +unsigned short *prev_halfword)
arch/mips/kernel/process.c struct mips_frame_info { · line 362
Quoted 10 lines
arch/mips/kernel/process.c +10
-0+{ +if (kernel_uses_mmips) { +/* fetch the first MIPS16 instruction */ +MIPS16_fetch_halfword (ip, &mi->halfword[0 ], prev_halfword); + +/* fetch the second half if it is a 32bit one */ +if (mm_insn_16bit (mi->halfword[0 ])) + mi->halfword[1 ] =0 ; +else +MIPS16_fetch_halfword (ip, &mi->halfword[1 ], prev_halfword);
Oversize line here.
arch/mips/kernel/process.c struct mips_frame_info { · line 363
Quoted 2 lines
arch/mips/kernel/process.c +2
-0+ }else { +/* do simple assignment for mips32 mode */
I suggest "standard MIPS mode" or suchlike as we do support MIPS64
execution as well. Also I suggest formatting sentences properly, starting with a capital letter and ending with a full stop.
arch/mips/kernel/process.c struct mips_frame_info { · line 369
Quoted 5 lines
arch/mips/kernel/process.c +5
-0+ *mi = **ip; + *ip +=1 ; + } +} +
As I noted above, I think this is all overcomplicated. I suggest
rewriting it such that it operates on ip being an u16 pointer (or maybe a union of an u16 and an u32 pointer) and only returning the mips_instruction union. This will make all this shuffling and the use of the prev_halfword temporary in the upper frame unnecessary.
arch/mips/kernel/process.c static int get_frame_info(struct mips_frame_info *info) · line 389
Quoted 8 lines
arch/mips/kernel/process.c +3
-1@@ -340,37 +384,37 @@ static int get_frame_info(struct mips_frame_info *info) max_insns =128U ;/* unknown function size */ max_insns =min (128U , max_insns); -for (i =0 ; i < max_insns; i++, ip++) { +if (kernel_uses_mmips) { +/* align start address to word boundary, lose mode bit. */ + ip = (union mips_instruction *)((unsigned long )ip & ~0x3 );
Is it OK to decode possibly half an instruction back here?
There will be no need to do this with the approach I outlined above, and
also please use msk_isa16_mode(...) then.
arch/mips/kernel/process.c static int get_frame_info(struct mips_frame_info *info) · line 411
Quoted 9 lines
arch/mips/kernel/process.c +9
-0+unsigned short tmp; + +if (inst.halfword[0 ] & mm_addiusp_func) { + tmp = (((inst.halfword[0 ] >>1 ) &0x1ff ) <<2 ); + info->frame_size = -(signed short )(tmp | ((tmp &0x100 ) ?0xfe00 :0 )); + }else { + tmp = (inst.halfword[0 ] >>1 ); + info->frame_size = -(signed short )(tmp &0xf ); + }
The immediate is signed in both cases, you need to take care of this,
e.g.:
short tmp; [...] info->frame_size = 0x8 - ((tmp & 0xf) ^ 0x8);
and please observe the special decoding rules of the ADDIUSP instruction's immediate value (the 0, 1, 510 and 511 encoded cases).
Also oversize lines.
Quoted 11 lines
arch/mips/kernel/process.c +3
-1@@ -390,20 +434,36 @@ static unsigned long get___schedule_addr(void) {return kallsyms_lookup_name ("__schedule" ); } -#else +#else /* CONFIG_KALLSYMS */ static unsigned long get___schedule_addr (void ) {union mips_instruction *ip = (void *)schedule; +union mips_instruction inst; +union mips_instruction *max_ip;
arch/mips/kernel/process.c static unsigned long get___schedule_addr(void) · line 448
Quoted 10 lines
arch/mips/kernel/process.c +4
-4int max_insns =8 ; -int i; +unsigned short halfword =0 ; -for (i =0 ; i < max_insns; i++, ip++) { -if (ip->j_format.opcode == j_op) -return J_TARGET (ip, ip->j_format.target); +if (kernel_uses_mmips) { +/* align start address to word boundary, lose mode bit */ + ip = (union mips_instruction *)((unsigned long )ip & ~0x3 );
See the note on instruction decoding above here as well.
Also all corrections for the preexisting bugs noted above need to be
separate patches.
Maciej
Quoted 8 lines · Maciej W. Rozycki
I think this:
arch/mips/kernel/process.c struct mips_frame_info { · line 267
Quoted 1 line
arch/mips/kernel/process.c +1
-0+return (((ip->u_format.uimmediate >>6 ) & mm_jalr_op) == mm_jalr_op);should be fully decoded (and oversize line fixed too):
return (((ip->u_format.uimmediate >> 6) & ~0x14) == mm_jalr_op);
Self-correction here:
return (((ip->u_format.uimmediate >> 6) & ~0x140) == mm_jalr_op);
-- sorry about the mistake.
Maciej