On Mon, 28 Aug 2023 at 18:00, Thomas Huth <th...@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 <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?
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
