Re: [PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts
On Friday 06 of November 2020 19:29:27 Philippe Mathieu-Daudé wrote: > On 11/6/20 6:11 PM, Peter Maydell wrote: > > The ctucan driver defines types for its registers which are a union > > of a uint32_t with a struct with bitfields for the individual > > fields within that register. This is a bad idea, because bitfields > > aren't portable. The ctu_can_fd_regs.h header works around the > > most glaring of the portability issues by defining the > > fields in two different orders depending on the setting of the > > __LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this > > is unconditionally set to 1, which is wrong for big-endian hosts. > > > > Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need > > for a "have we defined it already" guard, because the only place > > that should set it is ctucan_core.h, which has the usual > > double-inclusion guard. > > > > Signed-off-by: Peter Maydell > > --- > > Ideally all that bitfield-using code would be rewritten to use > > extract32 and deposit32 instead, IMHO. > > --- > > hw/net/can/ctucan_core.h | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h > > index f21cb1c5ec3..bbc09ae0678 100644 > > --- a/hw/net/can/ctucan_core.h > > +++ b/hw/net/can/ctucan_core.h > > @@ -31,8 +31,7 @@ > > #include "exec/hwaddr.h" > > #include "net/can_emu.h" > > > > - > > -#ifndef __LITTLE_ENDIAN_BITFIELD > > +#ifndef HOST_WORDS_BIGENDIAN > > #define __LITTLE_ENDIAN_BITFIELD 1 > > #endif > > Alternatively s/#ifdef __LITTLE_ENDIAN_BITFIELD/#ifndef > HOST_WORDS_BIGENDIAN/ the codebase and remove this here... But then we cannot have same generated, untouch header file common for Linux kernel and QEMU. Each uses different defines. Or at least it was the goal, but after mainline Linux review we switch in longer run to defines with use of BIT, GENMASK FIELD_GET and FIELD_PREP. But even GENMASK does not seem to be defined in QEMU headers even that it is referenced in include/hw/usb/dwc2-regs.h and include/standard-headers/asm-x86/kvm_para.h So idea to have nice common generated headers which can be checked for consistency and right version by diff for Linux kernel, QEMU and even userspace tests and other OSes (there with Linux defines substitution) seems to be only dream. Anyway, we switch to solution which is matching requirements of each project if it is suggested. But it takes time. Best wishes, Pavel
Re: [PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts
On 11/6/20 6:11 PM, Peter Maydell wrote: > The ctucan driver defines types for its registers which are a union > of a uint32_t with a struct with bitfields for the individual > fields within that register. This is a bad idea, because bitfields > aren't portable. The ctu_can_fd_regs.h header works around the > most glaring of the portability issues by defining the > fields in two different orders depending on the setting of the > __LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this > is unconditionally set to 1, which is wrong for big-endian hosts. > > Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need > for a "have we defined it already" guard, because the only place > that should set it is ctucan_core.h, which has the usual > double-inclusion guard. > > Signed-off-by: Peter Maydell > --- > Ideally all that bitfield-using code would be rewritten to use > extract32 and deposit32 instead, IMHO. > --- > hw/net/can/ctucan_core.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h > index f21cb1c5ec3..bbc09ae0678 100644 > --- a/hw/net/can/ctucan_core.h > +++ b/hw/net/can/ctucan_core.h > @@ -31,8 +31,7 @@ > #include "exec/hwaddr.h" > #include "net/can_emu.h" > > - > -#ifndef __LITTLE_ENDIAN_BITFIELD > +#ifndef HOST_WORDS_BIGENDIAN > #define __LITTLE_ENDIAN_BITFIELD 1 > #endif Alternatively s/#ifdef __LITTLE_ENDIAN_BITFIELD/#ifndef HOST_WORDS_BIGENDIAN/ the codebase and remove this here... Reviewed-by: Philippe Mathieu-Daudé
[PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts
The ctucan driver defines types for its registers which are a union of a uint32_t with a struct with bitfields for the individual fields within that register. This is a bad idea, because bitfields aren't portable. The ctu_can_fd_regs.h header works around the most glaring of the portability issues by defining the fields in two different orders depending on the setting of the __LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this is unconditionally set to 1, which is wrong for big-endian hosts. Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need for a "have we defined it already" guard, because the only place that should set it is ctucan_core.h, which has the usual double-inclusion guard. Signed-off-by: Peter Maydell --- Ideally all that bitfield-using code would be rewritten to use extract32 and deposit32 instead, IMHO. --- hw/net/can/ctucan_core.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h index f21cb1c5ec3..bbc09ae0678 100644 --- a/hw/net/can/ctucan_core.h +++ b/hw/net/can/ctucan_core.h @@ -31,8 +31,7 @@ #include "exec/hwaddr.h" #include "net/can_emu.h" - -#ifndef __LITTLE_ENDIAN_BITFIELD +#ifndef HOST_WORDS_BIGENDIAN #define __LITTLE_ENDIAN_BITFIELD 1 #endif -- 2.20.1
