Virtualization - Dec 2015 - [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <syeh at vmware.com>
wrote:
> Hi,
>
> On Tue, Dec 01, 2015 at 02:24:14PM -0800, Dmitry Torokhov wrote:
>> On Tue, Dec 01, 2015 at 02:18:49PM -0800, Sinclair Yeh wrote:
>> > v2:
>> > Instead of replacing existing VMMOUSE defines, only modify enough
>> > to use the new VMW_PORT define.
>> >
>> > v3:
>> > Use updated VMWARE_PORT() which requires hypervisor magic as an
added
>> > parameter
>> >
>> > Signed-off-by: Sinclair Yeh <syeh at vmware.com>
>> > Reviewed-by: Thomas Hellstrom <thellstrom at vmware.com>
>> > Reviewed-by: Alok N Kataria <akataria at vmware.com>
>> > Cc: pv-drivers at vmware.com
>> > Cc: linux-graphics-maintainer at vmware.com
>> > Cc: Dmitry Torokhov <dmitry.torokhov at gmail.com>
>> > Cc: Arnd Bergmann <arnd at arndb.de>
>> > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> > Cc: linux-kernel at vger.kernel.org
>> > Cc: virtualization at lists.linux-foundation.org
>> > Cc: linux-input at vger.kernel.org
>> > ---
>> >  drivers/input/mouse/vmmouse.c | 22 +++++++---------------
>> >  1 file changed, 7 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/drivers/input/mouse/vmmouse.c
b/drivers/input/mouse/vmmouse.c
>> > index e272f06..d34e3e4 100644
>> > --- a/drivers/input/mouse/vmmouse.c
>> > +++ b/drivers/input/mouse/vmmouse.c
>> > @@ -19,6 +19,7 @@
>> >  #include <linux/slab.h>
>> >  #include <linux/module.h>
>> >  #include <asm/hypervisor.h>
>> > +#include <asm/vmware.h>
>> >
>> >  #include "psmouse.h"
>> >  #include "vmmouse.h"
>> > @@ -84,21 +85,12 @@ struct vmmouse_data {
>> >   * implementing the vmmouse protocol. Should never execute on
>> >   * bare metal hardware.
>> >   */
>> > -#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)      \
>> > -({                                                 \
>> > -   unsigned long __dummy1, __dummy2;               \
>> > -   __asm__ __volatile__ ("inl %%dx" :              \
>> > -           "=a"(out1),                             \
>> > -           "=b"(out2),                             \
>> > -           "=c"(out3),                             \
>> > -           "=d"(out4),                             \
>> > -           "=S"(__dummy1),                         \
>> > -           "=D"(__dummy2) :                        \
>> > -           "a"(VMMOUSE_PROTO_MAGIC),               \
>> > -           "b"(in1),                               \
>> > -           "c"(VMMOUSE_PROTO_CMD_##cmd),           \
>> > -           "d"(VMMOUSE_PROTO_PORT) :               \
>> > -           "memory");                              \
>> > +#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)            
\
>> > +({                                                            \
>> > +   unsigned long __dummy1 = 0, __dummy2 = 0;                  \
>>
>> Why do we need to initialize dummies?
>
> Because for some commands those parameters to VMW_PORT() can be both
> input and outout.
The vmmouse commands do not use them as input though, so it seems we
are simply wasting CPU cycles setting them to 0 just because we are
using the new VMW_PORT here. Why do we need to switch? What is the
benefit of doing this?

Thanks.

-- 
Dmitry

Hi,

On Tue, Dec 01, 2015 at 02:45:27PM -0800, Dmitry Torokhov
wrote:
> On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <syeh at vmware.com>
wrote:
> > Hi,
> >
<snip>
> >> >   */
> >> > -#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)      \
> >> > -({                                                 \
> >> > -   unsigned long __dummy1, __dummy2;               \
> >> > -   __asm__ __volatile__ ("inl %%dx" :             
\
> >> > -           "=a"(out1),                            
\
> >> > -           "=b"(out2),                            
\
> >> > -           "=c"(out3),                            
\
> >> > -           "=d"(out4),                            
\
> >> > -           "=S"(__dummy1),                        
\
> >> > -           "=D"(__dummy2) :                       
\
> >> > -           "a"(VMMOUSE_PROTO_MAGIC),              
\
> >> > -           "b"(in1),                              
\
> >> > -           "c"(VMMOUSE_PROTO_CMD_##cmd),          
\
> >> > -           "d"(VMMOUSE_PROTO_PORT) :              
\
> >> > -           "memory");                             
\
> >> > +#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)       
\
> >> > +({                                                          
\
> >> > +   unsigned long __dummy1 = 0, __dummy2 = 0;                
\
> >>
> >> Why do we need to initialize dummies?
> >
> > Because for some commands those parameters to VMW_PORT() can be both
> > input and outout.
> 
> The vmmouse commands do not use them as input though, so it seems we
> are simply wasting CPU cycles setting them to 0 just because we are
> using the new VMW_PORT here. Why do we need to switch? What is the
> benefit of doing this?
There are two reasons.  One is to make the code more readable and
maintainable.  Rather than having mostly similar inline assembly
code sprinkled across multiple modules, we can just use the macros
and document that.

The second reason is this organization makes some on-going future
development easier.

Hope this helps.

Sinclair

On Tue, Dec 1, 2015 at 2:54 PM, Sinclair Yeh <syeh at vmware.com>
wrote:
> Hi,
>
> On Tue, Dec 01, 2015 at 02:45:27PM -0800, Dmitry Torokhov wrote:
>> On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <syeh at vmware.com>
wrote:
>> > Hi,
>> >
>
> <snip>
>
>> >> >   */
>> >> > -#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)   
\
>> >> > -({                                                 \
>> >> > -   unsigned long __dummy1, __dummy2;               \
>> >> > -   __asm__ __volatile__ ("inl %%dx" :         
\
>> >> > -           "=a"(out1),                        
\
>> >> > -           "=b"(out2),                        
\
>> >> > -           "=c"(out3),                        
\
>> >> > -           "=d"(out4),                        
\
>> >> > -           "=S"(__dummy1),                    
\
>> >> > -           "=D"(__dummy2) :                   
\
>> >> > -           "a"(VMMOUSE_PROTO_MAGIC),          
\
>> >> > -           "b"(in1),                          
\
>> >> > -           "c"(VMMOUSE_PROTO_CMD_##cmd),      
\
>> >> > -           "d"(VMMOUSE_PROTO_PORT) :          
\
>> >> > -           "memory");                         
\
>> >> > +#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)   
\
>> >> > +({                                                      
\
>> >> > +   unsigned long __dummy1 = 0, __dummy2 = 0;            
\
>> >>
>> >> Why do we need to initialize dummies?
>> >
>> > Because for some commands those parameters to VMW_PORT() can be
both
>> > input and outout.
>>
>> The vmmouse commands do not use them as input though, so it seems we
>> are simply wasting CPU cycles setting them to 0 just because we are
>> using the new VMW_PORT here. Why do we need to switch? What is the
>> benefit of doing this?
>
> There are two reasons.  One is to make the code more readable and
> maintainable.  Rather than having mostly similar inline assembly
> code sprinkled across multiple modules, we can just use the macros
> and document that.
At the cost of wasting cycles though :(.

Oh well, it is not like we are polling the backdoor here, so if you do
not care about a few wasted cycles I don't have to either ;)
>
> The second reason is this organization makes some on-going future
> development easier.
>
> Hope this helps.
>
> Sinclair
-- 
Dmitry

On Tue, Dec 01, 2015 at 02:54:20PM -0800, Sinclair Yeh
wrote:
> Hi,
> 
> On Tue, Dec 01, 2015 at 02:45:27PM -0800, Dmitry Torokhov wrote:
> > On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <syeh at
vmware.com> wrote:
> > > Hi,
> > >
> 
> <snip>
> 
> > >> >   */
> > >> > -#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)  
\
> > >> > -({                                                 \
> > >> > -   unsigned long __dummy1, __dummy2;               \
> > >> > -   __asm__ __volatile__ ("inl %%dx" :        
\
> > >> > -           "=a"(out1),                       
\
> > >> > -           "=b"(out2),                       
\
> > >> > -           "=c"(out3),                       
\
> > >> > -           "=d"(out4),                       
\
> > >> > -           "=S"(__dummy1),                   
\
> > >> > -           "=D"(__dummy2) :                  
\
> > >> > -           "a"(VMMOUSE_PROTO_MAGIC),         
\
> > >> > -           "b"(in1),                         
\
> > >> > -           "c"(VMMOUSE_PROTO_CMD_##cmd),     
\
> > >> > -           "d"(VMMOUSE_PROTO_PORT) :         
\
> > >> > -           "memory");                        
\
> > >> > +#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)  
\
> > >> > +({                                                     
\
> > >> > +   unsigned long __dummy1 = 0, __dummy2 = 0;           
\
> > >>
> > >> Why do we need to initialize dummies?
> > >
> > > Because for some commands those parameters to VMW_PORT() can be
both
> > > input and outout.
> > 
> > The vmmouse commands do not use them as input though, so it seems we
> > are simply wasting CPU cycles setting them to 0 just because we are
> > using the new VMW_PORT here. Why do we need to switch? What is the
> > benefit of doing this?
> 
> There are two reasons.  One is to make the code more readable and
> maintainable.  Rather than having mostly similar inline assembly
> code sprinkled across multiple modules, we can just use the macros
> and document that.
But the macro is only used here, and the variables aren't used at all,
so it makes no sense in this file.
> The second reason is this organization makes some on-going future
> development easier.
We don't plan for "future" development other than a single patch
series,
as we have no idea what that development is, nor if it will really
happen.  You can always change this file later if you need to, nothing
is keeping that from happening.

thanks,

greg k-h