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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
