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.3Hi 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?
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
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.
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
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
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~
© 2016 - 2024 Red Hat, Inc.