Re: [PATCH V2] LoongArch: Add unaligned access support

[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/