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

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

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@xxxxxxxxxx>:
>
> > 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@xxxxxxxxxx>
> > ---
> >   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.