[PATCH 0/3] hw/mips/jazz: Rework the NIC init code

[PATCH 0/3] hw/mips/jazz: Rework the NIC init code

[Thomas Huth]

The NIC init code of the jazz machines is rather cumbersome, with
a for-loop around it that is always left after the first iteration.
This patch series reworks this a little bit to make the code more
readable and shorter.

Thomas Huth (3):
  hw/mips/jazz: Remove the big_endian variable
  hw/mips/jazz: Move the NIC init code into a separate function
  hw/mips/jazz: Simplify the NIC setup code

 hw/mips/jazz.c | 89 +++++++++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 49 deletions(-)

-- 
2.39.3

[PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Thomas Huth]

There is an easier way to get a value that can be used to decide
whether the target is big endian or not: Simply use the
target_words_bigendian() function instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/mips/jazz.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index ca4426a92c..358bb6f74f 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -125,7 +125,7 @@ static void mips_jazz_init(MachineState *machine,
 {
     MemoryRegion *address_space = get_system_memory();
     char *filename;
-    int bios_size, n, big_endian;
+    int bios_size, n;
     Clock *cpuclk;
     MIPSCPU *cpu;
     MIPSCPUClass *mcc;
@@ -157,12 +157,6 @@ static void mips_jazz_init(MachineState *machine,
         [JAZZ_PICA61] = {33333333, 4},
     };
 
-#if TARGET_BIG_ENDIAN
-    big_endian = 1;
-#else
-    big_endian = 0;
-#endif
-
     if (machine->ram_size > 256 * MiB) {
         error_report("RAM size more than 256Mb is not supported");
         exit(EXIT_FAILURE);
@@ -301,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
             dev = qdev_new("dp8393x");
             qdev_set_nic_properties(dev, nd);
             qdev_prop_set_uint8(dev, "it_shift", 2);
-            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
+            qdev_prop_set_bit(dev, "big_endian", target_words_bigendian());
             object_property_set_link(OBJECT(dev), "dma_mr",
                                      OBJECT(rc4030_dma_mr), &error_abort);
             sysbus = SYS_BUS_DEVICE(dev);
-- 
2.39.3

[PATCH 2/3] hw/mips/jazz: Move the NIC init code into a separate function

[Thomas Huth]

The mips_jazz_init() function is already quite big, so moving
away some code here can help to make it more understandable.
Additionally, by moving this code into a separate function, the
next patch (that will refactor the for-loop around the NIC init
code) will be much shorter and easier to understand.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/mips/jazz.c | 62 ++++++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 358bb6f74f..a95a1bd743 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -114,6 +114,40 @@ static const MemoryRegionOps dma_dummy_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void mips_jazz_init_net(NICInfo *nd, IOMMUMemoryRegion *rc4030_dma_mr,
+                               DeviceState *rc4030, MemoryRegion *dp8393x_prom)
+{
+    DeviceState *dev;
+    SysBusDevice *sysbus;
+    int checksum, i;
+    uint8_t *prom;
+
+    qemu_check_nic_model(nd, "dp83932");
+
+    dev = qdev_new("dp8393x");
+    qdev_set_nic_properties(dev, nd);
+    qdev_prop_set_uint8(dev, "it_shift", 2);
+    qdev_prop_set_bit(dev, "big_endian", target_words_bigendian());
+    object_property_set_link(OBJECT(dev), "dma_mr",
+                             OBJECT(rc4030_dma_mr), &error_abort);
+    sysbus = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(sysbus, &error_fatal);
+    sysbus_mmio_map(sysbus, 0, 0x80001000);
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 4));
+
+    /* Add MAC address with valid checksum to PROM */
+    prom = memory_region_get_ram_ptr(dp8393x_prom);
+    checksum = 0;
+    for (i = 0; i < 6; i++) {
+        prom[i] = nd->macaddr.a[i];
+        checksum += prom[i];
+        if (checksum > 0xff) {
+            checksum = (checksum + 1) & 0xff;
+        }
+    }
+    prom[7] = 0xff - checksum;
+}
+
 #define MAGNUM_BIOS_SIZE_MAX 0x7e000
 #define MAGNUM_BIOS_SIZE                                                       \
         (BIOS_SIZE < MAGNUM_BIOS_SIZE_MAX ? BIOS_SIZE : MAGNUM_BIOS_SIZE_MAX)
@@ -287,33 +321,7 @@ static void mips_jazz_init(MachineState *machine,
             nd->model = g_strdup("dp83932");
         }
         if (strcmp(nd->model, "dp83932") == 0) {
-            int checksum, i;
-            uint8_t *prom;
-
-            qemu_check_nic_model(nd, "dp83932");
-
-            dev = qdev_new("dp8393x");
-            qdev_set_nic_properties(dev, nd);
-            qdev_prop_set_uint8(dev, "it_shift", 2);
-            qdev_prop_set_bit(dev, "big_endian", target_words_bigendian());
-            object_property_set_link(OBJECT(dev), "dma_mr",
-                                     OBJECT(rc4030_dma_mr), &error_abort);
-            sysbus = SYS_BUS_DEVICE(dev);
-            sysbus_realize_and_unref(sysbus, &error_fatal);
-            sysbus_mmio_map(sysbus, 0, 0x80001000);
-            sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 4));
-
-            /* Add MAC address with valid checksum to PROM */
-            prom = memory_region_get_ram_ptr(dp8393x_prom);
-            checksum = 0;
-            for (i = 0; i < 6; i++) {
-                prom[i] = nd->macaddr.a[i];
-                checksum += prom[i];
-                if (checksum > 0xff) {
-                    checksum = (checksum + 1) & 0xff;
-                }
-            }
-            prom[7] = 0xff - checksum;
+            mips_jazz_init_net(nd, rc4030_dma_mr, rc4030, dp8393x_prom);
             break;
         } else if (is_help_option(nd->model)) {
             error_report("Supported NICs: dp83932");
-- 
2.39.3

[PATCH 3/3] hw/mips/jazz: Simplify the NIC setup code

[Thomas Huth]

The for-loop does not make much sense here - it is always left after
the first iteration, so we can also check for nb_nics == 1 instead
which is way easier to understand.

Also, the checks for nd->model are superfluous since the code in
mips_jazz_init_net() calls qemu_check_nic_model() that already
takes care of this (i.e. initializing nd->model if it has not been
set yet, and checking whether it is the "help" option or the
supported NIC model).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/mips/jazz.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index a95a1bd743..86cd1d2fb2 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -172,7 +172,6 @@ static void mips_jazz_init(MachineState *machine,
     MemoryRegion *rtc = g_new(MemoryRegion, 1);
     MemoryRegion *dma_dummy = g_new(MemoryRegion, 1);
     MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
-    NICInfo *nd;
     DeviceState *dev, *rc4030;
     MMIOKBDState *i8042;
     SysBusDevice *sysbus;
@@ -315,21 +314,11 @@ static void mips_jazz_init(MachineState *machine,
     }
 
     /* Network controller */
-    for (n = 0; n < nb_nics; n++) {
-        nd = &nd_table[n];
-        if (!nd->model) {
-            nd->model = g_strdup("dp83932");
-        }
-        if (strcmp(nd->model, "dp83932") == 0) {
-            mips_jazz_init_net(nd, rc4030_dma_mr, rc4030, dp8393x_prom);
-            break;
-        } else if (is_help_option(nd->model)) {
-            error_report("Supported NICs: dp83932");
-            exit(1);
-        } else {
-            error_report("Unsupported NIC: %s", nd->model);
-            exit(1);
-        }
+    if (nb_nics == 1) {
+        mips_jazz_init_net(&nd_table[0], rc4030_dma_mr, rc4030, dp8393x_prom);
+    } else if (nb_nics > 1) {
+        error_report("This machine only supports one NIC");
+        exit(1);
     }
 
     /* SCSI adapter */
-- 
2.39.3

Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Richard Henderson]

On 8/25/23 10:51, Thomas Huth wrote:
> There is an easier way to get a value that can be used to decide
> whether the target is big endian or not: Simply use the
> target_words_bigendian() function instead.
> 
> Signed-off-by: Thomas Huth<thuth@redhat.com>
> ---
>   hw/mips/jazz.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Philippe Mathieu-Daudé]

Hi Thomas,

On 25/8/23 19:51, Thomas Huth wrote:
> There is an easier way to get a value that can be used to decide
> whether the target is big endian or not: Simply use the
> target_words_bigendian() function instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/mips/jazz.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)


> @@ -157,12 +157,6 @@ static void mips_jazz_init(MachineState *machine,
>           [JAZZ_PICA61] = {33333333, 4},
>       };
>   
> -#if TARGET_BIG_ENDIAN
> -    big_endian = 1;
> -#else
> -    big_endian = 0;
> -#endif
> -
>       if (machine->ram_size > 256 * MiB) {
>           error_report("RAM size more than 256Mb is not supported");
>           exit(EXIT_FAILURE);
> @@ -301,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
>               dev = qdev_new("dp8393x");
>               qdev_set_nic_properties(dev, nd);
>               qdev_prop_set_uint8(dev, "it_shift", 2);
> -            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
> +            qdev_prop_set_bit(dev, "big_endian", target_words_bigendian());

IIRC last time I tried that Peter pointed me at the documentation:

/**
  * target_words_bigendian:
  * Returns true if the (default) endianness of the target is big endian,
  * false otherwise. Note that in target-specific code, you can use
  * TARGET_BIG_ENDIAN directly instead. On the other hand, common
  * code should normally never need to know about the endianness of the
  * target, so please do *not* use this function unless you know very
  * well what you are doing!
  */

(Commit c95ac10340 "cpu: Provide a proper prototype for
  target_words_bigendian() in a header")

Should we update the comment?

Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Thomas Huth]

On 28/08/2023 14.19, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 25/8/23 19:51, Thomas Huth wrote:
>> There is an easier way to get a value that can be used to decide
>> whether the target is big endian or not: Simply use the
>> target_words_bigendian() function instead.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/mips/jazz.c | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> 
>> @@ -157,12 +157,6 @@ static void mips_jazz_init(MachineState *machine,
>>           [JAZZ_PICA61] = {33333333, 4},
>>       };
>> -#if TARGET_BIG_ENDIAN
>> -    big_endian = 1;
>> -#else
>> -    big_endian = 0;
>> -#endif
>> -
>>       if (machine->ram_size > 256 * MiB) {
>>           error_report("RAM size more than 256Mb is not supported");
>>           exit(EXIT_FAILURE);
>> @@ -301,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
>>               dev = qdev_new("dp8393x");
>>               qdev_set_nic_properties(dev, nd);
>>               qdev_prop_set_uint8(dev, "it_shift", 2);
>> -            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>> +            qdev_prop_set_bit(dev, "big_endian", target_words_bigendian());
> 
> IIRC last time I tried that Peter pointed me at the documentation:
> 
> /**
>   * target_words_bigendian:
>   * Returns true if the (default) endianness of the target is big endian,
>   * false otherwise. Note that in target-specific code, you can use
>   * TARGET_BIG_ENDIAN directly instead. On the other hand, common
>   * code should normally never need to know about the endianness of the
>   * target, so please do *not* use this function unless you know very
>   * well what you are doing!
>   */
> 
> (Commit c95ac10340 "cpu: Provide a proper prototype for
>   target_words_bigendian() in a header")
> 
> Should we update the comment?

What would you change? My motivation here was mainly to decrease the size of 
the code - I think it's way more complicated via the #if + extra variable 
compared to simply calling target_words_bigendian(), isn't it? I think the 
diffstat says it all...

  Thomas

Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Philippe Mathieu-Daudé]

On 28/8/23 14:41, Thomas Huth wrote:
> On 28/08/2023 14.19, Philippe Mathieu-Daudé wrote:
>> Hi Thomas,
>>
>> On 25/8/23 19:51, Thomas Huth wrote:
>>> There is an easier way to get a value that can be used to decide
>>> whether the target is big endian or not: Simply use the
>>> target_words_bigendian() function instead.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/mips/jazz.c | 10 ++--------
>>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>>
>>> @@ -157,12 +157,6 @@ static void mips_jazz_init(MachineState *machine,
>>>           [JAZZ_PICA61] = {33333333, 4},
>>>       };
>>> -#if TARGET_BIG_ENDIAN
>>> -    big_endian = 1;
>>> -#else
>>> -    big_endian = 0;
>>> -#endif
>>> -
>>>       if (machine->ram_size > 256 * MiB) {
>>>           error_report("RAM size more than 256Mb is not supported");
>>>           exit(EXIT_FAILURE);
>>> @@ -301,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
>>>               dev = qdev_new("dp8393x");
>>>               qdev_set_nic_properties(dev, nd);
>>>               qdev_prop_set_uint8(dev, "it_shift", 2);
>>> -            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>>> +            qdev_prop_set_bit(dev, "big_endian", 
>>> target_words_bigendian());
>>
>> IIRC last time I tried that Peter pointed me at the documentation:
>>
>> /**
>>   * target_words_bigendian:
>>   * Returns true if the (default) endianness of the target is big endian,
>>   * false otherwise. Note that in target-specific code, you can use
>>   * TARGET_BIG_ENDIAN directly instead. On the other hand, common
>>   * code should normally never need to know about the endianness of the
>>   * target, so please do *not* use this function unless you know very
>>   * well what you are doing!
>>   */
>>
>> (Commit c95ac10340 "cpu: Provide a proper prototype for
>>   target_words_bigendian() in a header")
>>
>> Should we update the comment?
> 
> What would you change? My motivation here was mainly to decrease the 
> size of the code - I think it's way more complicated via the #if + extra 
> variable compared to simply calling target_words_bigendian(), isn't it? 
> I think the diffstat says it all...

Is the comment misleading then? Why not decrease the code
size using target_words_bigendian() in all the similar cases?

$ git grep -A4 'if TARGET_BIG_ENDIAN' hw/

hw/microblaze/boot.c:145:#if TARGET_BIG_ENDIAN
hw/microblaze/boot.c-146-        big_endian = 1;
hw/microblaze/boot.c-147-#endif
--
hw/mips/jazz.c:160:#if TARGET_BIG_ENDIAN
hw/mips/jazz.c-161-    big_endian = 1;
hw/mips/jazz.c-162-#else
hw/mips/jazz.c-163-    big_endian = 0;
hw/mips/jazz.c-164-#endif
--
hw/mips/malta.c:378:#if TARGET_BIG_ENDIAN
hw/mips/malta.c-379-        val = 0x00000012;
hw/mips/malta.c-380-#else
hw/mips/malta.c-381-        val = 0x00000010;
hw/mips/malta.c-382-#endif
--
hw/mips/malta.c:631:#if TARGET_BIG_ENDIAN
hw/mips/malta.c-632-#define cpu_to_gt32(x) (x)
hw/mips/malta.c-633-#else
hw/mips/malta.c-634-#define cpu_to_gt32(x) bswap32(x)
hw/mips/malta.c-635-#endif
--
hw/mips/malta.c:881:#if TARGET_BIG_ENDIAN
hw/mips/malta.c-882-    big_endian = 1;
hw/mips/malta.c-883-#else
hw/mips/malta.c-884-    big_endian = 0;
hw/mips/malta.c-885-#endif
--
hw/mips/malta.c:1147:#if TARGET_BIG_ENDIAN
hw/mips/malta.c-1148-    be = 1;
hw/mips/malta.c-1149-#else
hw/mips/malta.c-1150-    be = 0;
hw/mips/malta.c-1151-#endif
--
hw/mips/mipssim.c:67:#if TARGET_BIG_ENDIAN
hw/mips/mipssim.c-68-    big_endian = 1;
hw/mips/mipssim.c-69-#else
hw/mips/mipssim.c-70-    big_endian = 0;
hw/mips/mipssim.c-71-#endif
--
hw/nios2/boot.c:153:#if TARGET_BIG_ENDIAN
hw/nios2/boot.c-154-        big_endian = 1;
hw/nios2/boot.c-155-#endif
--
hw/xtensa/sim.c:99:#if TARGET_BIG_ENDIAN
hw/xtensa/sim.c-100-    int big_endian = true;
hw/xtensa/sim.c-101-#else
hw/xtensa/sim.c-102-    int big_endian = false;
hw/xtensa/sim.c-103-#endif
--
hw/xtensa/xtfpga.c:222:#if TARGET_BIG_ENDIAN
hw/xtensa/xtfpga.c-223-    int be = 1;
hw/xtensa/xtfpga.c-224-#else
hw/xtensa/xtfpga.c-225-    int be = 0;
hw/xtensa/xtfpga.c-226-#endif

I'm just trying to be consistent. HW devices should be target
agnostic, thus not use anything related to target endianness
(TARGET_BIG_ENDIAN nor target_words_bigendian).

Machines know about their target endianness, and can propagate
that knowledge when creating their devices. Therefore using
TARGET_BIG_ENDIAN / target_words_bigendian is accepted there.
If TARGET_BIG_ENDIAN is too verbose, then let's use
target_words_bigendian() in all machines. That said, if we
use target_words_bigendian() in machine files, then some of
these files can be moved from specific_ss[] to system_ss[].

So within hw/ I'd restrict target_words_bigendian() use to
MachineClass::init() handlers, and prohibit TARGET_BIG_ENDIAN
from hw/. Only use in softmmu/, target, *-user/. If we agree
we can rewrite the comment, removing the "do *not* use this
function unless you know very well what you are doing!" which
is hard to interpret IMHO.

Regards,

Phil.

Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Thomas Huth]

On 28/08/2023 17.48, Philippe Mathieu-Daudé wrote:
> On 28/8/23 14:41, Thomas Huth wrote:
>> On 28/08/2023 14.19, Philippe Mathieu-Daudé wrote:
>>> Hi Thomas,
>>>
>>> On 25/8/23 19:51, Thomas Huth wrote:
>>>> There is an easier way to get a value that can be used to decide
>>>> whether the target is big endian or not: Simply use the
>>>> target_words_bigendian() function instead.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   hw/mips/jazz.c | 10 ++--------
>>>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>>
>>>
>>>> @@ -157,12 +157,6 @@ static void mips_jazz_init(MachineState *machine,
>>>>           [JAZZ_PICA61] = {33333333, 4},
>>>>       };
>>>> -#if TARGET_BIG_ENDIAN
>>>> -    big_endian = 1;
>>>> -#else
>>>> -    big_endian = 0;
>>>> -#endif
>>>> -
>>>>       if (machine->ram_size > 256 * MiB) {
>>>>           error_report("RAM size more than 256Mb is not supported");
>>>>           exit(EXIT_FAILURE);
>>>> @@ -301,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
>>>>               dev = qdev_new("dp8393x");
>>>>               qdev_set_nic_properties(dev, nd);
>>>>               qdev_prop_set_uint8(dev, "it_shift", 2);
>>>> -            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>>>> +            qdev_prop_set_bit(dev, "big_endian", 
>>>> target_words_bigendian());
>>>
>>> IIRC last time I tried that Peter pointed me at the documentation:
>>>
>>> /**
>>>   * target_words_bigendian:
>>>   * Returns true if the (default) endianness of the target is big endian,
>>>   * false otherwise. Note that in target-specific code, you can use
>>>   * TARGET_BIG_ENDIAN directly instead. On the other hand, common
>>>   * code should normally never need to know about the endianness of the
>>>   * target, so please do *not* use this function unless you know very
>>>   * well what you are doing!
>>>   */
>>>
>>> (Commit c95ac10340 "cpu: Provide a proper prototype for
>>>   target_words_bigendian() in a header")
>>>
>>> Should we update the comment?
>>
>> What would you change? My motivation here was mainly to decrease the size 
>> of the code - I think it's way more complicated via the #if + extra 
>> variable compared to simply calling target_words_bigendian(), isn't it? I 
>> think the diffstat says it all...
> 
> Is the comment misleading then? Why not decrease the code
> size using target_words_bigendian() in all the similar cases?
> 
> $ git grep -A4 'if TARGET_BIG_ENDIAN' hw/
> 
> hw/microblaze/boot.c:145:#if TARGET_BIG_ENDIAN
> hw/microblaze/boot.c-146-        big_endian = 1;
> hw/microblaze/boot.c-147-#endif
> -- 
> hw/mips/jazz.c:160:#if TARGET_BIG_ENDIAN
> hw/mips/jazz.c-161-    big_endian = 1;
> hw/mips/jazz.c-162-#else
> hw/mips/jazz.c-163-    big_endian = 0;
> hw/mips/jazz.c-164-#endif
> -- 
> hw/mips/malta.c:378:#if TARGET_BIG_ENDIAN
> hw/mips/malta.c-379-        val = 0x00000012;
> hw/mips/malta.c-380-#else
> hw/mips/malta.c-381-        val = 0x00000010;
> hw/mips/malta.c-382-#endif
> -- 
> hw/mips/malta.c:631:#if TARGET_BIG_ENDIAN
> hw/mips/malta.c-632-#define cpu_to_gt32(x) (x)
> hw/mips/malta.c-633-#else
> hw/mips/malta.c-634-#define cpu_to_gt32(x) bswap32(x)
> hw/mips/malta.c-635-#endif

If it's just about a variable that gets initialized to 0 or 1, replacing it 
with target_words_bigendian() certainly make a lot of sense. Not sure about 
this spot in malta.c, though, this is a bit different since it declares a 
macro instead.

> So within hw/ I'd restrict target_words_bigendian() use to
> MachineClass::init() handlers, and prohibit TARGET_BIG_ENDIAN
> from hw/. Only use in softmmu/, target, *-user/. If we agree
> we can rewrite the comment, removing the "do *not* use this
> function unless you know very well what you are doing!" which
> is hard to interpret IMHO.

Ok, now I got you, I think. Yes, I agree we should update the comment to say 
that it should not be used in *devices* (unless you know what you're doing, 
e.g. in virtio code). I just also found the original discussion which was 
about the same thoughts:

  https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg00939.html

  Thomas

Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable

[Peter Maydell]

On Mon, 28 Aug 2023 at 18:00, Thomas Huth <thuth@redhat.com> wrote:
>
> On 28/08/2023 17.48, Philippe Mathieu-Daudé wrote:
> > On 28/8/23 14:41, Thomas Huth wrote:
> >> On 28/08/2023 14.19, Philippe Mathieu-Daudé wrote:
> >>> Hi Thomas,
> >>>
> >>> On 25/8/23 19:51, Thomas Huth wrote:
> >>>> There is an easier way to get a value that can be used to decide
> >>>> whether the target is big endian or not: Simply use the
> >>>> target_words_bigendian() function instead.
> >>>>
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>> ---
> >>>>   hw/mips/jazz.c | 10 ++--------
> >>>>   1 file changed, 2 insertions(+), 8 deletions(-)
> >>>
> >>>
> >>>> @@ -157,12 +157,6 @@ static void mips_jazz_init(MachineState *machine,
> >>>>           [JAZZ_PICA61] = {33333333, 4},
> >>>>       };
> >>>> -#if TARGET_BIG_ENDIAN
> >>>> -    big_endian = 1;
> >>>> -#else
> >>>> -    big_endian = 0;
> >>>> -#endif
> >>>> -
> >>>>       if (machine->ram_size > 256 * MiB) {
> >>>>           error_report("RAM size more than 256Mb is not supported");
> >>>>           exit(EXIT_FAILURE);
> >>>> @@ -301,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
> >>>>               dev = qdev_new("dp8393x");
> >>>>               qdev_set_nic_properties(dev, nd);
> >>>>               qdev_prop_set_uint8(dev, "it_shift", 2);
> >>>> -            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
> >>>> +            qdev_prop_set_bit(dev, "big_endian",
> >>>> target_words_bigendian());
> >>>
> >>> IIRC last time I tried that Peter pointed me at the documentation:
> >>>
> >>> /**
> >>>   * target_words_bigendian:
> >>>   * Returns true if the (default) endianness of the target is big endian,
> >>>   * false otherwise. Note that in target-specific code, you can use
> >>>   * TARGET_BIG_ENDIAN directly instead. On the other hand, common
> >>>   * code should normally never need to know about the endianness of the
> >>>   * target, so please do *not* use this function unless you know very
> >>>   * well what you are doing!
> >>>   */
> >>>
> >>> (Commit c95ac10340 "cpu: Provide a proper prototype for
> >>>   target_words_bigendian() in a header")
> >>>
> >>> Should we update the comment?
> >>
> >> What would you change? My motivation here was mainly to decrease the size
> >> of the code - I think it's way more complicated via the #if + extra
> >> variable compared to simply calling target_words_bigendian(), isn't it? I
> >> think the diffstat says it all...
> >
> > Is the comment misleading then? Why not decrease the code
> > size using target_words_bigendian() in all the similar cases?

The idea of the comment is:
(1) if you're in common code, then it's rather odd to want to
know the endianness of the target
(2) if you're not in common code you can use TARGET_BIG_ENDIAN
directly, which will evaluate to the same thing as
target_words_bigendian() but without doing the function call.

The function is only needed in the (currently) unusual case where
you are in a compiled-once-for-all-targets source file but you
still need to know the target endianness.

> If it's just about a variable that gets initialized to 0 or 1, replacing it
> with target_words_bigendian() certainly make a lot of sense. Not sure about
> this spot in malta.c, though, this is a bit different since it declares a
> macro instead.

You can use
   qdev_prop_set_bit(dev, "big_endian", TARGET_BIG_ENDIAN);

because the macro is always defined, to either 0 or 1.

The reason to maybe rethink this would be for the purposes
of getting board source files to compile-once, which it feels
to me is still rather far away.

thanks
-- PMM

Re: [PATCH 0/3] hw/mips/jazz: Rework the NIC init code

[Michael Tokarev]

25.08.2023 20:51, Thomas Huth wrote:
> The NIC init code of the jazz machines is rather cumbersome, with
> a for-loop around it that is always left after the first iteration.
> This patch series reworks this a little bit to make the code more
> readable and shorter.
> 
> Thomas Huth (3):
>    hw/mips/jazz: Remove the big_endian variable
>    hw/mips/jazz: Move the NIC init code into a separate function
>    hw/mips/jazz: Simplify the NIC setup code

Thomas, after comments for "hw/mips/jazz: Remove the big_endian variable",
will you respin?

Thanks!

/mjt

Re: [PATCH 0/3] hw/mips/jazz: Rework the NIC init code

[Thomas Huth]

On 01/09/2023 12.15, Michael Tokarev wrote:
> 25.08.2023 20:51, Thomas Huth wrote:
>> The NIC init code of the jazz machines is rather cumbersome, with
>> a for-loop around it that is always left after the first iteration.
>> This patch series reworks this a little bit to make the code more
>> readable and shorter.
>>
>> Thomas Huth (3):
>>    hw/mips/jazz: Remove the big_endian variable
>>    hw/mips/jazz: Move the NIC init code into a separate function
>>    hw/mips/jazz: Simplify the NIC setup code
> 
> Thomas, after comments for "hw/mips/jazz: Remove the big_endian variable",
> will you respin?

I've now sent a patch to clean up that ugliness with TARGET_BIG_ENDIAN first:

https://lore.kernel.org/qemu-devel/20230907113500.185276-1-thuth@redhat.com/

Once that has been accepted, I'll respin the other patches.

  Thomas