1. Patchew
  2. QEMU
  3. [PATCH 0/5] hw: Mark architecture specific devices with specific endianness
  4. View patch

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

Philippe Mathieu-Daudé posted 5 patches 2 weeks, 3 days ago
[PATCH 4/5] hw/openrisc: Mark devices as big-endian
Posted by Philippe Mathieu-Daudé 2 weeks, 3 days ago 
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/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);
-- 
2.45.2
Re: [PATCH 4/5] hw/openrisc: Mark devices as big-endian
Posted by Thomas Huth 2 weeks ago 
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/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);

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?

 Thomas
Re: [PATCH 4/5] hw/openrisc: Mark devices as big-endian
Posted by Philippe Mathieu-Daudé 2 weeks ago 
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.
Re: [PATCH 4/5] hw/openrisc: Mark devices as big-endian
Posted by Richard Henderson 2 weeks ago 
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~
Re: [PATCH 4/5] hw/openrisc: Mark devices as big-endian
Posted by Thomas Huth 2 weeks ago 
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>
Re: [PATCH 4/5] hw/openrisc: Mark devices as big-endian
Posted by Richard Henderson 2 weeks, 2 days ago 
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~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.