Re: [PATCH 01/13] target/riscv: Really use little endianness for 128-bit loads/stores

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Richard,

[Dropping Fabien Portas from Cc due to bouncing email]

On 10/10/25 20:44, Richard Henderson wrote:
> On 10/10/25 08:50, Philippe Mathieu-Daudé wrote:
>> Per commit a2f827ff4f4 ("target/riscv: accessors to registers upper
>> part and 128-bit load/store") description:
>>
>>   > The 128-bit ISA adds ldu, lq and sq. We provide support for these
>>   > instructions. Note that (a) we compute only 64-bit addresses to
>>   > actually access memory, cowardly utilizing the existing address
>>   > translation mechanism of QEMU, and (b) we assume for now
>>   > little-endian memory accesses.
>>
>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> However this commit used MO_TE (target endianness) for the
>> gen_load_i128() and gen_store_i128() helpers. Likely it was
>> unnoticed because current targets are only built using little
>> endianness:
>>
>>    $ git grep -L TARGET_BIG_ENDIAN=y configs/targets/riscv*
>>    configs/targets/riscv32-linux-user.mak
>>    configs/targets/riscv32-softmmu.mak
>>    configs/targets/riscv64-bsd-user.mak
>>    configs/targets/riscv64-linux-user.mak
>>    configs/targets/riscv64-softmmu.mak
>>
>> Replace by MO_TE -> MO_LE to really use little endianness.
>>
>> Cc: Fabien Portas <fabien.portas@grenoble-inp.org>
>> Cc: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
>> Fixes: a2f827ff4f4 ("target/riscv: accessors to registers upper part 
>> and 128-bit load/store")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/riscv/insn_trans/trans_rvi.c.inc | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/ 
>> insn_trans/trans_rvi.c.inc
>> index b9c71604687..df0b555176a 100644
>> --- a/target/riscv/insn_trans/trans_rvi.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
>> @@ -389,9 +389,11 @@ static bool gen_load_i128(DisasContext *ctx, 
>> arg_lb *a, MemOp memop)
>>           }
>>       } else {
>>           /* assume little-endian memory access for now */
>> -        tcg_gen_qemu_ld_tl(destl, addrl, ctx->mem_idx, MO_TEUQ);
>> +        MemOp memop = MO_LEUQ;
>> +
>> +        tcg_gen_qemu_ld_tl(destl, addrl, ctx->mem_idx, memop);
>>           tcg_gen_addi_tl(addrl, addrl, 8);
>> -        tcg_gen_qemu_ld_tl(desth, addrl, ctx->mem_idx, MO_TEUQ);
>> +        tcg_gen_qemu_ld_tl(desth, addrl, ctx->mem_idx, memop);
>>       }
>>       gen_set_gpr128(ctx, a->rd, destl, desth);
>> @@ -494,9 +496,11 @@ static bool gen_store_i128(DisasContext *ctx, 
>> arg_sb *a, MemOp memop)
>>           tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, memop);
>>       } else {
>>           /* little-endian memory access assumed for now */
>> -        tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, MO_TEUQ);
>> +        MemOp memop = MO_LEUQ;
>> +
>> +        tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, memop);
>>           tcg_gen_addi_tl(addrl, addrl, 8);
>> -        tcg_gen_qemu_st_tl(src2h, addrl, ctx->mem_idx, MO_TEUQ);
>> +        tcg_gen_qemu_st_tl(src2h, addrl, ctx->mem_idx, memop);
>>       }
>>       return true;
>>   }
> 
> We fix this to use tcg_gen_qemu_{ld,st}_i128.

IIUC the target doesn't use TCGv_i128 but a pair of TCGv_i64
named cpu_gpr[] and cpu_gprh[] (i.e. see how gen_set_gpr128()
set them). Using tcg_gen_qemu_{ld,st}_i128() makes sense but
implies a huge internals rewrite, which 1/ I'm not willing to
do without tests and 2/ I won't have time to do because of
priorities. Could we proceed with this simple patch meanwhile?

Regards,

Phil.