Michael Kelley
2021-Feb-04 16:46 UTC
[PATCH v5 13/16] asm-generic/hyperv: introduce hv_device_id and auxiliary structures
From: Wei Liu <wei.liu at kernel.org> Sent: Wednesday, February 3, 2021 6:09 AM> > On Wed, Feb 03, 2021 at 02:49:53PM +0100, Arnd Bergmann wrote: > > On Wed, Feb 3, 2021 at 2:26 PM Wei Liu <wei.liu at kernel.org> 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 at kernel.org> 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. > > Thanks for the information. > > I think we will need to wait until Microsoft Hypervisor clearly defines > the endianess in its header(s) before we can make changes to the copy in > Linux. > > > > > 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. > > Michael, let me know what you think. I'm not too fussed either way. > > Wei.I'm OK either way. In the Hyper-V code we've typically given such fields a name rather than leave them anonymous, which is why it stuck out. Michael> > > > > Arnd