Hi Greg,
For some reason it still didn't go through to our corporate mail server
but I see it on LKML.
On Mon, Nov 26, 2012 at 04:03:04PM -0800, Greg KH wrote:
> On Wed, Nov 07, 2012 at 10:43:03AM -0800, George Zhang wrote:
>
> > +static inline struct vmci_handle VMCI_MAKE_HANDLE(vmci_id cid, vmci_id rid)
> > +{
> > + struct vmci_handle h;
> > + h.context = cid;
> > + h.resource = rid;
> > + return h;
> > +}
>
> You return a structure on the stack that just went away? Yeah, I know
> it's an inline, but come on, that's not ok.
This is certainly OK even if it is not inline, we return the _value_,
not the pointer to the stacki memory. And yes, the structure is 64 bit
value so it is returned in registers.
>
> > +#define VMCI_HANDLE_TO_CONTEXT_ID(_handle) ((_handle).context)
> > +#define VMCI_HANDLE_TO_RESOURCE_ID(_handle) ((_handle).resource)
>
> It's longer to write the macro out than to just do .context or
> .resource, just use them instead.
We really want vmci_handle to be opaque where possible and use proper
accessors.
>
> > +#define VMCI_HANDLE_EQUAL(_h1, _h2) ((_h1).context == (_h2).context && \
> > + (_h1).resource == (_h2).resource)
>
> Inline function instead? How often do you really need this?
OK, makes sense.
>
> > +#define VMCI_INVALID_ID ~0
> > +static const struct vmci_handle VMCI_INVALID_HANDLE = { VMCI_INVALID_ID,
> > + VMCI_INVALID_ID
> > +};
>
> C99 initializers please.
OK.
>
> > +
> > +#define VMCI_HANDLE_INVALID(_handle) \
> > + VMCI_HANDLE_EQUAL((_handle), VMCI_INVALID_HANDLE)
>
> Gotta love magic values :(
?
>
> > +/*
> > + * The below defines can be used to send anonymous requests.
> > + * This also indicates that no response is expected.
> > + */
> > +#define VMCI_ANON_SRC_CONTEXT_ID VMCI_INVALID_ID
> > +#define VMCI_ANON_SRC_RESOURCE_ID VMCI_INVALID_ID
> > +#define VMCI_ANON_SRC_HANDLE vmci_make_handle(VMCI_ANON_SRC_CONTEXT_ID, \
> > + VMCI_ANON_SRC_RESOURCE_ID)
>
> So you just created an invalid message?
Anonymous one, yes.
>
> > +/* The lowest 16 context ids are reserved for internal use. */
> > +#define VMCI_RESERVED_CID_LIMIT ((u32) 16)
> > +
> > +/*
> > + * Hypervisor context id, used for calling into hypervisor
> > + * supplied services from the VM.
> > + */
> > +#define VMCI_HYPERVISOR_CONTEXT_ID 0
> > +
> > +/*
> > + * Well-known context id, a logical context that contains a set of
> > + * well-known services. This context ID is now obsolete.
> > + */
> > +#define VMCI_WELL_KNOWN_CONTEXT_ID 1
> > +
> > +/*
> > + * Context ID used by host endpoints.
> > + */
> > +#define VMCI_HOST_CONTEXT_ID 2
> > +
> > +#define VMCI_CONTEXT_IS_VM(_cid) (VMCI_INVALID_ID != (_cid) && \
> > + (_cid) > VMCI_HOST_CONTEXT_ID)
> > +
>
> Are you sure all of this stuff needs to be in a .h file that lives in
> include/linux/?
Probably not. We'll rebase to 3.7 and split as needed.
>
> > +/*
> > + * 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
>
> I don't recall ever getting a valid answer for this (if you did, my
> appologies, can you repeat it). What in the world are you talking about
> here? Why is your driver somehow special from the thousands of other
> ones that use the in-kernel IO macros properly for an ioctl?
Hmm, not sure, we'll review ioctl definitions and fix as needed.
Thanks!
--
Dmitry
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
