Re: [vmw_vmci 11/11] Apply the header code to make VMCI build

[Greg KH <gregkh@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