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 <th...@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.
