Re: [PATCH 1/3] hw/mips/jazz: Remove the big_endian variable
On Mon, 28 Aug 2023 at 18:00, Thomas Huth 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
> ---
> 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] = {, 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 1/3] hw/mips/jazz: Remove the big_endian variable
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
---
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] = {, 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 = 0x0012;
hw/mips/malta.c-380-#else
hw/mips/malta.c-381- val = 0x0010;
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
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
---
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] = {, 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 = 0x0012;
hw/mips/malta.c-380-#else
hw/mips/malta.c-381-val = 0x0010;
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
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
---
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] = {, 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
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
---
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] = {, 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
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 --- hw/mips/jazz.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) Reviewed-by: Richard Henderson r~
[PATCH 1/3] hw/mips/jazz: Remove the big_endian variable
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
---
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] = {, 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), _abort);
sysbus = SYS_BUS_DEVICE(dev);
--
2.39.3
