| Message ID | 20241106184612.71897-5-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Mark architecture specific devices with specific endianness | expand |
On 11/6/24 18:46, Philippe Mathieu-Daudé wrote: > 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(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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.
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. r~
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>
diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c index 9fb63515ef..794c175bdb 100644 --- a/hw/openrisc/openrisc_sim.c +++ b/hw/openrisc/openrisc_sim.c @@ -266,7 +266,7 @@ static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base, } serial_mm_init(get_system_memory(), base, 0, serial_irq, 115200, serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1), - DEVICE_NATIVE_ENDIAN); + DEVICE_BIG_ENDIAN); /* Add device tree node for serial. */ nodename = g_strdup_printf("/serial@%" HWADDR_PRIx, base); 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);
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(-)