Re: [PATCH v5 1/2] tcg/mips: Support unaligned access for user-only

[Philippe Mathieu-Daudé]

On 2/2/22 00:41, Richard Henderson wrote:
> This is kinda sorta the opposite of the other tcg hosts, where
> we get (normal) alignment checks for free with host SIGBUS and
> need to add code to support unaligned accesses.
>
> Fortunately, the ISA contains pairs of instructions that are
> used to implement unaligned memory accesses.  Use them.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tcg/mips/tcg-target.h     |   2 -
>   tcg/mips/tcg-target.c.inc | 334 +++++++++++++++++++++++++++++++++++++-
>   2 files changed, 328 insertions(+), 8 deletions(-)

> +static void __attribute__((unused))
> +tcg_out_qemu_ld_unalign(TCGContext *s, TCGReg lo, TCGReg hi,
> +                                    TCGReg base, MemOp opc, bool is_64)

Unaligned style to honor the function name? ;)

> +static void __attribute__((unused))
> +tcg_out_qemu_st_unalign(TCGContext *s, TCGReg lo, TCGReg hi,
> +                                    TCGReg base, MemOp opc)

Ditto.

> +    case MO_64 | MO_BSWAP:
> +        if (TCG_TARGET_REG_BITS == 64) {
> +            tcg_out_bswap64(s, TCG_TMP3, lo);
> +            lo = TCG_TMP3;
> +        } else if (use_mips32r2_instructions) {
> +            tcg_out_opc_reg(s, OPC_WSBH, TCG_TMP0, 0, MIPS_BE ? hi : lo);
> +            tcg_out_opc_reg(s, OPC_WSBH, TCG_TMP1, 0, MIPS_BE ? lo : hi);
> +            tcg_out_opc_sa(s, OPC_ROTR, TCG_TMP0, TCG_TMP0, 16);
> +            tcg_out_opc_sa(s, OPC_ROTR, TCG_TMP1, TCG_TMP1, 16);
> +            hi = MIPS_BE ? TCG_TMP0 : TCG_TMP1;
> +            lo = MIPS_BE ? TCG_TMP1 : TCG_TMP0;
> +        } else {
> +            tcg_out_bswap32(s, TCG_TMP3, MIPS_BE ? lo : hi, 0);
> +            tcg_out_opc_imm(s, sw1, TCG_TMP3, base, 0);
> +            tcg_out_opc_imm(s, sw2, TCG_TMP3, base, 3);

I'd keep the parity with ld_unalign and use 0 + 0, 0 + 3, ...

> +            tcg_out_bswap32(s, TCG_TMP3, MIPS_BE ? hi : lo, 0);
> +            tcg_out_opc_imm(s, sw1, TCG_TMP3, base, 4);
> +            tcg_out_opc_imm(s, sw2, TCG_TMP3, base, 7);

4 + 0, 4 + 3, ...

> +            break;
> +        }
> +        /* fall through */
> +    case MO_64:
> +        if (TCG_TARGET_REG_BITS == 64) {
> +            tcg_out_opc_imm(s, sd1, lo, base, 0);
> +            tcg_out_opc_imm(s, sd2, lo, base, 7);

Ditto, ...
> +        } else {
> +            tcg_out_opc_imm(s, sw1, MIPS_BE ? hi : lo, base, 0);
> +            tcg_out_opc_imm(s, sw2, MIPS_BE ? hi : lo, base, 3);
> +            tcg_out_opc_imm(s, sw1, MIPS_BE ? lo : hi, base, 4);
> +            tcg_out_opc_imm(s, sw2, MIPS_BE ? lo : hi, base, 7);

Ditto.

> +        }
> +        break;
> +
> +    default:
> +        tcg_abort();
> +    }
> +}

Beside the nitpicking comments, nothing to say, hardcore!
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>