Re: [PATCH 4/5] hw/openrisc: Mark devices as big-endian

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

Hi Thomas,

On 9/11/24 06:42, Thomas Huth wrote:
> Am Wed,  6 Nov 2024 18:46:11 +0000
> schrieb Philippe Mathieu-Daudé <philmd@linaro.org>:
> 
>> These devices are only used by the OpenRISC target, which is
>> only built as big-endian. Therefore the DEVICE_NATIVE_ENDIAN
>> definition expand to DEVICE_BIG_ENDIAN (besides, the
>> DEVICE_LITTLE_ENDIAN case isn't tested). Simplify directly
>> using DEVICE_BIG_ENDIAN.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/openrisc/openrisc_sim.c | 2 +-
>>   hw/openrisc/virt.c         | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)


>> diff --git a/hw/openrisc/virt.c b/hw/openrisc/virt.c
>> index 47d2c9bd3c..ede57fe391 100644
>> --- a/hw/openrisc/virt.c
>> +++ b/hw/openrisc/virt.c
>> @@ -236,7 +236,7 @@ static void openrisc_virt_serial_init(OR1KVirtState *state, hwaddr base,
>>       qemu_irq serial_irq = get_per_cpu_irq(cpus, num_cpus, irq_pin);
>>   
>>       serial_mm_init(get_system_memory(), base, 0, serial_irq, 115200,
>> -                   serial_hd(0), DEVICE_NATIVE_ENDIAN);
>> +                   serial_hd(0), DEVICE_BIG_ENDIAN);
>>   
>>       /* Add device tree node for serial. */
>>       nodename = g_strdup_printf("/serial@%" HWADDR_PRIx, base);
> 
> According to https://openrisc.io/or1k.html the openrisc CPU could be
> implemented as little endian, too ... so would it make sense to use
> a runtime detected value here instead?

While this patch is a code change, it aims to not introduce any
functional change. We are not building (nor testing) these devices
in a little endian configuration:

$ git grep -l TARGET_BIG_ENDIAN configs/targets/*softmmu*
configs/targets/hppa-softmmu.mak
configs/targets/m68k-softmmu.mak
configs/targets/microblaze-softmmu.mak
configs/targets/mips-softmmu.mak
configs/targets/mips64-softmmu.mak
configs/targets/or1k-softmmu.mak
                 ^^^^
configs/targets/ppc-softmmu.mak
configs/targets/ppc64-softmmu.mak
configs/targets/s390x-softmmu.mak
configs/targets/sh4eb-softmmu.mak
configs/targets/sparc-softmmu.mak
configs/targets/sparc64-softmmu.mak
configs/targets/xtensaeb-softmmu.mak

$ git grep -L TARGET_BIG_ENDIAN configs/targets/*softmmu*
configs/targets/aarch64-softmmu.mak
configs/targets/alpha-softmmu.mak
configs/targets/arm-softmmu.mak
configs/targets/avr-softmmu.mak
configs/targets/i386-softmmu.mak
configs/targets/loongarch64-softmmu.mak
configs/targets/microblazeel-softmmu.mak
configs/targets/mips64el-softmmu.mak
configs/targets/mipsel-softmmu.mak
configs/targets/riscv32-softmmu.mak
configs/targets/riscv64-softmmu.mak
configs/targets/rx-softmmu.mak
configs/targets/sh4-softmmu.mak
configs/targets/tricore-softmmu.mak
configs/targets/x86_64-softmmu.mak
configs/targets/xtensa-softmmu.mak

(no little-endian config here)

Having little-endian OpenRISC is certainly welcomed, but it
has to be done separately, preferably adding test coverage.

Should I rework the commit description to be more precise?

Regards,

Phil.