On Wed, Feb 3, 2021 at 2:26 PM Wei Liu <wei.liu@xxxxxxxxxx> wrote:
> On Tue, Feb 02, 2021 at 05:02:48PM +0000, Wei Liu wrote:
> > On Tue, Jan 26, 2021 at 01:26:52AM +0000, Michael Kelley wrote:
> > > From: Wei Liu <wei.liu@xxxxxxxxxx> Sent: Wednesday, January 20, 2021 4:01 AM
> > > > +union hv_device_id {
> > > > + u64 as_uint64;
> > > > +
> > > > + struct {
> > > > + u64 :62;
> > > > + u64 device_type:2;
> > > > + };
> > >
> > > Are the above 4 lines extraneous junk?
> > > If not, a comment would be helpful. And we
> > > would normally label the 62 bit field as
> > > "reserved0" or something similar.
> > >
> >
> > No. It is not junk. I got this from a header in tree.
> >
> > I am inclined to just drop this hunk. If that breaks things, I will use
> > "reserved0".
> >
>
> It turns out adding reserved0 is required. Dropping this hunk does not
> work.
Generally speaking, bitfields are not great for specifying binary interfaces,
as the actual bit order can differ by architecture. The normal way we get
around it in the kernel is to use basic integer types and define macros
for bit masks. Ideally, each such field should also be marked with a
particular endianess as __le64 or __be64, in case this is ever used with
an Arm guest running a big-endian kernel.
That said, if you do not care about the specific order of the bits, having
anonymous bitfields for the reserved members is fine, I don't see a
reason to name it as reserved.
Arnd
