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

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

On Tue, Dec 01, 2015 at 04:04:08PM -0800, Greg Kroah-Hartman
wrote:
> 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.
Maybe it's because I didn't CC you on the rest of the series.  I
wasn't
sure what the proper distribution list is for each part.

This new macro is also used in arch/x86/kernel/cpu/vmware.c and
vmw_balloon.c

> 
> > 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.
So the intent of this series is to centralize similar lines of inline
assembly code that are currently used by 3 different kernel modules
to a central place.  The new vmware.h [patch 0/6] becomes the one header
to include for common guest-host communication needs.

> thanks,
> 
> greg k-h

On 12/02/2015 01:04 AM, Greg Kroah-Hartman wrote:
> 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.
>
IMO, this makes a lot of sense because we now get a single definition of
VMW_PORT in the platform code that a developer can refer to to
understand things like 32-64 bit compatibilty, and usage conditions and
it also forces the developer to adopt the good practice of clearing
currently unused input variables rather than to leave them undefined. In
addition, if something needs to be changed we have one single place to
change rather than a lot of places scattered all over various kernel
modules.

Things that we (I) previously, for example, didn't get quite right in
the vmmouse module despite spending a considerable amount of time on the
subject.

Thanks,
Thomas

On Tue, Dec 01, 2015 at 06:21:06PM -0800, Sinclair Yeh
wrote:
> On Tue, Dec 01, 2015 at 04:04:08PM -0800, Greg Kroah-Hartman wrote:
> > 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.
> 
> Maybe it's because I didn't CC you on the rest of the series.  I
wasn't
> sure what the proper distribution list is for each part.
Use scripts/get_maintainer.pl, that's what it is there for.  A number of
those patches should go through me, if not all of them, if you want them
merged...
> 
> This new macro is also used in arch/x86/kernel/cpu/vmware.c and
> vmw_balloon.c
And it's used inconsistantly in those patches (you don't set the dummy
variables to 0 in all of them...)  Now maybe that's just how the asm
functions work, but it's not very obvious as to why this is at all.
> > > 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.
> 
> So the intent of this series is to centralize similar lines of inline
> assembly code that are currently used by 3 different kernel modules
> to a central place.  The new vmware.h [patch 0/6] becomes the one header
> to include for common guest-host communication needs.
Why can't it go into vmw_vmci_defs.h instead, or your other .h file, why
create yet-another-.h-file for your bus?  You already have 2, this would
make it 3, which seems like a lot...

thanks,

greg k-h