Messages in this thread | | | Date | Mon, 17 Oct 2022 17:19:18 +0800 | Subject | Re: [PATCH V2] LoongArch: Add unaligned access support | From | WANG Xuerui <> |
| |
On 2022/10/17 16:59, Rui Wang wrote: >> +void emulate_load_store_insn(struct pt_regs *regs, void __user *addr, unsigned int *pc) >> +{ >> + bool user = user_mode(regs); >> + unsigned int res; >> + unsigned long origpc; >> + unsigned long origra; >> + unsigned long value = 0; >> + union loongarch_instruction insn; >> + >> + origpc = (unsigned long)pc; >> + origra = regs->regs[1]; >> + >> + perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, 0); >> + >> + /* >> + * This load never faults. >> + */ >> + __get_inst(&insn.word, pc, user); >> + if (user && !access_ok(addr, 8)) >> + goto sigbus; >> + >> + if (insn.reg2i12_format.opcode == ldd_op || >> + insn.reg2i14_format.opcode == ldptrd_op || >> + insn.reg3_format.opcode == ldxd_op) { >> + res = unaligned_read(addr, &value, 8, 1); >> + if (res) >> + goto fault; >> + regs->regs[insn.reg2i12_format.rd] = value; >> + } else if (insn.reg2i12_format.opcode == ldw_op || >> + insn.reg2i14_format.opcode == ldptrw_op || >> + insn.reg3_format.opcode == ldxw_op) { >> + res = unaligned_read(addr, &value, 4, 1); >> + if (res) >> + goto fault; >> + regs->regs[insn.reg2i12_format.rd] = value; >> + } else if (insn.reg2i12_format.opcode == ldwu_op || >> + insn.reg3_format.opcode == ldxwu_op) { >> + res = unaligned_read(addr, &value, 4, 0); >> + if (res) >> + goto fault; >> + regs->regs[insn.reg2i12_format.rd] = value; >> + } else if (insn.reg2i12_format.opcode == ldh_op || >> + insn.reg3_format.opcode == ldxh_op) { >> + res = unaligned_read(addr, &value, 2, 1); >> + if (res) >> + goto fault; >> + regs->regs[insn.reg2i12_format.rd] = value; >> + } else if (insn.reg2i12_format.opcode == ldhu_op || >> + insn.reg3_format.opcode == ldxhu_op) { >> + res = unaligned_read(addr, &value, 2, 0); >> + if (res) >> + goto fault; >> + regs->regs[insn.reg2i12_format.rd] = value; >> + } else if (insn.reg2i12_format.opcode == std_op || >> + insn.reg2i14_format.opcode == stptrd_op || >> + insn.reg3_format.opcode == stxd_op) { >> + value = regs->regs[insn.reg2i12_format.rd]; >> + res = unaligned_write(addr, value, 8); >> + if (res) >> + goto fault; >> + } else if (insn.reg2i12_format.opcode == stw_op || >> + insn.reg2i14_format.opcode == stptrw_op || >> + insn.reg3_format.opcode == stxw_op) { >> + value = regs->regs[insn.reg2i12_format.rd]; >> + res = unaligned_write(addr, value, 4); >> + if (res) >> + goto fault; >> + } else if (insn.reg2i12_format.opcode == sth_op || >> + insn.reg3_format.opcode == stxh_op) { >> + value = regs->regs[insn.reg2i12_format.rd]; >> + res = unaligned_write(addr, value, 2); >> + if (res) >> + goto fault; >> + } else if (insn.reg2i12_format.opcode == fldd_op || >> + insn.reg3_format.opcode == fldxd_op) { >> + res = unaligned_read(addr, &value, 8, 1); >> + if (res) >> + goto fault; >> + write_fpr(insn.reg2i12_format.rd, value); >> + } else if (insn.reg2i12_format.opcode == flds_op || >> + insn.reg3_format.opcode == fldxs_op) { >> + res = unaligned_read(addr, &value, 4, 1); >> + if (res) >> + goto fault; >> + write_fpr(insn.reg2i12_format.rd, value); >> + } else if (insn.reg2i12_format.opcode == fstd_op || >> + insn.reg3_format.opcode == fstxd_op) { >> + value = read_fpr(insn.reg2i12_format.rd); >> + res = unaligned_write(addr, value, 8); >> + if (res) >> + goto fault; >> + } else if (insn.reg2i12_format.opcode == fsts_op || >> + insn.reg3_format.opcode == fstxs_op) { >> + value = read_fpr(insn.reg2i12_format.rd); >> + res = unaligned_write(addr, value, 4); >> + if (res) >> + goto fault; >> + } else >> + goto sigbus; > > So many condtional branches for linear instruction matching, use > switch case is better? > > 0000000000000238 <emulate_load_store_insn>: > ... > 35c: 1470180f lu12i.w $t3, 229568(0x380c0) > 360: 580141af beq $t1, $t3, 320(0x140) # 4a0 > <emulate_load_store_insn+0x268> > 364: 1451000f lu12i.w $t3, 165888(0x28800) > 368: 5801dd8f beq $t0, $t3, 476(0x1dc) # 544 > [snip]
The code structure here is basically several switches intermingled with each other, each matching opcodes for one insn format, but sharing much code once matched. Quite difficult to express with C without some kind of code duplication.
But we can try:
{ int size; bool is_read, is_signed;
switch (insn.reg2i12_format.opcode) { case ldd_op: size = 8; is_read = true; is_signed = true; break;
case std_op: size = 8; is_read = false; value = regs->regs[insn.reg2i12_format.rd]; break;
case ldw_op: size = 4; is_read = true; is_signed = true; break;
case ldwu_op: size = 4; is_read = true; is_signed = false; break;
/* other opcodes */
default: /* not 2RI12 */ break; }
switch (insn.reg2i14_format.opcode) { case ldptrd_op: size = 8; is_read = true; is_signed = true; break;
case ldptrw_op: size = 4; is_read = true; is_signed = true; break;
// ... }
// other formats
if (!size) { /* no match */ goto sigbus; }
if (is_read) res = unaligned_read(addr, &value, size, is_signed); else res = unaligned_write(addr, value, size);
if (res) goto fault;
// ... }
This way at least the data flow is clearer, and probably the codegen quality would benefit as well due to the clearer switch-case structures. (It's okay if this is not the case, it's not performance-critical anyway because if we ever hit this at all, performance would have already tanked.)
-- WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
| |