Re: [PATCH v4 13/15] uapi: hyperv: Add mshv driver headers defining hypervisor ABIs

[Nuno Das Neves]

On 10/4/2023 10:50 AM, Greg KH wrote:
> On Wed, Oct 04, 2023 at 05:36:32PM +0000, Dexuan Cui wrote:
> > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Sent: Tuesday, October 3, 2023 11:10 PM
> > > [...]
> > > On Tue, Oct 03, 2023 at 04:37:01PM -0700, Nuno Das Neves wrote:
> > > > On 9/30/2023 11:19 PM, Greg KH wrote:
> > > > > On Sat, Sep 30, 2023 at 10:01:58PM +0000, Wei Liu wrote:
> > > > > > On Sat, Sep 30, 2023 at 08:09:19AM +0200, Greg KH wrote:
> > > > > > > On Fri, Sep 29, 2023 at 11:01:39AM -0700, Nuno Das Neves wrote:
> > > > > > > > +/* Define connection identifier type. */
> > > > > > > > +union hv_connection_id {
> > > > > > > > +   __u32 asu32;
> > > > > > > > +   struct {
> > > > > > > > +           __u32 id:24;
> > > > > > > > +           __u32 reserved:8;
> > > > > > > > +   } __packed u;
> >
> > IMO the "__packed" is unnecessary.
> >
> > > > > > > bitfields will not work properly in uapi .h files, please never do that.
> > > > > >
> > > > > > Can you clarify a bit more why it wouldn't work? Endianess? Alignment?
> > > > >
> > > > > Yes to both.
> > > > >
> > > > > Did you all read the documentation for how to write a kernel api?  If
> > > > > not, please do so.  I think it mentions bitfields, but it not, it really
> > > > > should as of course, this will not work properly with different endian
> > > > > systems or many compilers.
> > > >
> > > > Yes, in
> > > https://docs.k/
> > > ernel.org%2Fdriver-
> > > api%2Fioctl.html&data=05%7C01%7Cdecui%40microsoft.com%7Ce404769e0f
> > > 85493f0aa108dbc4a08a27%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C
> > > 0%7C638319966071263290%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> > > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> > > 7C%7C&sdata=RiLNA5DRviWBQK6XXhxC4m77raSDBb%2F0BB6BDpFPUJY%3D
> > > &reserved=0 it says that it is
> > > > "better to avoid" bitfields.
> > > >
> > > > Unfortunately bitfields are used in the definition of the hypervisor
> > > > ABI. We import these definitions directly from the hypervisor code.
> > >
> > > So why do you feel you have to use this specific format for your
> > > user/kernel api?  That is not what is going to the hypervisor.
These *are* going to the hypervisor - we use these same definitions in
our driver for the kernel/hypervisor API. This is so we don't have to
maintain two separate definitions for user/kernel and kernel/hypervisor
APIs.

> > If it's hard to avoid bitfield here, maybe we can refer to the definition of
> > struct iphdr in include/uapi/linux/ip.h
>
> It is not hard to avoid using bitfields, just use the proper definitions
> to make this portable for all compilers.  And ick, ip.h is not a good
> thing to follow :)
Greg, there is nothing making us use bitfields. It just makes the work
of porting the hypervisor definitions to Linux easier - aided by the
fact that in practice, all the compilers in our stack produce the same
code for these.

If that stops being true, or we need to support some other scenario,
then I can see the value in changing it. Right now it just feels like
pointless work.

Just a reminder - we are the only consumers of this code right now; no
one else can meaningfully use this interface yet.

That all said, if you really insist on changing it, then please say so.
And please point to an example of how it should be done so there is no
confusion on the best path forward.

Thanks,
Nuno

> thanks,
>
> greg k-h