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

[Thomas Huth <thuth@redhat.com>]

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