From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg KH Subject: Re: [vmw_vmci 11/11] Apply the header code to make VMCI build Date: Thu, 26 Jul 2012 16:56:06 -0700 Message-ID: <20120726235606.GD3849@kroah.com> References: <1343345980-32397-1-git-send-email-astiegmann@vmware.com> <1343345980-32397-12-git-send-email-astiegmann@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1343345980-32397-12-git-send-email-astiegmann@vmware.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: "Andrew Stiegmann (stieg)" Cc: pv-drivers@vmware.com, vm-crosstalk@vmware.com, linux-kernel@vger.kernel.org, cschamp@vmware.com, virtualization@lists.linux-foundation.org List-Id: virtualization@lists.linuxfoundation.org On Thu, Jul 26, 2012 at 04:39:40PM -0700, Andrew Stiegmann (stieg) wrote: > +#define ASSERT(cond) BUG_ON(!(cond)) Don't do that, you just crashed someone's box and now they have no way to recover it and tell you that you broke it. > +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK)) > +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED) > + > +#define PCI_VENDOR_ID_VMWARE 0x15AD What's wrong with the one in pci_ids.h? > +#define PCI_DEVICE_ID_VMWARE_VMCI 0x0740 > +#define VMCI_DRIVER_VERSION_STRING "9.5.5.0-k" Do you really need this? > +#define MODULE_NAME "vmw_vmci" The kernel provides this for you already, don't duplicate it. > + > +/* Print magic... whee! */ > +#ifdef pr_fmt > +#undef pr_fmt No need for these 2 lines > +#define pr_fmt(fmt) MODULE_NAME ": " fmt > +#endif Or this one. > +/* > + * Linux defines _IO* macros, but the core kernel code ignore the encoded > + * ioctl value. It is up to individual drivers to decode the value (for > + * example to look at the size of a structure to determine which version > + * of a specific command should be used) or not (which is what we > + * currently do, so right now the ioctl value for a given command is the > + * command itself). > + * > + * Hence, we just define the IOCTL_VMCI_foo values directly, with no > + * intermediate IOCTLCMD_ representation. > + */ > +# define IOCTLCMD(_cmd) IOCTL_VMCI_ ## _cmd Are you sure about this comment? > + > +enum { > + /* > + * We need to bracket the range of values used for ioctls, > + * because x86_64 Linux forces us to explicitly register ioctl > + * handlers by value for handling 32 bit ioctl syscalls. > + * Hence FIRST and LAST. Pick something for FIRST that > + * doesn't collide with vmmon (2001+). > + */ > + IOCTLCMD(FIRST) = 1951, > + IOCTLCMD(VERSION) = IOCTLCMD(FIRST), > + > + /* BEGIN VMCI */ > + IOCTLCMD(INIT_CONTEXT), > + > + /* > + * The following two were used for process and datagram > + * process creation. They are not used anymore and reserved > + * for future use. They will fail if issued. > + */ > + IOCTLCMD(RESERVED1), > + IOCTLCMD(RESERVED2), > + > + /* > + * The following used to be for shared memory. It is now > + * unused and and is reserved for future use. It will fail if > + * issued. > + */ > + IOCTLCMD(RESERVED3), > + > + /* > + * The follwoing three were also used to be for shared > + * memory. An old WS6 user-mode client might try to use them > + * with the new driver, but since we ensure that only contexts > + * created by VMX'en of the appropriate version > + * (VMCI_VERSION_NOTIFY or VMCI_VERSION_NEWQP) or higher use > + * these ioctl, everything is fine. > + */ > + IOCTLCMD(QUEUEPAIR_SETVA), > + IOCTLCMD(NOTIFY_RESOURCE), > + IOCTLCMD(NOTIFICATIONS_RECEIVE), > + IOCTLCMD(VERSION2), > + IOCTLCMD(QUEUEPAIR_ALLOC), > + IOCTLCMD(QUEUEPAIR_SETPAGEFILE), > + IOCTLCMD(QUEUEPAIR_DETACH), > + IOCTLCMD(DATAGRAM_SEND), > + IOCTLCMD(DATAGRAM_RECEIVE), > + IOCTLCMD(DATAGRAM_REQUEST_MAP), > + IOCTLCMD(DATAGRAM_REMOVE_MAP), > + IOCTLCMD(CTX_ADD_NOTIFICATION), > + IOCTLCMD(CTX_REMOVE_NOTIFICATION), > + IOCTLCMD(CTX_GET_CPT_STATE), > + IOCTLCMD(CTX_SET_CPT_STATE), > + IOCTLCMD(GET_CONTEXT_ID), > + IOCTLCMD(LAST), > + /* END VMCI */ > + > + /* > + * VMCI Socket IOCTLS are defined next and go from > + * IOCTLCMD(LAST) (1972) to 1990. VMware reserves a range of > + * 4 ioctls for VMCI Sockets to grow. We cannot reserve many > + * ioctls here since we are close to overlapping with vmmon > + * ioctls (2001+). Define a meta-ioctl if running out of this > + * binary space. > + */ > + IOCTLCMD(SOCKETS_LAST) = 1994, /* 1994 on Linux. */ > + > + /* > + * The VSockets ioctls occupy the block above. We define a > + * new range of VMCI ioctls to maintain binary compatibility > + * between the user land and the kernel driver. Careful, > + * vmmon ioctls start from 2001, so this means we can add only > + * 4 new VMCI ioctls. Define a meta-ioctl if running out of > + * this binary space. > + */ > + IOCTLCMD(FIRST2), > + IOCTLCMD(SET_NOTIFY) = IOCTLCMD(FIRST2), /* 1995 on Linux. */ > + IOCTLCMD(LAST2), > +}; That's a lot of ioctls. Why not just create a new system call, or many system calls, instead? > +/* > + * This struct is used to contain data for events. Size of this struct is a > + * multiple of 8 bytes, and all fields are aligned to their natural alignment. > + */ > +struct vmci_event_data { > + uint32_t event; /* 4 bytes. */ > + uint32_t _pad; > + /* Event payload is put here. */ > +}; Why not put an empty array so you can get to the data easier instead of having to do looney inline functions like this: > +/* > + * We use the following inline function to access the payload data > + * associated with an event data. > + */ > +static inline void *vmci_event_data_payload(struct vmci_event_data *evData) > +{ > + return (void *)((char *)evData + sizeof *evData); > +} Same goes for other structures that you do this same thing. greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752792Ab2GZX4L (ORCPT ); Thu, 26 Jul 2012 19:56:11 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:46779 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751965Ab2GZX4K (ORCPT ); Thu, 26 Jul 2012 19:56:10 -0400 Date: Thu, 26 Jul 2012 16:56:06 -0700 From: Greg KH To: "Andrew Stiegmann (stieg)" Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, pv-drivers@vmware.com, vm-crosstalk@vmware.com, cschamp@vmware.com Subject: Re: [vmw_vmci 11/11] Apply the header code to make VMCI build Message-ID: <20120726235606.GD3849@kroah.com> References: <1343345980-32397-1-git-send-email-astiegmann@vmware.com> <1343345980-32397-12-git-send-email-astiegmann@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1343345980-32397-12-git-send-email-astiegmann@vmware.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 26, 2012 at 04:39:40PM -0700, Andrew Stiegmann (stieg) wrote: > +#define ASSERT(cond) BUG_ON(!(cond)) Don't do that, you just crashed someone's box and now they have no way to recover it and tell you that you broke it. > +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK)) > +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED) > + > +#define PCI_VENDOR_ID_VMWARE 0x15AD What's wrong with the one in pci_ids.h? > +#define PCI_DEVICE_ID_VMWARE_VMCI 0x0740 > +#define VMCI_DRIVER_VERSION_STRING "9.5.5.0-k" Do you really need this? > +#define MODULE_NAME "vmw_vmci" The kernel provides this for you already, don't duplicate it. > + > +/* Print magic... whee! */ > +#ifdef pr_fmt > +#undef pr_fmt No need for these 2 lines > +#define pr_fmt(fmt) MODULE_NAME ": " fmt > +#endif Or this one. > +/* > + * Linux defines _IO* macros, but the core kernel code ignore the encoded > + * ioctl value. It is up to individual drivers to decode the value (for > + * example to look at the size of a structure to determine which version > + * of a specific command should be used) or not (which is what we > + * currently do, so right now the ioctl value for a given command is the > + * command itself). > + * > + * Hence, we just define the IOCTL_VMCI_foo values directly, with no > + * intermediate IOCTLCMD_ representation. > + */ > +# define IOCTLCMD(_cmd) IOCTL_VMCI_ ## _cmd Are you sure about this comment? > + > +enum { > + /* > + * We need to bracket the range of values used for ioctls, > + * because x86_64 Linux forces us to explicitly register ioctl > + * handlers by value for handling 32 bit ioctl syscalls. > + * Hence FIRST and LAST. Pick something for FIRST that > + * doesn't collide with vmmon (2001+). > + */ > + IOCTLCMD(FIRST) = 1951, > + IOCTLCMD(VERSION) = IOCTLCMD(FIRST), > + > + /* BEGIN VMCI */ > + IOCTLCMD(INIT_CONTEXT), > + > + /* > + * The following two were used for process and datagram > + * process creation. They are not used anymore and reserved > + * for future use. They will fail if issued. > + */ > + IOCTLCMD(RESERVED1), > + IOCTLCMD(RESERVED2), > + > + /* > + * The following used to be for shared memory. It is now > + * unused and and is reserved for future use. It will fail if > + * issued. > + */ > + IOCTLCMD(RESERVED3), > + > + /* > + * The follwoing three were also used to be for shared > + * memory. An old WS6 user-mode client might try to use them > + * with the new driver, but since we ensure that only contexts > + * created by VMX'en of the appropriate version > + * (VMCI_VERSION_NOTIFY or VMCI_VERSION_NEWQP) or higher use > + * these ioctl, everything is fine. > + */ > + IOCTLCMD(QUEUEPAIR_SETVA), > + IOCTLCMD(NOTIFY_RESOURCE), > + IOCTLCMD(NOTIFICATIONS_RECEIVE), > + IOCTLCMD(VERSION2), > + IOCTLCMD(QUEUEPAIR_ALLOC), > + IOCTLCMD(QUEUEPAIR_SETPAGEFILE), > + IOCTLCMD(QUEUEPAIR_DETACH), > + IOCTLCMD(DATAGRAM_SEND), > + IOCTLCMD(DATAGRAM_RECEIVE), > + IOCTLCMD(DATAGRAM_REQUEST_MAP), > + IOCTLCMD(DATAGRAM_REMOVE_MAP), > + IOCTLCMD(CTX_ADD_NOTIFICATION), > + IOCTLCMD(CTX_REMOVE_NOTIFICATION), > + IOCTLCMD(CTX_GET_CPT_STATE), > + IOCTLCMD(CTX_SET_CPT_STATE), > + IOCTLCMD(GET_CONTEXT_ID), > + IOCTLCMD(LAST), > + /* END VMCI */ > + > + /* > + * VMCI Socket IOCTLS are defined next and go from > + * IOCTLCMD(LAST) (1972) to 1990. VMware reserves a range of > + * 4 ioctls for VMCI Sockets to grow. We cannot reserve many > + * ioctls here since we are close to overlapping with vmmon > + * ioctls (2001+). Define a meta-ioctl if running out of this > + * binary space. > + */ > + IOCTLCMD(SOCKETS_LAST) = 1994, /* 1994 on Linux. */ > + > + /* > + * The VSockets ioctls occupy the block above. We define a > + * new range of VMCI ioctls to maintain binary compatibility > + * between the user land and the kernel driver. Careful, > + * vmmon ioctls start from 2001, so this means we can add only > + * 4 new VMCI ioctls. Define a meta-ioctl if running out of > + * this binary space. > + */ > + IOCTLCMD(FIRST2), > + IOCTLCMD(SET_NOTIFY) = IOCTLCMD(FIRST2), /* 1995 on Linux. */ > + IOCTLCMD(LAST2), > +}; That's a lot of ioctls. Why not just create a new system call, or many system calls, instead? > +/* > + * This struct is used to contain data for events. Size of this struct is a > + * multiple of 8 bytes, and all fields are aligned to their natural alignment. > + */ > +struct vmci_event_data { > + uint32_t event; /* 4 bytes. */ > + uint32_t _pad; > + /* Event payload is put here. */ > +}; Why not put an empty array so you can get to the data easier instead of having to do looney inline functions like this: > +/* > + * We use the following inline function to access the payload data > + * associated with an event data. > + */ > +static inline void *vmci_event_data_payload(struct vmci_event_data *evData) > +{ > + return (void *)((char *)evData + sizeof *evData); > +} Same goes for other structures that you do this same thing. greg k-h