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

From: Wei Liu <wei.liu@kernel.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Nuno Das Neves <nunodasneves@linux.microsoft.com>,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
	x86@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-arch@vger.kernel.org, patches@lists.linux.dev,
	mikelley@microsoft.com, kys@microsoft.com, wei.liu@kernel.org,
	haiyangz@microsoft.com, decui@microsoft.com,
	apais@linux.microsoft.com, Tianyu.Lan@microsoft.com,
	ssengar@linux.microsoft.com, mukeshrathor@microsoft.com,
	stanislav.kinsburskiy@gmail.com, jinankjain@linux.microsoft.com,
	vkuznets@redhat.com, tglx@linutronix.de, mingo@redhat.com,
	bp@alien8.de, dave.hansen@linux.intel.com, hpa@zytor.com,
	will@kernel.org, catalin.marinas@arm.com
Subject: Re: [PATCH v4 13/15] uapi: hyperv: Add mshv driver headers defining hypervisor ABIs
Date: Sat, 30 Sep 2023 22:01:58 +0000	[thread overview]
Message-ID: <ZRia1uyFfEkSqmXw@liuwe-devbox-debian-v2> (raw)
In-Reply-To: <2023093057-eggplant-reshoot-8513@gregkh>

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:
> > These must be in uapi because they will be used in the mshv ioctl API.
> > 
> > Version numbers for each file:
> > hvhdk.h		25212
> > hvhdk_mini.h	25294
> > hvgdk.h		25125
> > hvgdk_mini.h	25294
> 
> what are version numbers?

These are internal version numbers for the hypervisor headers. We keep
track of them so that we can detect if there are any breakages in the
ABI, and thus either ask them to be fixed or we come up with ways to
maintain compatibility. People outside of Microsoft don't need to worry
about this. If you don't think this information belongs in the commit
message, we can drop it.

> 
> > These are unstable interfaces and as such must be compiled independently
> > from published interfaces found in hyperv-tlfs.h.
> 
> uapi files can NOT be unstable, that's the opposite of an api :(
> 

You made a few suggestions in the past. Nuno responded here:

https://lore.kernel.org/linux-hyperv/1692309711-5573-1-git-send-email-nunodasneves@linux.microsoft.com/T/#m3dd8035e381a1344acd7f570c3f5921b7415bedb

> > Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> > Acked-by: Wei Liu <wei.liu@kernel.org>
> > ---
> >  include/uapi/hyperv/hvgdk.h      |   41 +
> >  include/uapi/hyperv/hvgdk_mini.h | 1076 ++++++++++++++++++++++++
> >  include/uapi/hyperv/hvhdk.h      | 1342 ++++++++++++++++++++++++++++++
> >  include/uapi/hyperv/hvhdk_mini.h |  160 ++++
> >  4 files changed, 2619 insertions(+)
> >  create mode 100644 include/uapi/hyperv/hvgdk.h
> >  create mode 100644 include/uapi/hyperv/hvgdk_mini.h
> >  create mode 100644 include/uapi/hyperv/hvhdk.h
> >  create mode 100644 include/uapi/hyperv/hvhdk_mini.h
> > 
> > diff --git a/include/uapi/hyperv/hvgdk.h b/include/uapi/hyperv/hvgdk.h
> > new file mode 100644
> > index 000000000000..9bcbb7d902b2
> > --- /dev/null
> > +++ b/include/uapi/hyperv/hvgdk.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: MIT */
> 
> That's usually not a good license for a new uapi .h file, why did you
> choose this one?
> 

This is chosen so that other Microsoft developers who don't normally
work on Linux can review this code.

> > +/* Define connection identifier type. */
> > +union hv_connection_id {
> > +	__u32 asu32;
> > +	struct {
> > +		__u32 id:24;
> > +		__u32 reserved:8;
> > +	} __packed u;
> 
> 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?
Or something else?

Just by eyeballing the header files under include/uapi, there are a
non-trivial number of files that use bitfields.

include/uapi/linux/cdrom.h
include/uapi/linux/hdreg.h
include/uapi/linux/if_pppox.h
include/uapi/linux/adfs_fs.h
include/uapi/linux/atm.h
include/uapi/linux/batadv_packet.h
include/uapi/linux/bpf.h
include/uapi/linux/cciss_defs.h
include/uapi/linux/dccp.h
include/uapi/linux/erspan.h
include/uapi/linux/i2o-dev.h
include/uapi/linux/icmp.h
include/uapi/linux/icmpv6.h
include/uapi/linux/idxd.h
include/uapi/linux/if_hippi.h
include/uapi/linux/igmp.h
include/uapi/linux/inet_diag.h
include/uapi/linux/ioam6.h
include/uapi/linux/ip.h
include/uapi/linux/netfilter/xt_policy.h
include/uapi/linux/perf_event.h
include/uapi/linux/rpl.h
include/uapi/linux/tcp.h
include/uapi/linux/usb/raw_gadget.h
include/uapi/linux/watch_queue.h
include/uapi/scsi/scsi_bsg_ufs.h
include/uapi/sound/asound.h
include/uapi/sound/skl-tplg-interface.h

Also under arch/x86/include/uapi/asm:

arch/x86/include/uapi/asm/kvm.h

Can you help us understand how we can make our code work like the others
listed above? There must be a way since they are all in the tree. We're
happy to make adjustments.

Thanks,
Wei.

> 
> thanks,
> 
> greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel