Re: [PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts

[Pavel Pisa]

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 <peter.maydell@linaro.org>
> > ---
> > 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