| Message ID | 20230825175123.624114-2-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/mips/jazz: Rework the NIC init code | expand |
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~
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?
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
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);
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(-)