Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Fri, Nov 30, 2012 at 12:58:25PM -0800, Dmitry Torokhov wrote: > On Friday, November 30, 2012 12:44:06 PM Greg KH wrote: > > On Fri, Nov 30, 2012 at 12:09:40PM -0800, Dmitry Torokhov wrote: > > > On Friday, November 30, 2012 10:57:55 AM Greg KH wrote: > > > > On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > > > > > However you snipped the rest of my reply: do we really need to > > > > > renumber > > > > > ioctls? There is no benefit for the driver as its ioctl handler does > > > > > not parse the numbers into components. > > > > > > > > I don't know if you need to renumber, I really don't understand what you > > > > were trying to do with this code, and as it was acting differently from > > > > all other kernel ioctl declarations, I asked for some clarity. > > > > > > > > If you can rewrite it to look sane, and keep the same numbers, that's > > > > fine with me. > > > > > > OK, it looks like we can redo them as: > > > > > > #define IOCTL_VMCI_VERSION_IO(7, 0x9f)/* 1951 */ > > > #define IOCTL_VMCI_INIT_CONTEXT _IO(7, 0xa0)/* 1952 */ > > > > > > Is this acceptable? > > > > Sure, that's better. You also got lucky, '7' happens to be unused right > > now. > > Excellent. You said you want the next drop after -rc1, right? Yes please, I will be ignoring patches until then. greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 12:44:06 PM Greg KH wrote: > On Fri, Nov 30, 2012 at 12:09:40PM -0800, Dmitry Torokhov wrote: > > On Friday, November 30, 2012 10:57:55 AM Greg KH wrote: > > > On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > > > > However you snipped the rest of my reply: do we really need to > > > > renumber > > > > ioctls? There is no benefit for the driver as its ioctl handler does > > > > not parse the numbers into components. > > > > > > I don't know if you need to renumber, I really don't understand what you > > > were trying to do with this code, and as it was acting differently from > > > all other kernel ioctl declarations, I asked for some clarity. > > > > > > If you can rewrite it to look sane, and keep the same numbers, that's > > > fine with me. > > > > OK, it looks like we can redo them as: > > > > #define IOCTL_VMCI_VERSION _IO(7, 0x9f)/* 1951 */ > > #define IOCTL_VMCI_INIT_CONTEXT _IO(7, 0xa0)/* 1952 */ > > > > Is this acceptable? > > Sure, that's better. You also got lucky, '7' happens to be unused right > now. Excellent. You said you want the next drop after -rc1, right? -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Fri, Nov 30, 2012 at 12:09:40PM -0800, Dmitry Torokhov wrote: > On Friday, November 30, 2012 10:57:55 AM Greg KH wrote: > > On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > > > However you snipped the rest of my reply: do we really need to renumber > > > ioctls? There is no benefit for the driver as its ioctl handler does > > > not parse the numbers into components. > > > > I don't know if you need to renumber, I really don't understand what you > > were trying to do with this code, and as it was acting differently from > > all other kernel ioctl declarations, I asked for some clarity. > > > > If you can rewrite it to look sane, and keep the same numbers, that's > > fine with me. > > OK, it looks like we can redo them as: > > #define IOCTL_VMCI_VERSION_IO(7, 0x9f)/* 1951 */ > #define IOCTL_VMCI_INIT_CONTEXT _IO(7, 0xa0)/* 1952 */ > > Is this acceptable? Sure, that's better. You also got lucky, '7' happens to be unused right now. greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 10:57:55 AM Greg KH wrote: > On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > > However you snipped the rest of my reply: do we really need to renumber > > ioctls? There is no benefit for the driver as its ioctl handler does > > not parse the numbers into components. > > I don't know if you need to renumber, I really don't understand what you > were trying to do with this code, and as it was acting differently from > all other kernel ioctl declarations, I asked for some clarity. > > If you can rewrite it to look sane, and keep the same numbers, that's > fine with me. OK, it looks like we can redo them as: #define IOCTL_VMCI_VERSION _IO(7, 0x9f)/* 1951 */ #define IOCTL_VMCI_INIT_CONTEXT _IO(7, 0xa0)/* 1952 */ Is this acceptable? Thanks, Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > However you snipped the rest of my reply: do we really need to renumber > ioctls? There is no benefit for the driver as its ioctl handler does > not parse the numbers into components. I don't know if you need to renumber, I really don't understand what you were trying to do with this code, and as it was acting differently from all other kernel ioctl declarations, I asked for some clarity. If you can rewrite it to look sane, and keep the same numbers, that's fine with me. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 10:39:18 AM Greg KH wrote: > On Fri, Nov 30, 2012 at 09:20:41AM -0800, Dmitry Torokhov wrote: > > On Friday, November 30, 2012 09:09:21 AM Greg KH wrote: > > > On Fri, Nov 30, 2012 at 08:47:46AM -0800, Andy King wrote: > > > > I didn't get the resend either, so it seems our corporate mail really > > > > is > > > > eating messages. Lovely. > > > > > > > > > > > +#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? > > > > > > > > Because we're morons. And unfortunately, we've shipped our product > > > > using those broken definitions: our VMX uses them to talk to the > > > > driver. > > > > So here's what we'd like to do. We will send out a patch soon that > > > > fixes the other issues you mention and also adds IOCTL definitions the > > > > proper way using _IOBLAH(). But we'd also like to retain these broken > > > > definitions for a short period, commented as such, at least until we > > > > can get out a patch release to Workstation 9, at which point we can > > > > remove them. Does that sound reasonable? > > > > > > It has been my experience, that when people say "We will remove that api > > > sometime in the future", it never happens. So why not just do it now? > > > > > > Especially given that this code will be coming out in 3.9 at the > > > earliest, and that is 6 months away, so that should be plenty of time to > > > get this fixed up. > > > > Our schedule for releasing hosted products is not necessarily aligned > > with mainline kernel releases. > > And kernel developers don't really care about company schedules, nor > should they, you know this :) > That is why we are offering a compromise so that older installations have a chance to work and nobody has to care about schedules too much. However you snipped the rest of my reply: do we really need to renumber ioctls? There is no benefit for the driver as its ioctl handler does not parse the numbers into components. Thanks, Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Fri, Nov 30, 2012 at 09:20:41AM -0800, Dmitry Torokhov wrote: > On Friday, November 30, 2012 09:09:21 AM Greg KH wrote: > > On Fri, Nov 30, 2012 at 08:47:46AM -0800, Andy King wrote: > > > I didn't get the resend either, so it seems our corporate mail really is > > > eating messages. Lovely. > > > > > > > > > +#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? > > > > > > Because we're morons. And unfortunately, we've shipped our product > > > using those broken definitions: our VMX uses them to talk to the driver. > > > So here's what we'd like to do. We will send out a patch soon that > > > fixes the other issues you mention and also adds IOCTL definitions the > > > proper way using _IOBLAH(). But we'd also like to retain these broken > > > definitions for a short period, commented as such, at least until we > > > can get out a patch release to Workstation 9, at which point we can > > > remove them. Does that sound reasonable? > > > > It has been my experience, that when people say "We will remove that api > > sometime in the future", it never happens. So why not just do it now? > > > > Especially given that this code will be coming out in 3.9 at the > > earliest, and that is 6 months away, so that should be plenty of time to > > get this fixed up. > > Our schedule for releasing hosted products is not necessarily aligned > with mainline kernel releases. And kernel developers don't really care about company schedules, nor should they, you know this :) thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 09:09:21 AM Greg KH wrote: > On Fri, Nov 30, 2012 at 08:47:46AM -0800, Andy King wrote: > > I didn't get the resend either, so it seems our corporate mail really is > > eating messages. Lovely. > > > > > > > +#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? > > > > Because we're morons. And unfortunately, we've shipped our product > > using those broken definitions: our VMX uses them to talk to the driver. > > So here's what we'd like to do. We will send out a patch soon that > > fixes the other issues you mention and also adds IOCTL definitions the > > proper way using _IOBLAH(). But we'd also like to retain these broken > > definitions for a short period, commented as such, at least until we > > can get out a patch release to Workstation 9, at which point we can > > remove them. Does that sound reasonable? > > It has been my experience, that when people say "We will remove that api > sometime in the future", it never happens. So why not just do it now? > > Especially given that this code will be coming out in 3.9 at the > earliest, and that is 6 months away, so that should be plenty of time to > get this fixed up. Our schedule for releasing hosted products is not necessarily aligned with mainline kernel releases. Also, thinking about it some more, maybe we should simply keep the old ioctls as is? Yes, they would not use the standard kernel style encoding for direction, etc, but the encoding only important if ioctl handler wants to parse and use it. Since we do not flat ioctl number is fine with us. That said we 'll clean up comments and numbers to remove stuff not applicable to mainline. Thanks, Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Monday, November 26, 2012 04:32:39 PM Greg KH wrote:
> On Mon, Nov 26, 2012 at 04:23:57PM -0800, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > For some reason it still didn't go through to our corporate mail server
> > but I see it on LKML.
>
> Good.
>
> > 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.
>
> Even on a 32bit processor?
I thought it would, but it looks like it won't. Maybe we'll just switch it
to a macro with C99 style initializators to keep the same semantic but
avoid the question.
> Also, you already have another function that
> does this same thing, so having 2 functions in the same patch seems odd,
> right?
Yes, you can say that it is probably a bit excessive.
OK, now that we are on the same page we'll go and fix the issues.
Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Mon, Oct 29, 2012 at 07:32:55PM -0700, Greg KH wrote: > On Mon, Oct 29, 2012 at 06:05:38PM -0700, George Zhang wrote: > > --- /dev/null > > +++ b/drivers/misc/vmw_vmci/Makefile > > @@ -0,0 +1,43 @@ > > + > > +# > > +# Linux driver for VMware's VMCI device. > > +# > > +# Copyright (C) 2007-2012, VMware, Inc. All Rights Reserved. > > +# > > +# This program is free software; you can redistribute it and/or modify it > > +# under the terms of the GNU General Public License as published by the > > +# Free Software Foundation; version 2 of the License and no later version. > > +# > > +# This program is distributed in the hope that it will be useful, but > > +# WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or > > +# NON INFRINGEMENT. See the GNU General Public License for more > > +# details. > > +# > > +# Maintained by: Andrew Stiegmann > > +# > > + > > That's a big boiler-plate for a makefile :) > > Wait, what's Andrew's name doing here, and yet it isn't on any of the > signed-off-by: or From: lines of the driver? Surely you aren't the only > contributor here? > > > +# > > +# Makefile for the VMware VMCI > > +# > > That's the only needed comment for this file, if even that. > > > + > > +obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci.o > > + > > +vmw_vmci-y += vmci_context.o > > +vmw_vmci-y += vmci_datagram.o > > +vmw_vmci-y += vmci_doorbell.o > > +vmw_vmci-y += vmci_driver.o > > +vmw_vmci-y += vmci_event.o > > +vmw_vmci-y += vmci_guest.o > > +vmw_vmci-y += vmci_handle_array.o > > +vmw_vmci-y += vmci_host.o > > +vmw_vmci-y += vmci_queue_pair.o > > +vmw_vmci-y += vmci_resource.o > > +vmw_vmci-y += vmci_route.o > > You can do this cleaner with multiple .o objects on the same line... > > > +vmci: > > + $(MAKE) -C ../../.. SUBDIRS=$$PWD CONFIG_VMWARE_VMCI=m modules > > + > > +clean: > > + $(MAKE) -C ../../.. SUBDIRS=$$PWD CONFIG_VMWARE_VMCI=m clean > > What are these two last targets for? I'm guessing this is from when you > ported it from a stand-along module? Please remove them. We'll clean it all up. Thanks for going over the code. -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
