| Message ID | 1449008332-9394-3-git-send-email-syeh@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@vmware.com> > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> > Reviewed-by: Alok N Kataria <akataria@vmware.com> > Cc: pv-drivers@vmware.com > Cc: linux-graphics-maintainer@vmware.com > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-kernel@vger.kernel.org > Cc: virtualization@lists.linux-foundation.org > Cc: linux-input@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? > + VMW_PORT(in1, VMMOUSE_PROTO_CMD_##cmd, VMMOUSE_PROTO_PORT, \ > + VMMOUSE_PROTO_MAGIC, \ > + out1, out2, out3, out4, __dummy1, __dummy2); \ > }) > Thanks.
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@vmware.com> > > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> > > Reviewed-by: Alok N Kataria <akataria@vmware.com> > > Cc: pv-drivers@vmware.com > > Cc: linux-graphics-maintainer@vmware.com > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: linux-kernel@vger.kernel.org > > Cc: virtualization@lists.linux-foundation.org > > Cc: linux-input@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. So it's safer to initialize them to 0. Since they can potentially be an output, we can't make them a constant. > > > + VMW_PORT(in1, VMMOUSE_PROTO_CMD_##cmd, VMMOUSE_PROTO_PORT, \ > > + VMMOUSE_PROTO_MAGIC, \ > > + out1, out2, out3, out4, __dummy1, __dummy2); \ > > }) > > > > Thanks. > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <syeh@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@vmware.com> >> > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> >> > Reviewed-by: Alok N Kataria <akataria@vmware.com> >> > Cc: pv-drivers@vmware.com >> > Cc: linux-graphics-maintainer@vmware.com >> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> > Cc: Arnd Bergmann <arnd@arndb.de> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > Cc: linux-kernel@vger.kernel.org >> > Cc: virtualization@lists.linux-foundation.org >> > Cc: linux-input@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.
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 1, 2015 at 2:54 PM, Sinclair Yeh <syeh@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@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
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 Why are these here and not below the --- line? And no changelog text at all? Not acceptable :( greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 02, 2015 at 07:31:24AM -0800, Greg Kroah-Hartman wrote: > 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@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... Ok, thanks. Let me see if it make sense to use one of the existing 2 files. Either way, I'll respin this series to include all the comments so far. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 02, 2015 at 07:31:24AM -0800, Greg Kroah-Hartman wrote: > 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@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... Umm, you are not saying that vmmouse should include vmci header file(s), are you? Because the 2 are unrelated and vmci does not use the hypervisor port to communicate with host IIRC. Thanks.
On 12/02/2015 06:26 PM, Dmitry Torokhov wrote: > On Wed, Dec 02, 2015 at 07:31:24AM -0800, Greg Kroah-Hartman wrote: >> 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@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... > Umm, you are not saying that vmmouse should include vmci header file(s), > are you? Because the 2 are unrelated and vmci does not use the > hypervisor port to communicate with host IIRC. Also the platform setup code uses the hypervisor port, so it's a natural place for the macro defines. /Thomas > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 02, 2015 at 09:26:34AM -0800, Dmitry Torokhov wrote: > On Wed, Dec 02, 2015 at 07:31:24AM -0800, Greg Kroah-Hartman wrote: > > 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@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... > > Umm, you are not saying that vmmouse should include vmci header file(s), > are you? Because the 2 are unrelated and vmci does not use the > hypervisor port to communicate with host IIRC. vmmouse should include some type of "vmware bus" .h file, if it's not the vmw_* files, what are they for? My point being, I didn't see the need to add another .h file when we should probably already have one for this bus, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 02, 2015 at 10:45:28AM -0800, Greg Kroah-Hartman wrote: > On Wed, Dec 02, 2015 at 09:26:34AM -0800, Dmitry Torokhov wrote: > > On Wed, Dec 02, 2015 at 07:31:24AM -0800, Greg Kroah-Hartman wrote: > > > 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@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... > > > > Umm, you are not saying that vmmouse should include vmci header file(s), > > are you? Because the 2 are unrelated and vmci does not use the > > hypervisor port to communicate with host IIRC. > > vmmouse should include some type of "vmware bus" .h file, if it's not > the vmw_* files, what are they for? I see: dtor@dtor-ws:~/kernel/work$ ls include/linux/vmw* include/linux/vmw_vmci_api.h include/linux/vmw_vmci_defs.h so they are for VMCI. > My point being, I didn't see the > need to add another .h file when we should probably already have one for > this bus, right? You keep saying "bus" and I am confused. There is not really a bus there as (unfortunately) VMware did not come with a unified interface for host/guest communication, but rather let each group come up with their own way of doing things. So we have vmmouse and, as I was reminded, userspace agent using one virtual IO port to communicate, vmballoon is using another IO port, pvscsi, vmxnet3 and vmci are virtual PCI devices and Thomas can tell more about vmwgfx as I never looked at it closely. So if we want to consolidate the hypervisor port accessors (which I am not entirely convinced is worthwhile as the set of arguments/returned values/registers used depends on command sent through the port and these unified macros now require unneeded initializations of dummy variables) then we indeed need a new header. Thanks.
On Wed, Dec 02, 2015 at 10:45:28AM -0800, Greg Kroah-Hartman wrote: > On Wed, Dec 02, 2015 at 09:26:34AM -0800, Dmitry Torokhov wrote: > > On Wed, Dec 02, 2015 at 07:31:24AM -0800, Greg Kroah-Hartman wrote: > > > 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@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... > > > > Umm, you are not saying that vmmouse should include vmci header file(s), > > are you? Because the 2 are unrelated and vmci does not use the > > hypervisor port to communicate with host IIRC. > > vmmouse should include some type of "vmware bus" .h file, if it's not > the vmw_* files, what are they for? My point being, I didn't see the > need to add another .h file when we should probably already have one for > this bus, right? I had a brief chat with Thomas on this, and he added to Dmitry's reply earlier. The two existing *.h files are for the VMCI driver, and the new macros are used by the platform code and also other drivers like vmw_balloon and vmmouse. It makes more sense to create a new vmware.h file in arch/x86/include/asm Sinclair -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; \ + VMW_PORT(in1, VMMOUSE_PROTO_CMD_##cmd, VMMOUSE_PROTO_PORT, \ + VMMOUSE_PROTO_MAGIC, \ + out1, out2, out3, out4, __dummy1, __dummy2); \ }) /**