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

[Thomas Huth <huth@tuxfamily.org>]

Am Sat, 9 Nov 2024 10:08:16 -0800
schrieb Richard Henderson <richard.henderson@linaro.org>:

> On 11/9/24 07:58, Philippe Mathieu-Daudé wrote:
> > 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
> >                  ^^^^  
> 
> The openrisc little-endian control is in a control register: SR[LEE] (which we do not 
> implement at present).
> 
> So any openrisc little-endian support would look like qemu-system-ppc64.  I would not 
> expect devices to switch endianness at all.

Ok, thanks, in that case, I think the patch is fine.

FWIW:
Reviewed-by: Thomas Huth <huth@tuxfamily.org>