Re: [PATCH v2 9/9] dt-bindings: drop redundant part of title (manual)
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)
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)
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)
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
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
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
> ---
> 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
> 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 != &x86_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)
> copy_array(&h->init,
Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers.
On Tue, Sep 26, 2017 at 02:36:57AM -0400, Pankaj Gupta wrote: > > > > > A bit late to a party, but: > > > > On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong wrote: > > > From: Rusty Russell > > > > > > There's currently a big lock around everything, and it means that we > > > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current) > > > while the rng is reading. This is a real problem when the rng is slow, > > > or blocked (eg. virtio_rng with qemu's default /dev/random backend) > > > > > > This doesn't help (it leaves the current lock untouched), just adds a > > > lock to protect the read function and the static buffers, in preparation > > > for transition. > > > > > > Signed-off-by: Rusty Russell > > > --- > > ... > > > > > > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char > > > __user *buf, > > > goto out_unlock; > > > } > > > > > > + mutex_lock(&reading_mutex); > > > > I think this breaks O_NONBLOCK: we have hwrng core thread that is > > constantly pumps underlying rng for data; the thread takes the mutex > > and calls rng_get_data() that blocks until RNG responds. This means > > that even user specified O_NONBLOCK here we'll be waiting until > > [hwrng] thread releases reading_mutex before we can continue. > > I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns > without waiting for data which can let mutex to be used by other > threads waiting if any? > > rng_dev_read > rng_get_data > virtio_read As I said in the paragraph above the code that potentially holds the mutex for long time is the thread in hwrng core: hwrng_fillfn(). As it calls rng_get_data() with "wait" argument == 1 it may block while holding reading_mutex, which, in turn, will block rng_dev_read(), even if it was called with O_NONBLOCK. Thanks. -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers.
A bit late to a party, but:
On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong wrote:
> From: Rusty Russell
>
> There's currently a big lock around everything, and it means that we
> can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> while the rng is reading. This is a real problem when the rng is slow,
> or blocked (eg. virtio_rng with qemu's default /dev/random backend)
>
> This doesn't help (it leaves the current lock untouched), just adds a
> lock to protect the read function and the static buffers, in preparation
> for transition.
>
> Signed-off-by: Rusty Russell
> ---
...
>
> @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
> __user *buf,
> goto out_unlock;
> }
>
> + mutex_lock(&reading_mutex);
I think this breaks O_NONBLOCK: we have hwrng core thread that is
constantly pumps underlying rng for data; the thread takes the mutex
and calls rng_get_data() that blocks until RNG responds. This means
that even user specified O_NONBLOCK here we'll be waiting until
[hwrng] thread releases reading_mutex before we can continue.
> if (!data_avail) {
> bytes_read = rng_get_data(current_rng, rng_buffer,
> rng_buffer_size(),
> !(filp->f_flags & O_NONBLOCK));
> if (bytes_read < 0) {
> err = bytes_read;
> - goto out_unlock;
> + goto out_unlock_reading;
> }
> data_avail = bytes_read;
> }
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
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
> > > > > > > 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 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros
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 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
On Tue, Dec 1, 2015 at 2:54 PM, 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 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
On Tue, Dec 1, 2015 at 3:04 PM, Sinclair Yeh 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 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: [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros
On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh 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
>> > Reviewed-by: Thomas Hellstrom
>> > Reviewed-by: Alok N Kataria
>> > Cc: pv-driv...@vmware.com
>> > Cc: linux-graphics-maintai...@vmware.com
>> > Cc: Dmitry Torokhov
>> > Cc: Arnd Bergmann
>> > Cc: Greg Kroah-Hartman
>> > 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 4/6] Input: Remove vmmouse port reservation
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
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
> Reviewed-by: Thomas Hellstrom
> Reviewed-by: Alok N Kataria
> Cc: pv-driv...@vmware.com
> Cc: linux-graphics-maintai...@vmware.com
> Cc: Dmitry Torokhov
> Cc: Arnd Bergmann
> Cc: Greg Kroah-Hartman
> 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 v3] Add virtio-input driver.
On Tue, Mar 24, 2015 at 06:22:36PM +0100, Michael S. Tsirkin wrote:
> On Tue, Mar 24, 2015 at 09:23:37AM -0700, Dmitry Torokhov wrote:
> > 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
> > >
> > > 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
> > > > +S: Maintained
> > > > +F: drivers/virtio/virtio_input.c
> > > > +F: include/uapi/linux/virtio_input.h
> > > > +
> > > > VIA RHINE NETWORK DRIVER
> > > > M: Roger Luethi
> > > > 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
> > > > +#include
> > > > +#include
> > > > +
> > > > +#include
> > > > +#include
> > > > +
> > > > +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];
> > > > + spinlo
Re: [PATCH v3] Add virtio-input driver.
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
>
> 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
> > +S: Maintained
> > +F: drivers/virtio/virtio_input.c
> > +F: include/uapi/linux/virtio_input.h
> > +
> > VIA RHINE NETWORK DRIVER
> > M: Roger Luethi
> > 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
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +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
Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.
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 v2] Add virtio-input driver.
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
> ---
> 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
> +S: Maintained
> +F: drivers/virtio/virtio_input.c
> +F: include/uapi/linux/virtio_input.h
> +
> VIA RHINE NETWORK DRIVER
> M: Roger Luethi
> 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
> +#include
> +#include
> +
> +#include
> +#include
> +
> +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.
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 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
> > ---
> > 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
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +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)
> > +{
> > +
Re: [Pv-drivers] [PATCH 0/1] VM Sockets for Linux upstreaming
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
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 0/6] VSOCK for Linux upstreaming
On Tue, Jan 08, 2013 at 05:46:01PM -0800, David Miller wrote: > From: Dmitry Torokhov > Date: Tue, 08 Jan 2013 17:41:44 -0800 > > > On Tuesday, January 08, 2013 05:30:56 PM David Miller wrote: > >> From: Greg KH > >> 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-netdev&m=135301515818462&w=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 0/6] VSOCK for Linux upstreaming
On Tuesday, January 08, 2013 05:30:56 PM David Miller wrote: > From: Greg KH > 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-netdev&m=135301515818462&w=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 07/12] VMCI: queue pairs implementation.
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 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 12:44:06 PM Greg KH wrote: > On Fri, Nov 30, 2012 at 12:09:40PM -0800, Dmitry Torokhov wrote: > > On Friday, November 30, 2012 10:57:55 AM Greg KH wrote: > > > On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > > > > However you snipped the rest of my reply: do we really need to > > > > renumber > > > > ioctls? There is no benefit for the driver as its ioctl handler does > > > > not parse the numbers into components. > > > > > > I don't know if you need to renumber, I really don't understand what you > > > were trying to do with this code, and as it was acting differently from > > > all other kernel ioctl declarations, I asked for some clarity. > > > > > > If you can rewrite it to look sane, and keep the same numbers, that's > > > fine with me. > > > > OK, it looks like we can redo them as: > > > > #define IOCTL_VMCI_VERSION _IO(7, 0x9f)/* 1951 */ > > #define IOCTL_VMCI_INIT_CONTEXT _IO(7, 0xa0)/* 1952 */ > > > > Is this acceptable? > > Sure, that's better. You also got lucky, '7' happens to be unused right > now. Excellent. You said you want the next drop after -rc1, right? -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 10:57:55 AM Greg KH wrote: > On Fri, Nov 30, 2012 at 10:45:44AM -0800, Dmitry Torokhov wrote: > > However you snipped the rest of my reply: do we really need to renumber > > ioctls? There is no benefit for the driver as its ioctl handler does > > not parse the numbers into components. > > I don't know if you need to renumber, I really don't understand what you > were trying to do with this code, and as it was acting differently from > all other kernel ioctl declarations, I asked for some clarity. > > If you can rewrite it to look sane, and keep the same numbers, that's > fine with me. OK, it looks like we can redo them as: #define IOCTL_VMCI_VERSION _IO(7, 0x9f)/* 1951 */ #define IOCTL_VMCI_INIT_CONTEXT _IO(7, 0xa0)/* 1952 */ Is this acceptable? Thanks, Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On Friday, November 30, 2012 10:39:18 AM Greg KH wrote: > On Fri, Nov 30, 2012 at 09:20:41AM -0800, Dmitry Torokhov wrote: > > On Friday, November 30, 2012 09:09:21 AM Greg KH wrote: > > > On Fri, Nov 30, 2012 at 08:47:46AM -0800, Andy King wrote: > > > > I didn't get the resend either, so it seems our corporate mail really > > > > is > > > > eating messages. Lovely. > > > > > > > > > > > +#define IOCTLCMD(_cmd) IOCTL_VMCI_ ## _cmd > > > > > > > > > > > > I don't recall ever getting a valid answer for this (if you did, > > > > > > my > > > > > > appologies, can you repeat it). What in the world are you talking > > > > > > about here? Why is your driver somehow special from the thousands > > > > > > of other ones that use the in-kernel IO macros properly for an > > > > > > ioctl? > > > > > > > > Because we're morons. And unfortunately, we've shipped our product > > > > using those broken definitions: our VMX uses them to talk to the > > > > driver. > > > > So here's what we'd like to do. We will send out a patch soon that > > > > fixes the other issues you mention and also adds IOCTL definitions the > > > > proper way using _IOBLAH(). But we'd also like to retain these broken > > > > definitions for a short period, commented as such, at least until we > > > > can get out a patch release to Workstation 9, at which point we can > > > > remove them. Does that sound reasonable? > > > > > > It has been my experience, that when people say "We will remove that api > > > sometime in the future", it never happens. So why not just do it now? > > > > > > Especially given that this code will be coming out in 3.9 at the > > > earliest, and that is 6 months away, so that should be plenty of time to > > > get this fixed up. > > > > Our schedule for releasing hosted products is not necessarily aligned > > with mainline kernel releases. > > And kernel developers don't really care about company schedules, nor > should they, you know this :) > That is why we are offering a compromise so that older installations have a chance to work and nobody has to care about schedules too much. However you snipped the rest of my reply: do we really need to renumber ioctls? There is no benefit for the driver as its ioctl handler does not parse the numbers into components. Thanks, Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 12/12] VMCI: Some header and config files.
On 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 00/12] VMCI for Linux upstreaming
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 12/12] VMCI: Some header and config files.
On Monday, November 26, 2012 04:32:39 PM Greg KH wrote:
> On Mon, Nov 26, 2012 at 04:23:57PM -0800, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > For some reason it still didn't go through to our corporate mail server
> > but I see it on LKML.
>
> Good.
>
> > On Mon, Nov 26, 2012 at 04:03:04PM -0800, Greg KH wrote:
> > > On Wed, Nov 07, 2012 at 10:43:03AM -0800, George Zhang wrote:
> > > > +static inline struct vmci_handle VMCI_MAKE_HANDLE(vmci_id cid,
> > > > vmci_id rid) +{
> > > > + struct vmci_handle h;
> > > > + h.context = cid;
> > > > + h.resource = rid;
> > > > + return h;
> > > > +}
> > >
> > > You return a structure on the stack that just went away? Yeah, I know
> > > it's an inline, but come on, that's not ok.
> >
> > This is certainly OK even if it is not inline, we return the _value_,
> > not the pointer to the stacki memory. And yes, the structure is 64 bit
> > value so it is returned in registers.
>
> Even on a 32bit processor?
I thought it would, but it looks like it won't. Maybe we'll just switch it
to a macro with C99 style initializators to keep the same semantic but
avoid the question.
> Also, you already have another function that
> does this same thing, so having 2 functions in the same patch seems odd,
> right?
Yes, you can say that it is probably a bit excessive.
OK, now that we are on the same page we'll go and fix the issues.
Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 12/12] VMCI: Some header and config files.
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
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: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming
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
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 471/493] net: remove use of __devexit
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 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
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 Thanks, 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
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 > Cc: Rusty Russell > Cc: "Michael S. Tsirkin" > Cc: Shreyas Bhatewara > Cc: "VMware, Inc." > Cc: Francois Romieu > 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 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.
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 00/12] VMCI for Linux upstreaming
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 00/12] VMCI for Linux upstreaming
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 08/12] VMCI: resource object implementation.
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 05/12] VMCI: event handling implementation.
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 01/12] VMCI: context implementation.
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 12/12] VMCI: Some header and config files.
On Mon, Oct 29, 2012 at 07:32:55PM -0700, Greg KH wrote: > On Mon, Oct 29, 2012 at 06:05:38PM -0700, George Zhang wrote: > > --- /dev/null > > +++ b/drivers/misc/vmw_vmci/Makefile > > @@ -0,0 +1,43 @@ > > + > > +# > > +# Linux driver for VMware's VMCI device. > > +# > > +# Copyright (C) 2007-2012, VMware, Inc. All Rights Reserved. > > +# > > +# This program is free software; you can redistribute it and/or modify it > > +# under the terms of the GNU General Public License as published by the > > +# Free Software Foundation; version 2 of the License and no later version. > > +# > > +# This program is distributed in the hope that it will be useful, but > > +# WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or > > +# NON INFRINGEMENT. See the GNU General Public License for more > > +# details. > > +# > > +# Maintained by: Andrew Stiegmann > > +# > > + > > That's a big boiler-plate for a makefile :) > > Wait, what's Andrew's name doing here, and yet it isn't on any of the > signed-off-by: or From: lines of the driver? Surely you aren't the only > contributor here? > > > +# > > +# Makefile for the VMware VMCI > > +# > > That's the only needed comment for this file, if even that. > > > + > > +obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci.o > > + > > +vmw_vmci-y += vmci_context.o > > +vmw_vmci-y += vmci_datagram.o > > +vmw_vmci-y += vmci_doorbell.o > > +vmw_vmci-y += vmci_driver.o > > +vmw_vmci-y += vmci_event.o > > +vmw_vmci-y += vmci_guest.o > > +vmw_vmci-y += vmci_handle_array.o > > +vmw_vmci-y += vmci_host.o > > +vmw_vmci-y += vmci_queue_pair.o > > +vmw_vmci-y += vmci_resource.o > > +vmw_vmci-y += vmci_route.o > > You can do this cleaner with multiple .o objects on the same line... > > > +vmci: > > + $(MAKE) -C ../../.. SUBDIRS=$$PWD CONFIG_VMWARE_VMCI=m modules > > + > > +clean: > > + $(MAKE) -C ../../.. SUBDIRS=$$PWD CONFIG_VMWARE_VMCI=m clean > > What are these two last targets for? I'm guessing this is from when you > ported it from a stand-along module? Please remove them. We'll clean it all up. Thanks for going over the code. -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 08/12] VMCI: resource object implementation.
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 08/12] VMCI: resource object implementation.
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;
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.
FWIW:
[dtor@dtor-ws kernel]$ grep -r BUG_ON . | wc -l
11269
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.
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 05/12] VMCI: event handling implementation.
On Mon, Oct 29, 2012 at 07:24:46PM -0700, Greg KH wrote:
> On Mon, Oct 29, 2012 at 06:04:27PM -0700, George Zhang wrote:
> > +/*
> > + * Releases the given VMCISubscription.
> > + * Fires the destroy event if the reference count has gone to zero.
> > + */
> > +static void event_release(struct vmci_subscription *entry)
> > +{
> > + kref_put(&entry->kref, event_signal_destroy);
> > +}
>
> Same question as before with the kref_put() call, what is handling the
> locking here? It looks like a race to me.
The reference is taken only if event is on the list (which managed by
RCU and a mutex), so it is not possible to go from 0->1 for that
refcount.
Thanks,
Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 04/12] VMCI: device driver implementaton.
On Mon, Oct 29, 2012 at 07:23:47PM -0700, Greg KH wrote:
> On Mon, Oct 29, 2012 at 06:04:15PM -0700, George Zhang wrote:
> > +static int __init vmci_core_init(void)
> > +{
> > + int result;
> > +
> > + result = vmci_ctx_init();
> > + if (result < VMCI_SUCCESS) {
> > + pr_err("Failed to initialize VMCIContext (result=%d).\n",
> > + result);
>
> If you are going to use pr_* functions, it's usually a good idea to
> define pr_fmt in a consistant way so that it shows up the same across
> all of your .c files. See other drivers for examples of how to do this
> properly.
pr_fmt() is defined in drivers/misc/vmw_vmci/vmci_common_int.h
>
> > + return -EINVAL;
> > + }
> > +
> > + result = vmci_datagram_init();
> > + if (result < VMCI_SUCCESS) {
> > + pr_err("Failed to initialize VMCIDatagram (result=%d).\n",
> > + result);
> > + return -EINVAL;
> > + }
> > +
> > + result = vmci_event_init();
> > + if (result < VMCI_SUCCESS) {
> > + pr_err("Failed to initialize VMCIEvent (result=%d).\n", result);
> > + return -EINVAL;
> > + }
>
> You don't free and unwind things properly if things go wrong here, why
> not?
We do, the above calls do not need to be cleaned up.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit vmci_core_exit(void)
> > +{
> > + vmci_event_exit();
> > +}
> > +
> > +static int __init vmci_drv_init(void)
> > +{
> > + int error;
> > +
> > + error = vmci_core_init();
> > + if (error)
> > + return error;
> > +
> > + if (!vmci_disable_guest) {
> > + error = vmci_guest_init();
> > + if (error) {
> > + pr_warn("Failed to initialize guest personality
> > (err=%d).\n",
> > + error);
> > + } else {
> > + vmci_guest_personality_initialized = true;
> > + pr_info("Guest personality initialized and is %s\n",
> > + vmci_guest_code_active() ?
> > + "active" : "inactive");
> > + }
> > + }
> > +
> > + if (!vmci_disable_host) {
> > + error = vmci_host_init();
> > + if (error) {
> > + pr_warn("Unable to initialize host personality
> > (err=%d).\n",
> > + error);
> > + } else {
> > + vmci_host_personality_initialized = true;
> > + pr_info("Initialized host personality\n");
> > + }
> > + }
> > +
> > + if (!vmci_guest_personality_initialized &&
> > + !vmci_host_personality_initialized) {
> > + vmci_core_exit();
> > + return -ENODEV;
> > + }
> > +
> > + pr_debug("Module is initialized\n");
>
> Really? You need this message still?
These are debug messages so do not show up in logs by default.
>
> > + return 0;
> > +}
> > +module_init(vmci_drv_init);
> > +
> > +static void __exit vmci_drv_exit(void)
> > +{
> > + if (vmci_guest_personality_initialized)
> > + vmci_guest_exit();
> > +
> > + if (vmci_host_personality_initialized)
> > + vmci_host_exit();
> > +
> > + vmci_core_exit();
> > + pr_debug("Module is unloaded\n");
>
> Same here, is this really needed?
>
Still debug so no littering in logs.
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
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... 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. 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.
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.
I believe the comment should actually read that the function should not
be called from atomic contexts.
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.
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] [PATCH 04/10] VMCI: device driver implementaton.
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.
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] [vmw_vmci 11/11] Apply the header code to make VMCI build
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
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: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build
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: [vmw_vmci RFC 00/11] VMCI for Linux
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 v2] xen kconfig: relax INPUT_XEN_KBDDEV_FRONTEND deps
On Thu, Mar 15, 2012 at 01:23:05PM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Jan 11, 2012 at 11:29:20AM -0500, Andrew Jones wrote: > > > > > > - Original Message - > > > On Mon, Jan 09, 2012 at 06:51:41PM +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 > > > > > > Ok, but PV does? > > > > 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. > > > > > > That sounds like it would be universal irregardless if it is > > > PV or PVonHVM? > > > > This patch makes it such that if you want to use both, then you must > > select both. It also says that if you want FB, then you need the > > KBD. However, if you only want the KBD then you're fine with just > > that. So there isn't any risk of breaking configs designed to use > > FB, because FB should be manually selected for those configs anyway. > > Dmitry, > > I am OK with this patch. Should I pick it up on my tree for 3.4 or > are you OK doing it via your tree? Konrad, I don't have a good copy of the patch so it you could pick it up that would be great. 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
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 > > (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 (maintainer:INPUT > > (KEYBOARD,...,commit_signer:9/16=56%) > > Samuel Ortiz (commit_signer:3/16=19%) > > Anirudh Ghayal (commit_signer:2/16=12%) > > Peter Ujfalusi (commit_signer:2/16=12%) > > Alan Cox (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 > > > --- > > > 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
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
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: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
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
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: [PATCH] input: Fix interrupt enable in i8042_ctr when enabling interrupt fails
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
