Re: [PATCH v2 9/9] dt-bindings: drop redundant part of title (manual)

[Dmitry Torokhov]

On Mon, Nov 21, 2022 at 12:06:15PM +0100, Krzysztof Kozlowski wrote:
>  Documentation/devicetree/bindings/input/fsl,scu-key.yaml| 2 +-
>  Documentation/devicetree/bindings/input/matrix-keymap.yaml  | 2 +-

Acked-by: Dmitry Torokhov 

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 7/9] dt-bindings: drop redundant part of title (beginning)

[Dmitry Torokhov]

On Mon, Nov 21, 2022 at 12:06:13PM +0100, Krzysztof Kozlowski wrote:
>  Documentation/devicetree/bindings/input/gpio-keys.yaml  | 2 +-
>  Documentation/devicetree/bindings/input/microchip,cap11xx.yaml  | 2 +-

Acked-by: Dmitry Torokhov 

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 6/9] dt-bindings: drop redundant part of title (end, part three)

[Dmitry Torokhov]

On Mon, Nov 21, 2022 at 12:06:12PM +0100, Krzysztof Kozlowski wrote:
>  .../bindings/input/touchscreen/cypress,cy8ctma140.yaml  | 2 +-
>  .../bindings/input/touchscreen/cypress,cy8ctma340.yaml  | 2 +-
>  .../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml   | 2 +-
>  Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 2 +-
>  .../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml   | 2 +-
>  .../devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml | 2 +-
>  .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml  | 2 +-
>  .../devicetree/bindings/input/touchscreen/melfas,mms114.yaml| 2 +-
>  .../devicetree/bindings/input/touchscreen/mstar,msg2638.yaml| 2 +-
>  .../devicetree/bindings/input/touchscreen/ti,tsc2005.yaml   | 2 +-
>  .../devicetree/bindings/input/touchscreen/touchscreen.yaml  | 2 +-
>  .../devicetree/bindings/input/touchscreen/zinitix,bt400.yaml    | 2 +-

Acked-by: Dmitry Torokhov 

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/9] dt-bindings: drop redundant part of title (end)

[Dmitry Torokhov]

On Mon, Nov 21, 2022 at 12:06:10PM +0100, Krzysztof Kozlowski wrote:
>  .../devicetree/bindings/input/pine64,pinephone-keyboard.yaml| 2 +-
>  .../devicetree/bindings/input/touchscreen/chipone,icn8318.yaml  | 2 +-
>  .../devicetree/bindings/input/touchscreen/pixcir,pixcir_ts.yaml | 2 +-
>  .../devicetree/bindings/input/touchscreen/silead,gsl1680.yaml   | 2 +-

Acked-by: Dmitry Torokhov 

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RESEND v2] virtio-input: add multi-touch support

[Dmitry Torokhov]

Hi Vasyl,

On Tue, Dec 08, 2020 at 11:01:50PM +0200, Vasyl Vavrychuk wrote:
> From: Mathias Crombez 
> 
> Without multi-touch slots allocated, ABS_MT_SLOT events will be lost by
> input_handle_abs_event.
> 
> Signed-off-by: Mathias Crombez 
> Signed-off-by: Vasyl Vavrychuk 
> Tested-by: Vasyl Vavrychuk 
> ---
> v2: fix patch corrupted by corporate email server
> 
>  drivers/virtio/Kconfig| 11 +++
>  drivers/virtio/virtio_input.c |  8 
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 7b41130d3f35..2cfd5b01d96d 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -111,6 +111,17 @@ config VIRTIO_INPUT
>  
>If unsure, say M.
>  
> +config VIRTIO_INPUT_MULTITOUCH_SLOTS
> + depends on VIRTIO_INPUT
> + int "Number of multitouch slots"
> + range 0 64
> + default 10
> + help
> +  Define the number of multitouch slots used. Default to 10.
> +  This parameter is unused if there is no multitouch capability.

I believe the number of slots should be communicated to the guest by
the host, similarly to how the rest of input device capabilities is
transferred, instead of having static compile-time option.

> +
> +  0 will disable the feature.
> +
>  config VIRTIO_MMIO
>   tristate "Platform bus driver for memory mapped virtio devices"
>   depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index f1f6208edcf5..13f3d90e6c30 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -7,6 +7,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  struct virtio_input {
>   struct virtio_device   *vdev;
> @@ -205,6 +206,7 @@ static int virtinput_probe(struct virtio_device *vdev)
>   unsigned long flags;
>   size_t size;
>   int abs, err;
> + bool is_mt = false;
>  
>   if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>   return -ENODEV;
> @@ -287,9 +289,15 @@ static int virtinput_probe(struct virtio_device *vdev)
>   for (abs = 0; abs < ABS_CNT; abs++) {
>   if (!test_bit(abs, vi->idev->absbit))
>   continue;
> + if (input_is_mt_value(abs))
> + is_mt = true;
>   virtinput_cfg_abs(vi, abs);
>   }
>   }
> + if (is_mt)
> + input_mt_init_slots(vi->idev,
> + CONFIG_VIRTIO_INPUT_MULTITOUCH_SLOTS,
> + INPUT_MT_DIRECT);

Here errors need to be handled.

>  
>   virtio_device_ready(vdev);
>   vi->ready = true;
> -- 
> 2.23.0
> 

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/5] x86: add enum for hypervisors to replace x86_hyper

[Dmitry Torokhov]

On Thu, Nov 09, 2017 at 02:27:36PM +0100, Juergen Gross wrote:
> The x86_hyper pointer is only used for checking whether a virtual
> device is supporting the hypervisor the system is running on.
> 
> Use an enum for that purpose instead and drop the x86_hyper pointer.
> 
> Cc: k...@microsoft.com
> Cc: haiya...@microsoft.com
> Cc: sthem...@microsoft.com
> Cc: akata...@vmware.com
> Cc: pbonz...@redhat.com
> Cc: rkrc...@redhat.com
> Cc: boris.ostrov...@oracle.com
> Cc: de...@linuxdriverproject.org
> Cc: virtualization@lists.linux-foundation.org
> Cc: k...@vger.kernel.org
> Cc: xen-de...@lists.xenproject.org
> Cc: linux-graphics-maintai...@vmware.com
> Cc: pv-driv...@vmware.com
> Cc: dmitry.torok...@gmail.com
> Cc: xdeguill...@vmware.com
> Cc: moltm...@vmware.com
> Cc: a...@arndb.de
> Cc: gre...@linuxfoundation.org
> Cc: linux-in...@vger.kernel.org
> 
> Signed-off-by: Juergen Gross <jgr...@suse.com>
> ---
>  arch/x86/hyperv/hv_init.c |  2 +-
>  arch/x86/include/asm/hypervisor.h | 23 ++-
>  arch/x86/kernel/cpu/hypervisor.c  | 12 +---
>  arch/x86/kernel/cpu/mshyperv.c|  4 ++--
>  arch/x86/kernel/cpu/vmware.c  |  4 ++--
>  arch/x86/kernel/kvm.c |  4 ++--
>  arch/x86/xen/enlighten_hvm.c  |  4 ++--
>  arch/x86/xen/enlighten_pv.c   |  4 ++--
>  drivers/hv/vmbus_drv.c|  2 +-
>  drivers/input/mouse/vmmouse.c | 10 --

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

>  drivers/misc/vmw_balloon.c|  2 +-
>  11 files changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index a5db63f728a2..a0b86cf486e0 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -113,7 +113,7 @@ void hyperv_init(void)
>   u64 guest_id;
>   union hv_x64_msr_hypercall_contents hypercall_msr;
>  
> - if (x86_hyper != _hyper_ms_hyperv)
> + if (x86_hyper_type != X86_HYPER_MS_HYPERV)
>   return;
>  
>   /* Allocate percpu VP index */
> diff --git a/arch/x86/include/asm/hypervisor.h 
> b/arch/x86/include/asm/hypervisor.h
> index 0eca7239a7aa..1b0a5abcd8ae 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -29,6 +29,16 @@
>  /*
>   * x86 hypervisor information
>   */
> +
> +enum x86_hypervisor_type {
> + X86_HYPER_NATIVE = 0,
> + X86_HYPER_VMWARE,
> + X86_HYPER_MS_HYPERV,
> + X86_HYPER_XEN_PV,
> + X86_HYPER_XEN_HVM,
> + X86_HYPER_KVM,
> +};
> +
>  struct hypervisor_x86 {
>   /* Hypervisor name */
>   const char  *name;
> @@ -36,6 +46,9 @@ struct hypervisor_x86 {
>   /* Detection routine */
>   uint32_t(*detect)(void);
>  
> + /* Hypervisor type */
> + enum x86_hypervisor_type type;
> +
>   /* init time callbacks */
>   struct x86_hyper_init init;
>  
> @@ -43,15 +56,7 @@ struct hypervisor_x86 {
>   struct x86_hyper_runtime runtime;
>  };
>  
> -extern const struct hypervisor_x86 *x86_hyper;
> -
> -/* Recognized hypervisors */
> -extern const struct hypervisor_x86 x86_hyper_vmware;
> -extern const struct hypervisor_x86 x86_hyper_ms_hyperv;
> -extern const struct hypervisor_x86 x86_hyper_xen_pv;
> -extern const struct hypervisor_x86 x86_hyper_xen_hvm;
> -extern const struct hypervisor_x86 x86_hyper_kvm;
> -
> +extern enum x86_hypervisor_type x86_hyper_type;
>  extern void init_hypervisor_platform(void);
>  #else
>  static inline void init_hypervisor_platform(void) { }
> diff --git a/arch/x86/kernel/cpu/hypervisor.c 
> b/arch/x86/kernel/cpu/hypervisor.c
> index 6c1bf092..bea8d3e24f50 100644
> --- a/arch/x86/kernel/cpu/hypervisor.c
> +++ b/arch/x86/kernel/cpu/hypervisor.c
> @@ -26,6 +26,12 @@
>  #include 
>  #include 
>  
> +extern const struct hypervisor_x86 x86_hyper_vmware;
> +extern const struct hypervisor_x86 x86_hyper_ms_hyperv;
> +extern const struct hypervisor_x86 x86_hyper_xen_pv;
> +extern const struct hypervisor_x86 x86_hyper_xen_hvm;
> +extern const struct hypervisor_x86 x86_hyper_kvm;
> +
>  static const __initconst struct hypervisor_x86 * const hypervisors[] =
>  {
>  #ifdef CONFIG_XEN_PV
> @@ -41,8 +47,8 @@ static const __initconst struct hypervisor_x86 * const 
> hypervisors[] =
>  #endif
>  };
>  
> -const struct hypervisor_x86 *x86_hyper;
> -EXPORT_SYMBOL(x86_hyper);
> +enum x86_hypervisor_type x86_hyper_type;
> +EXPORT_SYMBOL(x86_hyper_type);
>  
>  static inline const struct hypervisor_x86 * __init
>  detect_hypervisor_vendor(void)
> @@ -87,6 +93,6 @@ void __init init_hypervisor_platform(void)
&g

Re: [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

[Dmitry Torokhov]

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 <s...@vmware.com> wrote:
> > > > > > Hi,
> > > > > >
> > > > 
> > > > 
> > > > 
> > > > > >> >   */
> > > > > >> > -#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.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

[Dmitry Torokhov]

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 <s...@vmware.com> 
> > > > > > > wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > >> >   */
> > > > > > > >> > -#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...
> &g

Re: [PATCH 4/6] Input: Remove vmmouse port reservation

[Dmitry Torokhov]

Hi Sinclair,

On Tue, Dec 1, 2015 at 2:18 PM, Sinclair Yeh  wrote:
> Port reservation is not required.

You need to expand on why we do not need to reserve port.

> Furthermore, this port is shared
> by other VMware services for host-side communication.

What services would that be? Do they reserve the port?

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

[Dmitry Torokhov]

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 <s...@vmware.com>
> Reviewed-by: Thomas Hellstrom <thellst...@vmware.com>
> Reviewed-by: Alok N Kataria <akata...@vmware.com>
> Cc: pv-driv...@vmware.com
> Cc: linux-graphics-maintai...@vmware.com
> Cc: Dmitry Torokhov <dmitry.torok...@gmail.com>
> Cc: Arnd Bergmann <a...@arndb.de>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: linux-ker...@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-in...@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 
>  #include 
>  #include 
> +#include 
>  
>  #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.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

[Dmitry Torokhov]

On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <s...@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 <s...@vmware.com>
>> > Reviewed-by: Thomas Hellstrom <thellst...@vmware.com>
>> > Reviewed-by: Alok N Kataria <akata...@vmware.com>
>> > Cc: pv-driv...@vmware.com
>> > Cc: linux-graphics-maintai...@vmware.com
>> > Cc: Dmitry Torokhov <dmitry.torok...@gmail.com>
>> > Cc: Arnd Bergmann <a...@arndb.de>
>> > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>> > Cc: linux-ker...@vger.kernel.org
>> > Cc: virtualization@lists.linux-foundation.org
>> > Cc: linux-in...@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 
>> >  #include 
>> >  #include 
>> > +#include 
>> >
>> >  #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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

[Dmitry Torokhov]

On Tue, Dec 1, 2015 at 2:54 PM, Sinclair Yeh <s...@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 <s...@vmware.com> wrote:
>> > Hi,
>> >
>
> 
>
>> >> >   */
>> >> > -#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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/6] Input: Remove vmmouse port reservation

[Dmitry Torokhov]

On Tue, Dec 1, 2015 at 3:04 PM, Sinclair Yeh <s...@vmware.com> wrote:
> Hi,
>
> On Tue, Dec 01, 2015 at 02:30:05PM -0800, Dmitry Torokhov wrote:
>> Hi Sinclair,
>>
>> On Tue, Dec 1, 2015 at 2:18 PM, Sinclair Yeh <s...@vmware.com> wrote:
>> > Port reservation is not required.
>>
>> You need to expand on why we do not need to reserve port.
>
> Thomas gave me this input earlier, too, so I added the one liner.
>
> There was a long discussion on accessing the port a few years ago:
> https://lkml.org/lkml/2008/9/24/512
>
>>
>> > Furthermore, this port is shared
>> > by other VMware services for host-side communication.
>>
>> What services would that be? Do they reserve the port?
>
> This port is used by quite a few guest-to-host communication capabilities,
> e.g. getting configuration, logging, etc.  Currently multiple kernel
> modules, and one or more priviledged guest user mode app, e.g.
> open-vmware-tools, use this port without reservation.

Ah, I forgot that vmmouse does not have a dedicated port...

>
> After some internal discussions, it was determined that no reservation
> is required when accessing the port in this manner.
>
> Do you want me to put the above in the commit message?

Not about the bit about "internal discussions", but the rest - yes please.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Dmitry Torokhov]

On Tue, Mar 24, 2015 at 04:25:14PM +0100, Gerd Hoffmann wrote:
 On Di, 2015-03-24 at 15:14 +0100, Michael S. Tsirkin wrote:
  On Tue, Mar 24, 2015 at 02:37:24PM +0100, Gerd Hoffmann wrote:
 Hi,
   
 input layer checks it and ignores events not supported (according to 
 the
 support bitmaps).

Right but support bitmaps come from host too, no?
   
   Yes, but the driver will not set invalid bits (bitcount argument for the
   virtinput_cfg_bits() function is the number of valid bits of the
   specific bitmap).
   
   cheers,
 Gerd
   
   
   
  
  Question: does linux ever get such events from userspace
  as opposed to sending them to userspace?
 
 Yes, it's possible using the userspace input driver
 (CONFIG_INPUT_UINPUT)

No, not through uinput (as from kernel POV uinput is also a driver), but
users can write into evdev.

Sending unknown codes is OK: events of unknown type will be dropped by
the input core, unknown event codes will be passed on; users not
recognizing event code should simply ignore it.

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3] Add virtio-input driver.

[Dmitry Torokhov]

On Tue, Mar 24, 2015 at 11:36:31AM +0100, Michael S. Tsirkin wrote:
 On Tue, Mar 24, 2015 at 08:32:01AM +0100, Gerd Hoffmann wrote:
  virtio-input is basically evdev-events-over-virtio, so this driver isn't
  much more than reading configuration from config space and forwarding
  incoming events to the linux input layer.
  
  Signed-off-by: Gerd Hoffmann kra...@redhat.com
 
 Looks pretty neat overall. I think I still see some
 small issues, but it's getting there.
 
  ---
   MAINTAINERS   |   6 +
   drivers/virtio/Kconfig|  10 ++
   drivers/virtio/Makefile   |   1 +
   drivers/virtio/virtio_input.c | 313 
  ++
   include/uapi/linux/Kbuild |   1 +
   include/uapi/linux/virtio_ids.h   |   1 +
   include/uapi/linux/virtio_input.h |  76 +
   7 files changed, 408 insertions(+)
   create mode 100644 drivers/virtio/virtio_input.c
   create mode 100644 include/uapi/linux/virtio_input.h
  
  diff --git a/MAINTAINERS b/MAINTAINERS
  index 358eb01..6f233dd 100644
  --- a/MAINTAINERS
  +++ b/MAINTAINERS
  @@ -10442,6 +10442,12 @@ S: Maintained
   F: drivers/vhost/
   F: include/uapi/linux/vhost.h
   
  +VIRTIO INPUT DRIVER
  +M: Gerd Hoffmann kra...@redhat.com
  +S: Maintained
  +F: drivers/virtio/virtio_input.c
  +F: include/uapi/linux/virtio_input.h
  +
   VIA RHINE NETWORK DRIVER
   M: Roger Luethi r...@hellgate.ch
   S: Maintained
  diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
  index b546da5..cab9f3f 100644
  --- a/drivers/virtio/Kconfig
  +++ b/drivers/virtio/Kconfig
  @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
   
   If unsure, say M.
   
  +config VIRTIO_INPUT
  +   tristate Virtio input driver
  +   depends on VIRTIO
  +   depends on INPUT
  +   ---help---
  +This driver supports virtio input devices such as
  +keyboards, mice and tablets.
  +
  +If unsure, say M.
  +
config VIRTIO_MMIO
  tristate Platform bus driver for memory mapped virtio devices
  depends on HAS_IOMEM
  diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
  index d85565b..41e30e3 100644
  --- a/drivers/virtio/Makefile
  +++ b/drivers/virtio/Makefile
  @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
   virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
   virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
   obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
  +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
  diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
  new file mode 100644
  index 000..cf112b2
  --- /dev/null
  +++ b/drivers/virtio/virtio_input.c
  @@ -0,0 +1,313 @@
  +#include linux/module.h
  +#include linux/virtio.h
  +#include linux/input.h
  +
  +#include uapi/linux/virtio_ids.h
  +#include uapi/linux/virtio_input.h
  +
  +struct virtio_input {
  +   struct virtio_device   *vdev;
  +   struct input_dev   *idev;
  +   char   name[64];
  +   char   serial[64];
  +   char   phys[64];
  +   struct virtqueue   *evt, *sts;
  +   struct virtio_input_event  evts[64];
  +   spinlock_t lock;
  +};
  +
  +static void virtinput_queue_evtbuf(struct virtio_input *vi,
  +  struct virtio_input_event *evtbuf)
  +{
  +   struct scatterlist sg[1];
  +
  +   sg_init_one(sg, evtbuf, sizeof(*evtbuf));
  +   virtqueue_add_inbuf(vi-evt, sg, 1, evtbuf, GFP_ATOMIC);
  +}
  +
  +static void virtinput_recv_events(struct virtqueue *vq)
  +{
  +   struct virtio_input *vi = vq-vdev-priv;
  +   struct virtio_input_event *event;
  +   unsigned long flags;
  +   unsigned int len;
  +
  +   spin_lock_irqsave(vi-lock, flags);
  +   while ((event = virtqueue_get_buf(vi-evt, len)) != NULL) {
  +   input_event(vi-idev,
  +   le16_to_cpu(event-type),
  +   le16_to_cpu(event-code),
  +   le32_to_cpu(event-value));
 
 What happens if input layer gets an
 unexpected event code or value?
 Or does something prevent it?
 
 
 
  +   virtinput_queue_evtbuf(vi, event);
  +   }
  +   virtqueue_kick(vq);
  +   spin_unlock_irqrestore(vi-lock, flags);
  +}
  +
  +static int virtinput_send_status(struct virtio_input *vi,
  +u16 type, u16 code, s32 value)
  +{
  +   struct virtio_input_event *stsbuf;
  +   struct scatterlist sg[1];
  +   unsigned long flags;
  +   int rc;
  +
  +   stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
  +   if (!stsbuf)
  +   return -ENOMEM;
  +
  +   stsbuf-type  = cpu_to_le16(type);
  +   stsbuf-code  = cpu_to_le16(code);
  +   stsbuf-value = cpu_to_le32(value);
  +   sg_init_one(sg, stsbuf, sizeof(*stsbuf));
  +
  +   spin_lock_irqsave(vi-lock, flags);
  +   rc = virtqueue_add_outbuf(vi-sts, sg, 1, stsbuf, GFP_ATOMIC);
  +   virtqueue_kick(vi-sts);
  +   spin_unlock_irqrestore(vi-lock, flags);

I think locking is wrong here. 

Re: [PATCH v2] Add virtio-input driver.

[Dmitry Torokhov]

Hi Gerd,

On Fri, Mar 20, 2015 at 01:39:29PM +0100, Gerd Hoffmann wrote:
 virtio-input is basically evdev-events-over-virtio, so this driver isn't
 much more than reading configuration from config space and forwarding
 incoming events to the linux input layer.
 
 Signed-off-by: Gerd Hoffmann kra...@redhat.com
 ---
  MAINTAINERS   |   6 +
  drivers/virtio/Kconfig|  10 ++
  drivers/virtio/Makefile   |   1 +
  drivers/virtio/virtio_input.c | 335 
 ++
  include/uapi/linux/Kbuild |   1 +
  include/uapi/linux/virtio_ids.h   |   1 +
  include/uapi/linux/virtio_input.h |  75 +
  7 files changed, 429 insertions(+)
  create mode 100644 drivers/virtio/virtio_input.c
  create mode 100644 include/uapi/linux/virtio_input.h
 
 diff --git a/MAINTAINERS b/MAINTAINERS
 index 0e1abe8..585e6cd 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -10435,6 +10435,12 @@ S:   Maintained
  F:   drivers/vhost/
  F:   include/uapi/linux/vhost.h
  
 +VIRTIO INPUT DRIVER
 +M:   Gerd Hoffmann kra...@redhat.com
 +S:   Maintained
 +F:   drivers/virtio/virtio_input.c
 +F:   include/uapi/linux/virtio_input.h
 +
  VIA RHINE NETWORK DRIVER
  M:   Roger Luethi r...@hellgate.ch
  S:   Maintained
 diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
 index b546da5..cab9f3f 100644
 --- a/drivers/virtio/Kconfig
 +++ b/drivers/virtio/Kconfig
 @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
  
If unsure, say M.
  
 +config VIRTIO_INPUT
 + tristate Virtio input driver
 + depends on VIRTIO
 + depends on INPUT
 + ---help---
 +  This driver supports virtio input devices such as
 +  keyboards, mice and tablets.
 +
 +  If unsure, say M.
 +
   config VIRTIO_MMIO
   tristate Platform bus driver for memory mapped virtio devices
   depends on HAS_IOMEM
 diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
 index d85565b..41e30e3 100644
 --- a/drivers/virtio/Makefile
 +++ b/drivers/virtio/Makefile
 @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
 +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
 diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
 new file mode 100644
 index 000..dd3215e
 --- /dev/null
 +++ b/drivers/virtio/virtio_input.c
 @@ -0,0 +1,335 @@
 +#include linux/module.h
 +#include linux/virtio.h
 +#include linux/input.h
 +
 +#include uapi/linux/virtio_ids.h
 +#include uapi/linux/virtio_input.h
 +
 +struct virtio_input {
 + struct virtio_device   *vdev;
 + struct input_dev   *idev;
 + char   name[64];
 + char   serial[64];
 + char   phys[64];
 + struct virtqueue   *evt, *sts;
 + struct virtio_input_event  evts[64];
 + struct mutex   lock;
 +};
 +
 +static ssize_t serial_show(struct device *dev,
 +struct device_attribute *attr, char *buf)
 +{
 + struct input_dev *idev = to_input_dev(dev);
 + struct virtio_input *vi = input_get_drvdata(idev);
 +
 + return sprintf(buf, %s\n, vi-serial);
 +}
 +static DEVICE_ATTR_RO(serial);

Since serial is uniq and uniq is already exposed in sysfs by input core
please remove this attribute (and the rest of attribute group handling).

 +
 +static struct attribute *dev_attrs[] = {
 + dev_attr_serial.attr,
 + NULL
 +};
 +
 +static umode_t dev_attrs_are_visible(struct kobject *kobj,
 +  struct attribute *a, int n)
 +{
 + struct device *dev = container_of(kobj, struct device, kobj);
 + struct input_dev *idev = to_input_dev(dev);
 + struct virtio_input *vi = input_get_drvdata(idev);
 +
 + if (a == dev_attr_serial.attr  !strlen(vi-serial))
 + return 0;
 +
 + return a-mode;
 +}
 +
 +static struct attribute_group dev_attr_grp = {
 + .attrs =dev_attrs,
 + .is_visible =   dev_attrs_are_visible,
 +};
 +
 +static const struct attribute_group *dev_attr_groups[] = {
 + dev_attr_grp,
 + NULL
 +};
 +
 +static void virtinput_queue_evtbuf(struct virtio_input *vi,
 +struct virtio_input_event *evtbuf)
 +{
 + struct scatterlist sg[1];
 +
 + sg_init_one(sg, evtbuf, sizeof(*evtbuf));
 + virtqueue_add_inbuf(vi-evt, sg, 1, evtbuf, GFP_ATOMIC);
 +}
 +
 +static void virtinput_recv_events(struct virtqueue *vq)
 +{
 + struct virtio_input *vi = vq-vdev-priv;
 + struct virtio_input_event *event;
 + unsigned int len;
 +
 + mutex_lock(vi-lock);
 + while ((event = virtqueue_get_buf(vi-evt, len)) != NULL) {
 + input_event(vi-idev,
 + le16_to_cpu(event-type),
 + le16_to_cpu(event-code),
 + 

Re: [PATCH 1/1] Add virtio-input driver.

[Dmitry Torokhov]

On Thu, Mar 19, 2015 at 01:29:49PM +0100, David Herrmann wrote:
 Hi
 
 (CC: Dmitry)
 
 On Thu, Mar 19, 2015 at 10:13 AM, Gerd Hoffmann kra...@redhat.com wrote:
  virtio-input is basically evdev-events-over-virtio, so this driver isn't
  much more than reading configuration from config space and forwarding
  incoming events to the linux input layer.
 
  Signed-off-by: Gerd Hoffmann kra...@redhat.com
  ---
   drivers/virtio/Kconfig|  10 ++
   drivers/virtio/Makefile   |   1 +
   drivers/virtio/virtio_input.c | 313 
  ++
   include/uapi/linux/virtio_ids.h   |   1 +
   include/uapi/linux/virtio_input.h |  65 
   5 files changed, 390 insertions(+)
   create mode 100644 drivers/virtio/virtio_input.c
   create mode 100644 include/uapi/linux/virtio_input.h
 
  diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
  index b546da5..cab9f3f 100644
  --- a/drivers/virtio/Kconfig
  +++ b/drivers/virtio/Kconfig
  @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
 
   If unsure, say M.
 
  +config VIRTIO_INPUT
  +   tristate Virtio input driver
  +   depends on VIRTIO
  +   depends on INPUT
  +   ---help---
  +This driver supports virtio input devices such as
  +keyboards, mice and tablets.
  +
  +If unsure, say M.
  +
config VIRTIO_MMIO
  tristate Platform bus driver for memory mapped virtio devices
  depends on HAS_IOMEM
  diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
  index d85565b..41e30e3 100644
  --- a/drivers/virtio/Makefile
  +++ b/drivers/virtio/Makefile
  @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
   virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
   virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
   obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
  +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
  diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
  new file mode 100644
  index 000..2d425cf
  --- /dev/null
  +++ b/drivers/virtio/virtio_input.c
  @@ -0,0 +1,313 @@
  +#include linux/module.h
  +#include linux/virtio.h
  +#include linux/input.h
  +
  +#include uapi/linux/virtio_ids.h
  +#include uapi/linux/virtio_input.h
  +
  +struct virtio_input {
  +   struct virtio_device   *vdev;
  +   struct input_dev   *idev;
  +   char   name[64];
  +   char   serial[64];
  +   char   phys[64];
  +   struct virtqueue   *evt, *sts;
  +   struct virtio_input_event  evts[64];
  +};
  +
  +static ssize_t serial_show(struct device *dev,
  +  struct device_attribute *attr, char *buf)
  +{
  +   struct input_dev *idev = to_input_dev(dev);
  +   struct virtio_input *vi = input_get_drvdata(idev);
  +   return sprintf(buf, %s\n, vi-serial);
  +}
  +static DEVICE_ATTR_RO(serial);

What is serial? Serial number?

  +
  +static struct attribute *dev_attrs[] = {
  +   dev_attr_serial.attr,
  +   NULL
  +};
  +
  +static umode_t dev_attrs_are_visible(struct kobject *kobj,
  +struct attribute *a, int n)
  +{
  +   struct device *dev = container_of(kobj, struct device, kobj);
  +   struct input_dev *idev = to_input_dev(dev);
  +   struct virtio_input *vi = input_get_drvdata(idev);
  +
  +   if (a == dev_attr_serial.attr  !strlen(vi-serial))
  +   return 0;
  +
  +   return a-mode;
  +}
  +
  +static struct attribute_group dev_attr_grp = {
  +   .attrs =dev_attrs,
  +   .is_visible =   dev_attrs_are_visible,
  +};
  +
  +static const struct attribute_group *dev_attr_groups[] = {
  +   dev_attr_grp,
  +   NULL
  +};
  +
  +static void virtinput_queue_evtbuf(struct virtio_input *vi,
  +  struct virtio_input_event *evtbuf)
  +{
  +   struct scatterlist sg[1];
  +
  +   sg_init_one(sg, evtbuf, sizeof(*evtbuf));
  +   virtqueue_add_inbuf(vi-evt, sg, 1, evtbuf, GFP_ATOMIC);
  +}
  +
  +static void virtinput_recv_events(struct virtqueue *vq)
  +{
  +   struct virtio_input *vi = vq-vdev-priv;
  +   struct virtio_input_event *event;
  +   unsigned int len;
  +
  +   while ((event = virtqueue_get_buf(vi-evt, len)) != NULL) {
  +   input_event(vi-idev,
  +   le16_to_cpu(event-type),
  +   le16_to_cpu(event-code),
  +   le32_to_cpu(event-value));
  +   virtinput_queue_evtbuf(vi, event);
  +   }
  +   virtqueue_kick(vq);
  +}
  +
  +static int virtinput_send_status(struct virtio_input *vi,
  +u16 type, u16 code, s32 value)
  +{
  +   struct virtio_input_event *stsbuf;
  +   struct scatterlist sg[1];
  +
  +   stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
  +   if (!stsbuf)
  +   

Re: [Pv-drivers] [PATCH 0/1] VM Sockets for Linux upstreaming

[Dmitry Torokhov]

Hi David,

On Wed, Feb 06, 2013 at 04:23:55PM -0800, Andy King wrote:
 In an effort to improve the out-of-the-box experience with Linux kernels for
 VMware users, VMware is working on readying the VM Sockets (VSOCK, formerly
 VMCI Sockets) (vsock) kernel module for inclusion in the Linux kernel. The
 purpose of this post is to acquire feedback on the vsock kernel module.
 
 Unlike previous submissions, where the new socket family was entirely reliant
 on VMware's VMCI PCI device (and thus VMware's hypervisor), VM Sockets is now
 completely[1] separated out into two parts, each in its own module:
 
 o Core socket code, which is transport-neutral and invokes transport
   callbacks to communicate with the hypervisor.  This is vsock.ko.
 o A VMCI transport, which communicates over VMCI with the VMware hypervisor.
   This is vmw_vsock_vmci_transport.ko, and it registers with the core module
   as a transport.
 
 This should provide a path to introducing additional transports, for example
 virtio, with the ultimate goal being to make this new socket family
 hypervisor-neutral.

As Andy mentioned in another e-mail, we would like very much to get
vsock in 3.9 release, so now that it is split into hypervisor neutral
and transport parts is there any high level issues that we need to
resolve before the code can be accepted?

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 1/1] VSOCK: Introduce VM Sockets

[Dmitry Torokhov]

Hi Neil,

On Friday, January 25, 2013 06:59:53 PM Neil Horman wrote:
 On Fri, Jan 25, 2013 at 09:37:50AM -0800, ack...@vmware.com wrote:
  +
  +config VMWARE_VSOCK
  +   tristate Virtual Socket protocol
  +   depends on VMWARE_VMCI
 
 What is CONFIG_VMWARE_VMCI?  I don't find that in any Kconfig in the tree?
 
 I''m still looking over the rest, but I get build issues if I just remove
 the dependency.

VMCI is in linux-next at the moment.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 07/12] VMCI: queue pairs implementation.

[Dmitry Torokhov]

Hi Greg,

On Tuesday, January 08, 2013 04:15:39 PM Greg KH wrote:
 On Tue, Jan 08, 2013 at 03:54:54PM -0800, George Zhang wrote:
 
  +/* Guest device port I/O. */
  +struct PPNSet {
  +   u64 num_produce_pages;
  +   u64 num_consume_pages;
  +   u32 *produce_ppns;
  +   u32 *consume_ppns;
  +   bool initialized;
  +};
 
 I know this is a private structure to the driver, so it's not that big
 of a deal at all, but the naming for this is a bit odd (mixed case.)
 
 Not a show stopper at all, but if you had run checkpatch.pl on it, it
 would have warned you about this.

Surprisingly it does not:

[dtor@dtor-ws vmci]$ ./scripts/checkpatch.pl -f  
drivers/misc/vmw_vmci/vmci_queue_pair.h 
total: 0 errors, 0 warnings, 191 lines checked

drivers/misc/vmw_vmci/vmci_queue_pair.h has no obvious style problems and is 
ready for submission.

Also silent on the patch itself...

We'll send a followup patch anyway.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 0/6] VSOCK for Linux upstreaming

[Dmitry Torokhov]

On Tuesday, January 08, 2013 05:30:56 PM David Miller wrote:
 From: Greg KH gre...@linuxfoundation.org
 Date: Tue, 8 Jan 2013 16:21:10 -0800
 
  On Tue, Jan 08, 2013 at 03:59:08PM -0800, George Zhang wrote:
  * * *
  
  This series of VSOCK linux upstreaming patches include latest udpate from
  VMware to address Greg's and all other's code review comments.
  
  Dave, you acked these patches a while ago,
 
 Really?  I'd like to see where I did that.
 
 Instead, what I remember doing was deferring to the feedback these
 folks received, stating that ideas that the virtio people had
 mentioned should be considered instead.
 
 http://marc.info/?l=linux-netdevm=135301515818462w=2

I believe Andy replied to Anthony's AF_VMCHANNEL post and the differences
between the proposed solutions.

 
 So definitely NACK this code and any infrastructure you've
 merged which essentialy depends upon it.

No, there is no infrastructure that depends on VSOCK, as VSOCK is built
on top of VMCI, not the other way around.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 0/6] VSOCK for Linux upstreaming

[Dmitry Torokhov]

On Tue, Jan 08, 2013 at 05:46:01PM -0800, David Miller wrote:
 From: Dmitry Torokhov d...@vmware.com
 Date: Tue, 08 Jan 2013 17:41:44 -0800
 
  On Tuesday, January 08, 2013 05:30:56 PM David Miller wrote:
  From: Greg KH gre...@linuxfoundation.org
  Date: Tue, 8 Jan 2013 16:21:10 -0800
  
   On Tue, Jan 08, 2013 at 03:59:08PM -0800, George Zhang wrote:
   * * *
   
   This series of VSOCK linux upstreaming patches include latest udpate 
   from
   VMware to address Greg's and all other's code review comments.
   
   Dave, you acked these patches a while ago,
  
  Really?  I'd like to see where I did that.
  
  Instead, what I remember doing was deferring to the feedback these
  folks received, stating that ideas that the virtio people had
  mentioned should be considered instead.
  
  http://marc.info/?l=linux-netdevm=135301515818462w=2
  
  I believe Andy replied to Anthony's AF_VMCHANNEL post and the differences
  between the proposed solutions.
 
 I'd much rather see a hypervisor neutral solution than a hypervisor
 specific one which this certainly is.

Objectively speaking neither solution is hypervisor neutral as there are
hypervisors that implement either VMCI or virtio or something else
entirely.

Our position is that VSOCK feature set is more complete and that it
should be possible to use transports other than VMCI for VSOCK traffic,
should interested parties implement them, and on this basis we ask to
include VSOCK.

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.

[Dmitry Torokhov]

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.

[Dmitry Torokhov]

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.

[Dmitry Torokhov]

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.

[Dmitry Torokhov]

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 086/493] net: remove use of __devexit_p

[Dmitry Torokhov]

On Mon, Nov 19, 2012 at 01:20:35PM -0500, Bill Pemberton wrote:
 CONFIG_HOTPLUG is going away as an option so __devexit_p is no longer
 needed.
 
 Signed-off-by: Bill Pemberton wf...@virginia.edu
 Cc: Rusty Russell ru...@rustcorp.com.au 
 Cc: Michael S. Tsirkin m...@redhat.com 
 Cc: Shreyas Bhatewara sbhatew...@vmware.com 
 Cc: VMware, Inc. pv-driv...@vmware.com 
 Cc: Francois Romieu rom...@fr.zoreil.com 
 Cc: virtualization@lists.linux-foundation.org 
 Cc: net...@vger.kernel.org 
 Cc: xen-de...@lists.xensource.com 
 ---
  drivers/net/virtio_net.c  | 2 +-
  drivers/net/vmxnet3/vmxnet3_drv.c | 2 +-

For VMXNET3:
 
Acked-by: Dmitry Torokhov d...@vmware.com

Thanks,
Dmitry


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 203/493] net: remove use of __devinit

[Dmitry Torokhov]

On Mon, Nov 19, 2012 at 01:22:32PM -0500, Bill Pemberton wrote:
 CONFIG_HOTPLUG is going away as an option so __devinit is no longer
 needed.

...

  drivers/net/vmxnet3/vmxnet3_drv.c  |  2 +-

For VMXNET3:

Acked-by: Dmitry Torokhov d...@vmware.com

Thanks,
Dmitry


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 471/493] net: remove use of __devexit

[Dmitry Torokhov]

On Mon, Nov 19, 2012 at 01:27:00PM -0500, Bill Pemberton wrote:
 CONFIG_HOTPLUG is going away as an option so __devexit is no
 longer needed.

...

  drivers/net/vmxnet3/vmxnet3_drv.c |  2 +-

For VMXNET3:

Acked-by: Dmitry Torokhov d...@vmware.com

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming

[Dmitry Torokhov]

On Monday, November 26, 2012 02:37:54 PM Greg KH wrote:
 On Wed, Nov 21, 2012 at 12:31:04PM -0800, George Zhang wrote:
  * * *
  This series of VMCI linux upstreaming patches include latest udpate from
  VMware.
  
  Summary of changes:
  - Sparse clean.
  - Checkpatch clean with one exception, a complex macro in
  
which we can't add parentheses.
  
  - Remove all runtime assertions.
  - Fix device name, so that existing user clients work.
  - Fix VMCI handle lookup.
 
 Given that you failed to answer the questions I asked the last time you
 posted this series, and you did not make any of the changes I asked for,
 I can't accept this (nor should you expect me to.)
 
 And people wonder why reviewers get so grumpy...
 
 My trees are now closed for the 3.8 merge window, so feel free to try
 again after 3.8-rc1 is out, and you have answered, and addressed, the
 questions and comments I made.

Greg, there were 3 specific complaints from you:

1. Given that this is a static function, there's no need for these
asserts, right?  Please send a follow-on patch removing all BUG_ON()
calls from these files, it's not acceptable to crash a user's box from
a driver that is handling parameters you are feeding it.

2. You obviously didn't run checkpatch on this file

3. This line causes sparse to complain.  The odds that userspace knows
what gcc is using for bool is pretty low.

Given the fact that the series addresses all 3 I fail to understand why
you would be grumpy.

Anyway, since there vsock has not been reviewed yet we are OK with
postponing this patch series till 3.9.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming

[Dmitry Torokhov]

On Monday, November 26, 2012 03:23:57 PM Greg KH wrote:
 On Mon, Nov 26, 2012 at 03:01:04PM -0800, Dmitry Torokhov wrote:
  On Monday, November 26, 2012 02:37:54 PM Greg KH wrote:
   On Wed, Nov 21, 2012 at 12:31:04PM -0800, George Zhang wrote:
* * *
This series of VMCI linux upstreaming patches include latest udpate
from
VMware.

Summary of changes:
- Sparse clean.
- Checkpatch clean with one exception, a complex macro in

  which we can't add parentheses.

- Remove all runtime assertions.
- Fix device name, so that existing user clients work.
- Fix VMCI handle lookup.
   
   Given that you failed to answer the questions I asked the last time you
   posted this series, and you did not make any of the changes I asked for,
   I can't accept this (nor should you expect me to.)
   
   And people wonder why reviewers get so grumpy...
   
   My trees are now closed for the 3.8 merge window, so feel free to try
   again after 3.8-rc1 is out, and you have answered, and addressed, the
   questions and comments I made.
  
  Greg, there were 3 specific complaints from you:
  
  1. Given that this is a static function, there's no need for these
  asserts, right?  Please send a follow-on patch removing all BUG_ON()
  calls from these files, it's not acceptable to crash a user's box from
  a driver that is handling parameters you are feeding it.
  
  2. You obviously didn't run checkpatch on this file
  
  3. This line causes sparse to complain.  The odds that userspace knows
  what gcc is using for bool is pretty low.
  
  Given the fact that the series addresses all 3 I fail to understand why
  you would be grumpy.
 
 You are ignoring my response to patch 12/12 for some reason (which
 repeated a bunch of the questions I had with that patch the last time it
 was posted.)  That is what I am referring to here.  None of those
 questions were addressed.

That one was explicitly acknowledged in
20121030052234.gh32...@dtor-ws.eng.vmware.com and fixed in series
posted on 11/01. Since it was fixed in earlier posting we did not
mention it again.

 
 Also, how was I to know that those 3 comments above were addressed?
 When someone posts questions and comments, please respond to those
 comments.  Don't just not respond at all and post the whole series 2
 weeks later with things changed and a vague comment of summary of
 changes in the 00 message.  Otherwise I will assume that you never even
 saw my post.

I thought Sparse clean and Checkpatch clean with one exception ...
are concrete enough, but I am open to improving the messaging. What 
would you like us to say?

 
 In other words, if someone takes the time to review and post comments,
 the least you can do is acknowledge those comments, right?

We did not want to litter mailing lists with OK responses, but will
do in the future.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming

[Dmitry Torokhov]

On Monday, November 26, 2012 03:44:26 PM Greg KH wrote:
 On Mon, Nov 26, 2012 at 03:36:52PM -0800, Dmitry Torokhov wrote:
  On Monday, November 26, 2012 03:23:57 PM Greg KH wrote:
   On Mon, Nov 26, 2012 at 03:01:04PM -0800, Dmitry Torokhov wrote:
On Monday, November 26, 2012 02:37:54 PM Greg KH wrote:
 On Wed, Nov 21, 2012 at 12:31:04PM -0800, George Zhang wrote:
  * * *
  This series of VMCI linux upstreaming patches include latest
  udpate
  from
  VMware.
  
  Summary of changes:
  - Sparse clean.
  - Checkpatch clean with one exception, a complex macro in
  
which we can't add parentheses.
  
  - Remove all runtime assertions.
  - Fix device name, so that existing user clients work.
  - Fix VMCI handle lookup.
 
 Given that you failed to answer the questions I asked the last time
 you
 posted this series, and you did not make any of the changes I asked
 for,
 I can't accept this (nor should you expect me to.)
 
 And people wonder why reviewers get so grumpy...
 
 My trees are now closed for the 3.8 merge window, so feel free to
 try
 again after 3.8-rc1 is out, and you have answered, and addressed,
 the
 questions and comments I made.

Greg, there were 3 specific complaints from you:

1. Given that this is a static function, there's no need for these
asserts, right?  Please send a follow-on patch removing all BUG_ON()
calls from these files, it's not acceptable to crash a user's box from
a driver that is handling parameters you are feeding it.

2. You obviously didn't run checkpatch on this file

3. This line causes sparse to complain.  The odds that userspace
knows
what gcc is using for bool is pretty low.

Given the fact that the series addresses all 3 I fail to understand
why
you would be grumpy.
   
   You are ignoring my response to patch 12/12 for some reason (which
   repeated a bunch of the questions I had with that patch the last time it
   was posted.)  That is what I am referring to here.  None of those
   questions were addressed.
  
  That one was explicitly acknowledged in
  20121030052234.gh32...@dtor-ws.eng.vmware.com and fixed in series
  posted on 11/01. Since it was fixed in earlier posting we did not
  mention it again.
 
 I questioned it on November 15, in:
   Message-ID: 20121116000118.ga8...@kroah.com
 
 Just ignoring that long response is acceptable?  Really?  I didn't ask
 enough questions in that review?  I see obvious comments in there that
 were _not_ addressed in the November 21st posting of that patch
 (typedefs for u32?  No c99 initializers?)

Hmm, neither I nor Google is aware of that msgid... So that would explain
why we have not addressed the comments that were in it ;)

Mind resending it, please? 

 
 And why isn't George responding to my comments when I ask questions?
 
 Also, please start numbering the submissions, this having to reference
 them by date is going to cause us all to get even more confused quicker.

OK, will do.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 12/12] VMCI: Some header and config files.

[Dmitry Torokhov]

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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming

[Dmitry Torokhov]

On Mon, Nov 26, 2012 at 07:27:07PM -0500, Woody Suwalski wrote:
 Greg KH wrote:
 On Mon, Nov 26, 2012 at 03:52:31PM -0800, Dmitry Torokhov wrote:
 
 Mind resending it, please?
 Now resent.
 I see both versions of Greg's message - one from 15 Nov, one
 today's. On my Gmail account...
 So Greg did post it...
 

Right, I also see it now in my personal LKML archive but for some
reason it didn't get delivered to my corporate mailbox. Weird.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 01/12] VMCI: context implementation.

[Dmitry Torokhov]

Hi Joe,

On Wed, Nov 21, 2012 at 01:04:46PM -0800, Joe Perches wrote:
 On Wed, 2012-11-21 at 12:31 -0800, George Zhang wrote:
  +   context = kzalloc(sizeof(*context), GFP_KERNEL);
  +   if (!context) {
  +   pr_warn(Failed to allocate memory for VMCI context.\n);
 
 OOM logging messages aren't necessary as alloc failures
 are already logged with a stack trace.
 

Are we sure we are going to keep this policy forever? I'd rather keep
the OOM warnings.

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 01/12] VMCI: context implementation.

[Dmitry Torokhov]

On Tuesday, October 30, 2012 08:46:52 AM Greg KH wrote:
 On Mon, Oct 29, 2012 at 09:01:40PM -0700, Dmitry Torokhov wrote:
  Hi Greg,
  
  On Mon, Oct 29, 2012 at 07:10:58PM -0700, Greg KH wrote:
   On Mon, Oct 29, 2012 at 06:03:42PM -0700, George Zhang wrote:
+/*
+ * Releases the VMCI context. If this is the last reference to
+ * the context it will be deallocated. A context is created with
+ * a reference count of one, and on destroy, it is removed from
+ * the context list before its reference count is
+ * decremented. Thus, if we reach zero, we are sure that nobody
+ * else are about to increment it (they need the entry in the
+ * context list for that). This function musn't be called with a
+ * lock held.
+ */
+void vmci_ctx_release(struct vmci_ctx *context)
+{
+   ASSERT(context);
+   kref_put(context-kref, ctx_free_ctx);
+}
+
   
   Hm, are you _sure_ you should be calling this without a lock held?
   That's usually kref-101, you MUST hold a lock when calling put,
   otherwise you can race a kref_get() call, and all hell can break loose.
   
   Because of this, some saner people (like Al Viro), have suggested that I
   force the kref_put() and kref_get() calls pass in a spinlock just to
   enforce this.
   
   So, tell me what I'm missing here, and why you put the comment here
   saying that it really is supposed to be called without a lock held?  How
   is that safe?
  
  Contexts are created/registered in vmci_ctx_init_ctx() and unregistered in
  vmci_ctx_release_ctx() and these operations are protected by
  ctx_list.lock spinlock. Context lookup (vmci_ctx_get) also uses spinlock
  to traverse list of registered contexts and then grabs reference to the
  [valid] context. The use of kref_put() without additional locking in
  vmci_ctx_release() is fine as there is no chance of another thread
  bumping count from 0 to 1.
 
 As I didn't see all callers of this holding that spinlock, it was
 confusing.  You should put this type of description somewhere so that
 other reviewers don't have the same questions.

Fair enough, we'll add better comments to this code.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 05/12] VMCI: event handling implementation.

[Dmitry Torokhov]

On Tuesday, October 30, 2012 08:50:41 AM Greg KH wrote:
 On Mon, Oct 29, 2012 at 10:01:52PM -0700, Dmitry Torokhov wrote:
  On Mon, Oct 29, 2012 at 07:26:05PM -0700, Greg KH wrote:
   On Mon, Oct 29, 2012 at 06:04:27PM -0700, George Zhang wrote:
+static void event_signal_destroy(struct kref *kref)
+{
+   struct vmci_subscription *entry =
+   container_of(kref, struct vmci_subscription, 
kref);
+
+   complete(entry-done);
+}
   
   Didn't you just leak memory here?  What frees the structure up?
  
  event_unregister_subscription() waits for that completion and frees the
  structure. We want event_unregister_subscription() to wait until all
  fired callbacks completed before unregister is complete.
 
 So all calls to this can just sit and spin waiting for others to clean
 up?  Odd, but ok.

Not all as there is logically only one owner of the subscription so
naturally it waits until all notification callbacks are done.

Frankly we have a change that gets rid of delayed ecvent callbacks
and so the refcounting is no longer needed at all.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 08/12] VMCI: resource object implementation.

[Dmitry Torokhov]

On Tuesday, October 30, 2012 08:51:59 AM Greg KH wrote:
 On Mon, Oct 29, 2012 at 10:20:16PM -0700, Dmitry Torokhov wrote:
  On Mon, Oct 29, 2012 at 07:29:05PM -0700, Greg KH wrote:
   On Mon, Oct 29, 2012 at 06:04:58PM -0700, George Zhang wrote:
+static struct vmci_resource *vmci_resource_lookup(struct vmci_handle
handle) +{
+   struct vmci_resource *r, *resource = NULL;
+   struct hlist_node *node;
+   unsigned int idx = vmci_resource_hash(handle);
+
+   BUG_ON(VMCI_HANDLE_EQUAL(handle, VMCI_INVALID_HANDLE));
   
   You just crashed a machine, with no chance for recovery.  Not a good
   idea.  Never a good idea.  Customers just lost data, and now they are
   mad.  Make sure you at least print out your email address so they know
   who to blame :)
   
   Seriously, never BUG() in a driver, warn, sure, but this just looks like
   a debugging assert().  Please remove all of these, they are sprinkled
   all over the driver code here, I'm only responding to one of them here.
   
   Even better yet, properly handle the error and keep on going, that's
   what the rest of the kernel does.  Or should :)
  
  For public APIs it certainly makes sense to check and handle erroneous
  input;
 It's not public, it's an in-kernel api.  See the static up there?  :)

Yes, exactly, that is not public but internal and that is why it might
be acceptable to enforce invariant. Cross-subsystem (i.e public from
the in-kernel POV) APIs should of course check and refuse bad input.

 
  internally it often makes sense to simply enforce invariants, because if
  we managed to get into that state that we consider impossible we can't
  really trust anything.
 
 Then error out, don't crash the box.  Again, this really looks like an
 ASSERT() you are trying to catch, which you know how well we like those
 in kernel code...

At certain point it simply does not make sense to add error handling
paths to handle situation that should be impossible to happen.

 
  FWIW:
  [dtor@dtor-ws kernel]$ grep -r BUG_ON . | wc -l
  11269
 
 I'm not saying that those are acceptable either, I just don't want to
 add any more to the kernel.

I do not think eradicating BUG_ONs from kernel in general is a good idea.
At some point you should just die with as much information as possible.

That said we'll go and see what could be converted from BUG_ON to WARN_ON
and what could be handled without taking the box down...

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming

[Dmitry Torokhov]

On Tue, Oct 30, 2012 at 08:48:01AM -0700, Greg KH wrote:
 On Mon, Oct 29, 2012 at 09:07:44PM -0700, Dmitry Torokhov wrote:
  Hi Greg,
  
  On Mon, Oct 29, 2012 at 07:19:38PM -0700, Greg KH wrote:
   On Mon, Oct 29, 2012 at 06:03:28PM -0700, George Zhang wrote:
 drivers/misc/Kconfig  |1
 drivers/misc/Makefile |2
 drivers/misc/vmw_vmci/Kconfig |   16
 drivers/misc/vmw_vmci/Makefile|   43
   
   Meta comment here, why drivers/misc/?  The other hypervisor
   infrastructures all have their own directory under drivers/  Should we
   be moving everything to drivers/hyperv/ somehow?
  
  drivers/hyperv is not the best name for obvious reasons...
 
 Sorry, yes :)

:)

 
  I think that even if we had a special directory for vmci having network
  drivers in Dave's realm and pvscsi in James's is best option, so the new
  directory would contain vmci and the balloon driver (vsock will go into
  net/).  Given that balloon is already in drivers/misc it looked like
  obvious place for VMCI as well.
 
 I agree that the individual drivers should go in the subsystem area,
 it's this hypervisor bus core type code that I'm questioning.  Right
 now every hypervisor is putting that logic in a different place in the
 kernel, having some consistency here would be nice.

Hmm, I wonder if miscellaneous and core hypervisor drivers should end
up in drivers/platform:

drivers/platform/hyperv
drivers/platform/olpc
drivers/platform/vmware
drivers/platform/xen
drivers/platform/x86

But really we'd like to get VMCI into mainline first and move to a new
place later if such a better place is found.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming

[Dmitry Torokhov]

On Tue, Oct 30, 2012 at 09:27:23AM -0700, Greg KH wrote:
 On Tue, Oct 30, 2012 at 09:18:07AM -0700, Dmitry Torokhov wrote:
I think that even if we had a special directory for vmci having network
drivers in Dave's realm and pvscsi in James's is best option, so the new
directory would contain vmci and the balloon driver (vsock will go into
net/).  Given that balloon is already in drivers/misc it looked like
obvious place for VMCI as well.
   
   I agree that the individual drivers should go in the subsystem area,
   it's this hypervisor bus core type code that I'm questioning.  Right
   now every hypervisor is putting that logic in a different place in the
   kernel, having some consistency here would be nice.
  
  Hmm, I wonder if miscellaneous and core hypervisor drivers should end
  up in drivers/platform:
  
  drivers/platform/hyperv
  drivers/platform/olpc
  drivers/platform/vmware
  drivers/platform/xen
  drivers/platform/x86
 
 That makes sense to me, nice.
 
  But really we'd like to get VMCI into mainline first and move to a new
  place later if such a better place is found.
 
 Heh, no one wants to fight for something to help everyone out, they just
 want their own code accepted :)

No, this is more about finding a person who would be maintaining it and
thus would review our code. For now we got you tagged and do not want to
let go of you :P

Or should we maintain our own stuff and have Linus pull it directly,
like Xen guys appear to be doing? Really, moving it is not an issue for
us.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 05/12] VMCI: event handling implementation.

[Dmitry Torokhov]

On Mon, Oct 29, 2012 at 07:26:05PM -0700, Greg KH wrote:
 On Mon, Oct 29, 2012 at 06:04:27PM -0700, George Zhang wrote:
  +static void event_signal_destroy(struct kref *kref)
  +{
  +   struct vmci_subscription *entry =
  +   container_of(kref, struct vmci_subscription, kref);
  +
  +   complete(entry-done);
  +}
 
 Didn't you just leak memory here?  What frees the structure up?

event_unregister_subscription() waits for that completion and frees the
structure. We want event_unregister_subscription() to wait until all
fired callbacks completed before unregister is complete.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 08/12] VMCI: resource object implementation.

[Dmitry Torokhov]

On Mon, Oct 29, 2012 at 07:29:46PM -0700, Greg KH wrote:
 On Mon, Oct 29, 2012 at 06:04:58PM -0700, George Zhang wrote:
  VMCI resource tracks all used resources within the vmci code.
 
 Same kref_put() with no lock seen question in this file, prove me
 wrong please.

Same proof as with others, the reference can't be taken unless the
resource is in hast table (which protected by RCU/spinlock) so no fear
of bouncing off 0.

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.

[Dmitry Torokhov]

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 pv-driv...@vmware.com
  +#
  +
 
 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

Re: [Pv-drivers] [PATCH 04/10] VMCI: device driver implementaton.

[Dmitry Torokhov]

On Thursday, October 25, 2012 01:31:48 PM Greg KH wrote:
 On Thu, Oct 25, 2012 at 01:16:00PM -0700, Andy King wrote:
  Hi Greg,
  
+EXPORT_SYMBOL(vmci_device_get);
   
   EXPORT_SYMBOL_GPL() for this, and all other exports?
  
  We'd prefer to leave them as vanilla exports.  While we're committed
  to open-sourcing everything, including our non-upstreamed drivers,
  we don't really have a strong opinion regarding consuming our exports
  in closed-source (general GPL issues aside).
 
 You can't just say general GPL issues aside.  Honestly, given your
 company's prior actions in regards to Linux kernel drivers and the
 licenses of them, I don't trust them at all.  To help gain that trust
 back, marking the exports in this manner will be a great improvement.
 
 To insist otherwise is to only reinforce my doubts, and reduce my
 wanting to even review or accept this code at all.  Sorry about that.

Huh? What are the concerns exactly? I do not really see difference between
EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL(). The code either derivative of the
kernel or it is not and so it either falls under the kernel license or not.

From out perspective we do not really care what other code might use VMCI,
all our Linux drivers, even if not all are upstream [yet], are GPL.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 04/10] VMCI: device driver implementaton.

[Dmitry Torokhov]

On Thursday, October 25, 2012 02:04:48 PM Greg KH wrote:
 On Thu, Oct 25, 2012 at 01:45:39PM -0700, Dmitry Torokhov wrote:
  On Thursday, October 25, 2012 01:31:48 PM Greg KH wrote:
   On Thu, Oct 25, 2012 at 01:16:00PM -0700, Andy King wrote:
Hi Greg,

  +EXPORT_SYMBOL(vmci_device_get);
 
 EXPORT_SYMBOL_GPL() for this, and all other exports?

We'd prefer to leave them as vanilla exports.  While we're committed
to open-sourcing everything, including our non-upstreamed drivers,
we don't really have a strong opinion regarding consuming our exports
in closed-source (general GPL issues aside).
   
   You can't just say general GPL issues aside.  Honestly, given your
   company's prior actions in regards to Linux kernel drivers and the
   licenses of them, I don't trust them at all.  To help gain that trust
   back, marking the exports in this manner will be a great improvement.
   
   To insist otherwise is to only reinforce my doubts, and reduce my
   wanting to even review or accept this code at all.  Sorry about that.
  
  Huh? What are the concerns exactly? I do not really see difference between
  EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL(). The code either derivative of the
  kernel or it is not and so it either falls under the kernel license or
  not.
 
 I totally agree.  In this case, do you think it falls under the kernel
 license or not?

VMCI implementation that we are submitting is GPL as witnessed by the license
notices on the source files ;)

 
  From out perspective we do not really care what other code might use VMCI,
  all our Linux drivers, even if not all are upstream [yet], are GPL.
 
 That's nice to hear, although without proof of that, we have to take
 your word :)

The source to Open VM Tools (which includes guest drivers that have not
been upstreamed yet) can be always found here:

https://sourceforge.net/projects/open-vm-tools/files/

and here:

git.opensource.vmware.com/opensource/open-vm-tools/

The hosted drivers are distributed in source form with the product and
here:

https://my.vmware.com/web/vmware/info/slug/desktop_end_user_computing/vmware_workstation/9_0#open_source

Thanks,
Dmitry




 
 thanks,
 
 greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [PATCH 04/10] VMCI: device driver implementaton.

[Dmitry Torokhov]

On Thursday, October 25, 2012 02:03:20 PM Greg KH wrote:
 On Thu, Oct 25, 2012 at 01:43:50PM -0700, Bhavesh Davda wrote:
  Hi Greg,
  
   You can't just say general GPL issues aside.  Honestly, given your
   company's prior actions in regards to Linux kernel drivers and the
   licenses of them, I don't trust them at all.  To help gain that trust
   back, marking the exports in this manner will be a great improvement.
  
  You don't expect us to just let that comment slide :) Can you be more
  specific about which prior actions in regards to which kernel
  drivers, and when?
 
 For many many years, you shipped closed source Linux kernel drivers for
 your products.  Only recently has this changed.

That was in the past. As far as I know our Linux drivers have been open
for more than 5 years, so not that recently.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build

[Dmitry Torokhov]

On Sat, Jul 28, 2012 at 12:55:35PM -0700, Greg KH wrote:
 On Fri, Jul 27, 2012 at 01:29:27PM -0700, Dmitry Torokhov wrote:
  On Fri, Jul 27, 2012 at 11:16:39AM -0700, Greg KH wrote:
   On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
 The kernel style is to use lower_case for everything.
 So this would become:
 
 vmci_device_get()
 
 This is obviously a very general comment and applies everywhere.

I wish I could lower case these symbols but VMCI has already existed
outside the mainline Linux tree for some time now and changing these
exported symbols would mean that other drivers that depend on VMCI
(vSock, vmhgfs) would need to change as well.   One thought that did
come to mind was exporting both VMCI_Device_Get and vmci_device_get
but that would likely just confuse people.  So in short I have made
function names lower case where possible, but exported symbols could
not be changed.
   
   Not true at all.  You want those drivers to be merged as well, right?
   So they will need to have their functions changed, and their code as
   well.
   
   Just wait until we get to the change your functionality around
   requests, those will require those drivers to change.  Right now we are
   at the silly and obvious things you did wrong stage of the review
   process :)
   
   So please fix these, and also, post these drivers as well, so we can see
   how they interact with the core code.
   
   Actually, if you are going to need lots of refactoring for these
   drivers, and the core, I would recommend putting this all in the staging
   tree, to allow that to happen over time.  That would ensure that your
   users keep having working systems, and let you modify the interfaces
   better and easier, than having to keep it all out-of-tree.
   
   What do you think?
  
  Actually I think that we'd prefer to keep this in a patch-based form, at
  least for now, because majority of our users get these drivers with
  VMware Tools and will continue doing so until ditsributions start
  enabling VMCI in their kernels. Which they probably won't until VMCI
  moves form staging. We'd also have to constantly adjust drivers that we
  are not working on getting upstream at this time to work with the
  rapidly changing version of VMCI in staging, which will just add work
  for us.
 
 That wouldn't be an issue if you just include all of the drivers in the
 tree at the same time, right?

Maybe it wouldn't, however at this time we have not scheduled any
resources for upstreaming vmhgfs driver. We however do seek feedback on
vmci driver (and later vsock) for which we did schedule resources.

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build

[Dmitry Torokhov]

Hi Alan,

On Fri, Jul 27, 2012 at 10:53:57AM +0100, Alan Cox wrote:
  +enum {
  +   VMCI_SUCCESS_QUEUEPAIR_ATTACH   =  5,
  +   VMCI_SUCCESS_QUEUEPAIR_CREATE   =  4,
  +   VMCI_SUCCESS_LAST_DETACH=  3,
  +   VMCI_SUCCESS_ACCESS_GRANTED =  2,
  +   VMCI_SUCCESS_ENTRY_DEAD =  1,
 
 We've got a nice collection of Linux error codes than you, and it would
 make the driver enormously more readable on the Linux side if as low
 level as possible it started using Linux error codes.

If VMCI was only used on Linux we'd definitely do that; however VMCI
core is shared among several operating systems (much like ACPI is) and
we'd like to limit divergencies between them while conforming to the
kernel coding style as much as possible.

We'll make sure that we will not leak VMCI-specific errors to the
standard kernel APIs.

Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build

[Dmitry Torokhov]

On Fri, Jul 27, 2012 at 11:16:39AM -0700, Greg KH wrote:
 On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
   The kernel style is to use lower_case for everything.
   So this would become:
   
   vmci_device_get()
   
   This is obviously a very general comment and applies everywhere.
  
  I wish I could lower case these symbols but VMCI has already existed
  outside the mainline Linux tree for some time now and changing these
  exported symbols would mean that other drivers that depend on VMCI
  (vSock, vmhgfs) would need to change as well.   One thought that did
  come to mind was exporting both VMCI_Device_Get and vmci_device_get
  but that would likely just confuse people.  So in short I have made
  function names lower case where possible, but exported symbols could
  not be changed.
 
 Not true at all.  You want those drivers to be merged as well, right?
 So they will need to have their functions changed, and their code as
 well.
 
 Just wait until we get to the change your functionality around
 requests, those will require those drivers to change.  Right now we are
 at the silly and obvious things you did wrong stage of the review
 process :)
 
 So please fix these, and also, post these drivers as well, so we can see
 how they interact with the core code.
 
 Actually, if you are going to need lots of refactoring for these
 drivers, and the core, I would recommend putting this all in the staging
 tree, to allow that to happen over time.  That would ensure that your
 users keep having working systems, and let you modify the interfaces
 better and easier, than having to keep it all out-of-tree.
 
 What do you think?

Actually I think that we'd prefer to keep this in a patch-based form, at
least for now, because majority of our users get these drivers with
VMware Tools and will continue doing so until ditsributions start
enabling VMCI in their kernels. Which they probably won't until VMCI
moves form staging. We'd also have to constantly adjust drivers that we
are not working on getting upstream at this time to work with the
rapidly changing version of VMCI in staging, which will just add work
for us.

So we'd like to get more feedback and have a chance to address issues
and then decide whether staying in staging makes sense or not.

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [vmw_vmci RFC 00/11] VMCI for Linux

[Dmitry Torokhov]

Hi Greg,

On Mon, Jun 04, 2012 at 03:57:57PM -0700, Greg KH wrote:
 On Fri, Jun 01, 2012 at 08:33:02AM -0700, Andy King wrote:
  Greg,
  
  Thanks so much for the comments and apologies for the delayed response.
  
   Don't we have something like this already for KVM and maybe Xen?
   virtio?  Can't you use that code instead of a new block of code that
   is only used by vmware users?  It has virtual pci devices which
   should give you what you want/need here, right?
  
   If not, why doesn't that work for you?  Would it be easier to just
   extend it?
  
  The VMCI virtual device for which this driver is intended has been
  around a lot longer than this submission might suggest.  The virtual
  hardware was released in a product before Rusty sent his RFC and
  quite a bit before it made it to mainline; there was, regrettably,
  no virtio then.
  
  As such, it was designed to be its own transport, and it's something
  that is now very much fixed at the hardware level (enhancements
  not withstanding), and which we have to support all the way back.
 
 What hardware are you refering to here?

The virtual hardware that is currently shipping and has been shipping
for a few years.

 
  In addition to that, our hypervisor endpoints are written using
  the existing device backend; virtio doesn't currently make a lot of
  sense for them, and would require a lot of additional work.
  
  All of this is unfortunate.  While I agree that virtio is certainly
  the right approach, and we need to avoid this proliferation, I think
  at this point we'd really like to try and upstream this in its current
  form.  There's certainly the possibility going forwards that we could
  add a glue layer, such that other clients could use virtio if they're
  willing to write their own hypervisor endpoints.
  
  Does that sound reasonable?
 
 Not really, why should we take an interface that is tied to something
 that you are saying isn't something we should be using?

That is not what Andy said. If virtio was available when we started
shipping VMCI then we certainly could have used that, but since it
wasn't there we invented something else.

 Don't you also
 have control over the hypervisor side of things in order to properly
 design this type of thing?

We do not have a time machine to go back and change products that we
already shipped to the customers. It is probably the same story as with
Hyper-V's vmbus which is not virtio.

Besides, virtio is not available on non-Linux guests with we have to
support as well, and than affected the design decisions in hypervisor
layer that have been made several years ago.

Thanks,
Dmitry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Xen-devel] [PATCH 2/4] xen kconfig: relax INPUT_XEN_KBDDEV_FRONTEND deps

[Dmitry Torokhov]

Hi Andrew,

On Fri, Jan 06, 2012 at 10:58:06AM -0500, Andrew Jones wrote:
 
 
 - Original Message -
  On Fri, Jan 06, 2012 at 10:43:09AM +0100, Andrew Jones wrote:
   PV-on-HVM guests may want to use the xen keyboard/mouse frontend,
   but
   they don't use the xen frame buffer frontend. For this case it
   doesn't
   make much sense for INPUT_XEN_KBDDEV_FRONTEND to depend on
   XEN_FBDEV_FRONTEND. The opposite direction always makes more sense,
   i.e.
   if you're using xenfb, then you'll want xenkbd. Switch the
   dependencies.
  
  You need to CC as well these people that have 'maintainer' field on
  them:
  
  konrad@phenom:~/work/linux$ scripts/get_maintainer.pl -f
  drivers/video/Kconfig
  Florian Tobias Schandinat florianschandi...@gmx.de
  (maintainer:FRAMEBUFFER LAYER)
  linux-fb...@vger.kernel.org (open list:FRAMEBUFFER LAYER)
  linux-ker...@vger.kernel.org (open list)
  konrad@phenom:~/work/linux$ scripts/get_maintainer.pl -f
  drivers/input/misc/Kconfig
  Dmitry Torokhov dmitry.torok...@gmail.com (maintainer:INPUT
  (KEYBOARD,...,commit_signer:9/16=56%)
  Samuel Ortiz sa...@linux.intel.com (commit_signer:3/16=19%)
  Anirudh Ghayal agha...@codeaurora.org (commit_signer:2/16=12%)
  Peter Ujfalusi peter.ujfal...@ti.com (commit_signer:2/16=12%)
  Alan Cox a...@linux.intel.com (commit_signer:2/16=12%)
  linux-in...@vger.kernel.org (open list:INPUT (KEYBOARD,...)
  linux-ker...@vger.kernel.org (open list)
  
 
 Thanks. Replied with them in CC.
 
 Drew
 
   
   Signed-off-by: Andrew Jones drjo...@redhat.com
   ---
drivers/input/misc/Kconfig |2 +-
drivers/video/Kconfig  |1 +
2 files changed, 2 insertions(+), 1 deletions(-)
   
   diff --git a/drivers/input/misc/Kconfig
   b/drivers/input/misc/Kconfig
   index 22d875f..36c15bf 100644
   --- a/drivers/input/misc/Kconfig
   +++ b/drivers/input/misc/Kconfig
   @@ -533,7 +533,7 @@ config INPUT_CMA3000_I2C

config INPUT_XEN_KBDDEV_FRONTEND
 tristate Xen virtual keyboard and mouse support
   - depends on XEN_FBDEV_FRONTEND
   + depends on XEN

This is OK with me.

 default y
 select XEN_XENBUS_FRONTEND
 help
   diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
   index d83e967..269b299 100644
   --- a/drivers/video/Kconfig
   +++ b/drivers/video/Kconfig
   @@ -2269,6 +2269,7 @@ config XEN_FBDEV_FRONTEND
 select FB_SYS_IMAGEBLIT
 select FB_SYS_FOPS
 select FB_DEFERRED_IO
   + select INPUT_XEN_KBDDEV_FRONTEND

But here you need to either depend on or select INPUT as select does not
resolve dependencies for selected symbol.

Thanks.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3]: Staging: hv: Use native wait primitives

[Dmitry Torokhov]

On Tue, Feb 15, 2011 at 05:52:46PM +, KY Srinivasan wrote:
 
 If I understand you correctly, you would be prefer to have unbounded waiting 
 with comments 
 justifying why we cannot have timeouts. I will roll out a patch once the tree 
 stabilizes.
 

Just make sure to add a comment explaining why the wait is unbounded.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3

[Dmitry Torokhov]

On Wednesday 05 May 2010 10:23:16 am Christoph Hellwig wrote:
 On Tue, May 04, 2010 at 04:02:25PM -0700, Pankaj Thakkar wrote:
  The plugin image is provided by the IHVs along with the PF driver and is
  packaged in the hypervisor. The plugin image is OS agnostic and can be
  loaded either into a Linux VM or a Windows VM. The plugin is written
  against the Shell API interface which the shell is responsible for
  implementing. The API
 
 We're not going to add any kind of loader for binry blobs into kernel
 space, sorry.  Don't even bother wasting your time on this.
 

It would not be a binary blob but software properly released under GPL.
The current plan is for the shell to enforce GPL requirement on the
plugin code, similar to what module loaded does for regular kernel
modules.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3

[Dmitry Torokhov]

On Wednesday 05 May 2010 10:31:20 am Christoph Hellwig wrote:
 On Wed, May 05, 2010 at 10:29:40AM -0700, Dmitry Torokhov wrote:
   We're not going to add any kind of loader for binry blobs into kernel
   space, sorry.  Don't even bother wasting your time on this.
  
  It would not be a binary blob but software properly released under GPL.
  The current plan is for the shell to enforce GPL requirement on the
  plugin code, similar to what module loaded does for regular kernel
  modules.
 
 The mechanism described in the document is loading a binary blob
 coded to an abstract API.

Yes, with the exception that the only body of code that will be
accepted by the shell should be GPL-licensed and thus open and available
for examining. This is not different from having a standard kernel
module that is loaded normally and plugs into a certain subsystem.
The difference is that the binary resides not on guest filesystem
but elsewhere.

 
 That's something entirely different from having normal modules for
 the Virtual Functions, which we already have for various pieces of
 hardware anyway.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3

[Dmitry Torokhov]

On Wednesday 05 May 2010 01:09:48 pm Arnd Bergmann wrote:
   If you have any interesting in developing this further, do:
   
(1) move the limited VF drivers directly into the kernel tree,
talk to them through a normal ops vector
  
  [PT] This assumes that all the VF drivers would always be available.
  Also we have to support windows and our current design supports it
  nicely in an OS agnostic manner.
 
 Your approach assumes that the plugin is always available, which has
 exactly the same implications.

Since plugin[s] are carried by the host they are indeed always
available.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH] input: Fix interrupt enable in i8042_ctr when enabling interrupt fails

[Dmitry Torokhov]

Hi Steven, Markus,

On 9/10/07, Steven Rostedt [EMAIL PROTECTED] wrote:

 --
 On Mon, 10 Sep 2007, Markus Armbruster wrote:
 
  I believe this possible, but unlikely (perhaps not so unlikely on
  virtual machines).  Scenarios involve enable succeeding the first
  time, failing the second time, and succeeding the third time.  I can
  provide details, but the point I'd like to make is not that this is
  broken (although it is, strictly speaking), but that it is not
  obviously correct where it easily could be: just clear the interrupt
  enable bits when writing them to the hardware failed, like the old
  code did.
 

 I also want to stress that this is more of a clean up for technically
 correct code than a bug fix.  This bug probably would never happen on
 baremetal unless it was running on broken hardware.

  BUT!!!

 With more and more systems going to a virtual environment, having a bug or
 some other anomaly can trigger the error that this patch prevents. The
 patch will also trigger a print in the case of running on a buggy virtual
 machine, which would help out the developers of that virtual machine to
 fix their code.


The patch is in my tree and will be merged in the next window.

-- 
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization