Re: [PATCH] target/riscv: Fix endianness swap on compressed instructions

[Ben Dooks <ben.dooks@codethink.co.uk>]

On 30/09/2025 07:59, Heinrich Schuchardt wrote:
> On 9/29/25 16:37, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>>
>> On 29/9/25 13:55, Valentin Haudiquet wrote:
>>> From: vhaudiquet <vhaudiquet343@hotmail.fr>
>>>
>>> Three instructions were not using the endianness swap flag, which 
>>> resulted in a bug on big-endian architectures.
>>
>> I suppose you mean "big-endian host architectures".
>> If so, please clarify.
> 
> Hello Phil,
> 
> Ubuntu is providing QEMU to emulate RISC-V both on little-endian hosts 
> like X86 and ARM as well as on big-endian hosts like the IBM S/390.
> 
> The Unprivileged RISC-V ISA Specification has this sentence:
> 
> "The base ISA has been defined to have a little-endian memory system, 
> with big-endian or bi-endian as non-standard variants."
> 
> According to the Privileged RISC-V ISA Specification the mstatus and 
> mstatush register may be used to set the endianness at runtime.
> 
> "The MBE, SBE, and UBE bits in mstatus and mstatush are WARL fields that 
> control the endianness of memory accesses other than instruction 
> fetches. Instruction fetches are always little-endian."
> 
> Currently RISC-V work focuses on little-endian targets but there is 
> active community work to enable big-endian Linux for RISC-V.
> 
> Therefore a solution is required that considers both the host and the 
> target endianness.
> 
>>
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3131
>>> Buglink: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/2123828
>>>
>>> Signed-off-by: Valentin Haudiquet <valentin.haudiquet@canonical.com>
>>> ---
>>>   target/riscv/insn_trans/trans_rvzce.c.inc | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/riscv/insn_trans/trans_rvzce.c.inc b/target/ 
>>> riscv/ insn_trans/trans_rvzce.c.inc
>>> index c77c2b927b..dd15af0f54 100644
>>> --- a/target/riscv/insn_trans/trans_rvzce.c.inc
>>> +++ b/target/riscv/insn_trans/trans_rvzce.c.inc
>>> @@ -88,13 +88,13 @@ static bool trans_c_lbu(DisasContext *ctx, 
>>> arg_c_lbu *a)
>>>   static bool trans_c_lhu(DisasContext *ctx, arg_c_lhu *a)
>>>   {
>>>       REQUIRE_ZCB(ctx);
>>> -    return gen_load(ctx, a, MO_UW);
>>> +    return gen_load(ctx, a, MO_TEUW);
>> NAck.
>> Please do not use MO_TE* anymore. Use explicit endianness.
>>
>> So far all our RISC-V targets are little-endian:
>>
>>    $ git grep TARGET_BIG_ENDIAN configs/targets/riscv*
>>    $
>>
>> If you are not worried about RISCV core running in BE mode
>> (as we currently don't check MSTATUS_[USM]BE bits), your change
>> should be:
>>
>>   -    return gen_load(ctx, a, MO_UW);
>>   +    return gen_load(ctx, a, MO_UW | MO_LE);
> 
> I saw your patches to remove MO_TE from little-endian only targets like 
> i386 but RISC-V is different.
> 
> We should foresee that in future RISC-V targets run in either little- 
> endian or big-endian mode and implement our code accordingly.
> 
> When big-endian RISC-V comes upon QEMU, we should avoid duplicating the 
> target code but reuse what we have.
> 
> MO_UW | MO_LE looks wrong in this context.
> 
> We should stay consistent with
> 
> target/riscv/insn_trans/trans_rvi.c.inc
> target/riscv/insn_trans/trans_rvzfh.c.inc
> target/riscv/insn_trans/trans_xthead.c.inc
> 
> which currently use MO_TEUW.
> 
> For the time being, I would suggest to stick to MO_TE* to maintain the 
> information in which code locations we need to consider the target 
> endianness.
> 
> Targets that can switch endianness at runtime (e.g. MIPS) use an 
> architecture specific implemenation of mo_endian(ctx). When implementing 
> big-endian RISC-V support in future, we can use the MO_TE occurrences as 
> indicator where to use mo_endian() instead.
> 
> With these considerations in mind the current patch looks good to me.
> 
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

You've reminded me that we still haven't managed to get movement on
the big-endian runtime patches.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html