[Qemu-devel] [PATCH 0/2 v2] xenfb: Fix protocol for HVM guests

[Qemu-devel] [PATCH 0/2 v2] xenfb: Fix protocol for HVM guests

[Owen Smith]

This series is intended to alow HVM guests to use the vkbd device with a
PV frontend driver. Initial version at:
http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenvkbd.git;a=tree

Since the vkbd device is initialised for HVM guests, using the vkbd device
can allow the disabling of the USB controller and USB tablet and maintaining
the same easy to use absolute pointer. The main problem, is that the vkbd 
device does not connect unless a vfb device is present, to facilitate the
rescaling of absolute coordinates.
This series rearranges the input handlers to use the qemu_input_handler_*
functions, by using the callbacks directly, rather than via the compatability
layer in input-legacy.c. The series also registers the "feature-raw-pointer"
feature to indicate the backend can supply raw unscaled absolute cordinates.
The frontend uses "request-raw-pointer" to request raw unscaled values.

Adding the feature is also to enable detecting the difference between older,
broken, backend and newer backends that can connect without the vfb present.

v2:
* reworks the input handlers to use qemu_input_handler_* functions
* rename the feature proposed to a better name - the name better reflects the
  intended use-case (raw pointer vs backend requires vfb device)

Owen Smith (2):
  xenfb: Use qemu_input_handler_* calls directly
  xenfb: Allow vkbd to connect without a DisplayState

 hw/display/xenfb.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 139 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH 0/2 v2] xenfb: Fix protocol for HVM guests

[Owen Smith]

This series is intended to alow HVM guests to use the vkbd device with a
PV frontend driver. Initial version at:
http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenvkbd.git;a=tree

Since the vkbd device is initialised for HVM guests, using the vkbd device
can allow the disabling of the USB controller and USB tablet and maintaining
the same easy to use absolute pointer. The main problem, is that the vkbd 
device does not connect unless a vfb device is present, to facilitate the
rescaling of absolute coordinates.
This series rearranges the input handlers to use the qemu_input_handler_*
functions, by using the callbacks directly, rather than via the compatability
layer in input-legacy.c. The series also registers the "feature-raw-pointer"
feature to indicate the backend can supply raw unscaled absolute cordinates.
The frontend uses "request-raw-pointer" to request raw unscaled values.

Adding the feature is also to enable detecting the difference between older,
broken, backend and newer backends that can connect without the vfb present.

v2:
* reworks the input handlers to use qemu_input_handler_* functions
* rename the feature proposed to a better name - the name better reflects the
  intended use-case (raw pointer vs backend requires vfb device)

Owen Smith (2):
  xenfb: Use qemu_input_handler_* calls directly
  xenfb: Allow vkbd to connect without a DisplayState

 hw/display/xenfb.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 139 insertions(+), 18 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [PATCH 1/2 v2] xenfb: Use qemu_input_handler_* calls directly

[Owen Smith]

The xenvkbd input device uses functions from input-legacy.c
Use the appropriate qemu_input_handler_* functions instead
of calling functions in input-legacy.c that in turn call
the correct functions.
The bulk of this patch removes the extra layer of calls
by moving the required structure members into the XenInput
struct.

Signed-off-by: Owen Smith <owen.smith@citrix.com>
---
 hw/display/xenfb.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 113 insertions(+), 8 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index e76c0d8..88815df 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -27,6 +27,7 @@
 #include "qemu/osdep.h"
 
 #include "hw/hw.h"
+#include "ui/input.h"
 #include "ui/console.h"
 #include "hw/xen/xen_backend.h"
 
@@ -54,7 +55,14 @@ struct XenInput {
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
     int button_state;       /* Last seen pointer button state */
     int extended;
-    QEMUPutMouseEntry *qmouse;
+    /* kbd */
+    QemuInputHandler hkbd;
+    QemuInputHandlerState *qkbd;
+    /* mouse */
+    QemuInputHandler hmouse;
+    QemuInputHandlerState *qmouse;
+    int axis[INPUT_AXIS__MAX];
+    int buttons;
 };
 
 #define UP_QUEUE 8
@@ -293,6 +301,21 @@ static void xenfb_key_event(void *opaque, int scancode)
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
 }
 
+static void xenfb_legacy_key_event(DeviceState *dev, QemuConsole *src,
+                                   InputEvent *evt)
+{
+    struct XenInput *in = (struct XenInput *)dev;
+    int scancodes[3], i, count;
+    InputKeyEvent *key = evt->u.key.data;
+
+    count = qemu_input_key_value_to_scancode(key->key,
+                                             key->down,
+                                             scancodes);
+    for (i = 0; i < count; ++i) {
+        xenfb_key_event(in, scancodes[i]);
+    }
+}
+
 /*
  * Send a mouse event from the client to the guest OS
  *
@@ -333,6 +356,70 @@ static void xenfb_mouse_event(void *opaque,
     xenfb->button_state = button_state;
 }
 
+static void xenfb_legacy_mouse_event(DeviceState *dev, QemuConsole *src,
+                                     InputEvent *evt)
+{
+    static const int bmap[INPUT_BUTTON__MAX] = {
+        [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
+        [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
+        [INPUT_BUTTON_RIGHT]  = MOUSE_EVENT_RBUTTON,
+    };
+    struct XenInput *in = (struct XenInput *)dev;
+    InputBtnEvent *btn;
+    InputMoveEvent *move;
+
+    switch (evt->type) {
+    case INPUT_EVENT_KIND_BTN:
+        btn = evt->u.btn.data;
+        if (btn->down) {
+            in->buttons |= bmap[btn->button];
+        } else {
+            in->buttons &= ~bmap[btn->button];
+        }
+        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_UP) {
+            xenfb_mouse_event(in,
+                              in->axis[INPUT_AXIS_X],
+                              in->axis[INPUT_AXIS_Y],
+                              -1,
+                              in->buttons);
+        }
+        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+            xenfb_mouse_event(in,
+                              in->axis[INPUT_AXIS_X],
+                              in->axis[INPUT_AXIS_Y],
+                              1,
+                              in->buttons);
+        }
+        break;
+    case INPUT_EVENT_KIND_ABS:
+        move = evt->u.abs.data;
+        in->axis[move->axis] = move->value;
+        break;
+    case INPUT_EVENT_KIND_REL:
+        move = evt->u.rel.data;
+        in->axis[move->axis] += move->value;
+        break;
+    default:
+        break;
+    }
+}
+
+static void xenfb_legacy_mouse_sync(DeviceState *dev)
+{
+    struct XenInput *in = (struct XenInput *)dev;
+
+    xenfb_mouse_event(in,
+                      in->axis[INPUT_AXIS_X],
+                      in->axis[INPUT_AXIS_Y],
+                      0,
+                      in->buttons);
+
+    if (!in->abs_pointer_wanted) {
+        in->axis[INPUT_AXIS_X] = 0;
+        in->axis[INPUT_AXIS_Y] = 0;
+    }
+}
+
 static int input_init(struct XenDevice *xendev)
 {
     xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
@@ -353,7 +440,6 @@ static int input_initialise(struct XenDevice *xendev)
     if (rc != 0)
 	return rc;
 
-    qemu_add_kbd_event_handler(xenfb_key_event, in);
     return 0;
 }
 
@@ -366,24 +452,43 @@ static void input_connected(struct XenDevice *xendev)
         in->abs_pointer_wanted = 0;
     }
 
+    if (in->qkbd) {
+        qemu_input_handler_unregister(in->qkbd);
+    }
     if (in->qmouse) {
-        qemu_remove_mouse_event_handler(in->qmouse);
+        qemu_input_handler_unregister(in->qmouse);
     }
     trace_xenfb_input_connected(xendev, in->abs_pointer_wanted);
-    in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
-					      in->abs_pointer_wanted,
-					      "Xen PVFB Mouse");
+
+    in->hkbd.name = "legacy-kbd";
+    in->hkbd.mask = INPUT_EVENT_MASK_KEY;
+    in->hkbd.event = xenfb_legacy_key_event;
+    in->qkbd = qemu_input_handler_register((DeviceState *)in,
+                                           &in->hkbd);
+    qemu_input_handler_activate(in->qkbd);
+
+    in->hmouse.name = "Xen PVFB Mouse";
+    in->hmouse.mask = INPUT_EVENT_MASK_BTN |
+        (in->abs_pointer_wanted ? INPUT_EVENT_MASK_ABS : INPUT_EVENT_MASK_REL);
+    in->hmouse.event = xenfb_legacy_mouse_event;
+    in->hmouse.sync = xenfb_legacy_mouse_sync;
+    in->qmouse = qemu_input_handler_register((DeviceState *)in,
+                                             &in->hmouse);
+    qemu_input_handler_activate(in->qmouse);
 }
 
 static void input_disconnect(struct XenDevice *xendev)
 {
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
 
+    if (in->qkbd) {
+        qemu_input_handler_unregister(in->qkbd);
+        in->qkbd = NULL;
+    }
     if (in->qmouse) {
-	qemu_remove_mouse_event_handler(in->qmouse);
+        qemu_input_handler_unregister(in->qmouse);
 	in->qmouse = NULL;
     }
-    qemu_add_kbd_event_handler(NULL, NULL);
     common_unbind(&in->c);
 }
 
-- 
2.1.4

[PATCH 1/2 v2] xenfb: Use qemu_input_handler_* calls directly

[Owen Smith]

The xenvkbd input device uses functions from input-legacy.c
Use the appropriate qemu_input_handler_* functions instead
of calling functions in input-legacy.c that in turn call
the correct functions.
The bulk of this patch removes the extra layer of calls
by moving the required structure members into the XenInput
struct.

Signed-off-by: Owen Smith <owen.smith@citrix.com>
---
 hw/display/xenfb.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 113 insertions(+), 8 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index e76c0d8..88815df 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -27,6 +27,7 @@
 #include "qemu/osdep.h"
 
 #include "hw/hw.h"
+#include "ui/input.h"
 #include "ui/console.h"
 #include "hw/xen/xen_backend.h"
 
@@ -54,7 +55,14 @@ struct XenInput {
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
     int button_state;       /* Last seen pointer button state */
     int extended;
-    QEMUPutMouseEntry *qmouse;
+    /* kbd */
+    QemuInputHandler hkbd;
+    QemuInputHandlerState *qkbd;
+    /* mouse */
+    QemuInputHandler hmouse;
+    QemuInputHandlerState *qmouse;
+    int axis[INPUT_AXIS__MAX];
+    int buttons;
 };
 
 #define UP_QUEUE 8
@@ -293,6 +301,21 @@ static void xenfb_key_event(void *opaque, int scancode)
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
 }
 
+static void xenfb_legacy_key_event(DeviceState *dev, QemuConsole *src,
+                                   InputEvent *evt)
+{
+    struct XenInput *in = (struct XenInput *)dev;
+    int scancodes[3], i, count;
+    InputKeyEvent *key = evt->u.key.data;
+
+    count = qemu_input_key_value_to_scancode(key->key,
+                                             key->down,
+                                             scancodes);
+    for (i = 0; i < count; ++i) {
+        xenfb_key_event(in, scancodes[i]);
+    }
+}
+
 /*
  * Send a mouse event from the client to the guest OS
  *
@@ -333,6 +356,70 @@ static void xenfb_mouse_event(void *opaque,
     xenfb->button_state = button_state;
 }
 
+static void xenfb_legacy_mouse_event(DeviceState *dev, QemuConsole *src,
+                                     InputEvent *evt)
+{
+    static const int bmap[INPUT_BUTTON__MAX] = {
+        [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
+        [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
+        [INPUT_BUTTON_RIGHT]  = MOUSE_EVENT_RBUTTON,
+    };
+    struct XenInput *in = (struct XenInput *)dev;
+    InputBtnEvent *btn;
+    InputMoveEvent *move;
+
+    switch (evt->type) {
+    case INPUT_EVENT_KIND_BTN:
+        btn = evt->u.btn.data;
+        if (btn->down) {
+            in->buttons |= bmap[btn->button];
+        } else {
+            in->buttons &= ~bmap[btn->button];
+        }
+        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_UP) {
+            xenfb_mouse_event(in,
+                              in->axis[INPUT_AXIS_X],
+                              in->axis[INPUT_AXIS_Y],
+                              -1,
+                              in->buttons);
+        }
+        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+            xenfb_mouse_event(in,
+                              in->axis[INPUT_AXIS_X],
+                              in->axis[INPUT_AXIS_Y],
+                              1,
+                              in->buttons);
+        }
+        break;
+    case INPUT_EVENT_KIND_ABS:
+        move = evt->u.abs.data;
+        in->axis[move->axis] = move->value;
+        break;
+    case INPUT_EVENT_KIND_REL:
+        move = evt->u.rel.data;
+        in->axis[move->axis] += move->value;
+        break;
+    default:
+        break;
+    }
+}
+
+static void xenfb_legacy_mouse_sync(DeviceState *dev)
+{
+    struct XenInput *in = (struct XenInput *)dev;
+
+    xenfb_mouse_event(in,
+                      in->axis[INPUT_AXIS_X],
+                      in->axis[INPUT_AXIS_Y],
+                      0,
+                      in->buttons);
+
+    if (!in->abs_pointer_wanted) {
+        in->axis[INPUT_AXIS_X] = 0;
+        in->axis[INPUT_AXIS_Y] = 0;
+    }
+}
+
 static int input_init(struct XenDevice *xendev)
 {
     xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
@@ -353,7 +440,6 @@ static int input_initialise(struct XenDevice *xendev)
     if (rc != 0)
 	return rc;
 
-    qemu_add_kbd_event_handler(xenfb_key_event, in);
     return 0;
 }
 
@@ -366,24 +452,43 @@ static void input_connected(struct XenDevice *xendev)
         in->abs_pointer_wanted = 0;
     }
 
+    if (in->qkbd) {
+        qemu_input_handler_unregister(in->qkbd);
+    }
     if (in->qmouse) {
-        qemu_remove_mouse_event_handler(in->qmouse);
+        qemu_input_handler_unregister(in->qmouse);
     }
     trace_xenfb_input_connected(xendev, in->abs_pointer_wanted);
-    in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
-					      in->abs_pointer_wanted,
-					      "Xen PVFB Mouse");
+
+    in->hkbd.name = "legacy-kbd";
+    in->hkbd.mask = INPUT_EVENT_MASK_KEY;
+    in->hkbd.event = xenfb_legacy_key_event;
+    in->qkbd = qemu_input_handler_register((DeviceState *)in,
+                                           &in->hkbd);
+    qemu_input_handler_activate(in->qkbd);
+
+    in->hmouse.name = "Xen PVFB Mouse";
+    in->hmouse.mask = INPUT_EVENT_MASK_BTN |
+        (in->abs_pointer_wanted ? INPUT_EVENT_MASK_ABS : INPUT_EVENT_MASK_REL);
+    in->hmouse.event = xenfb_legacy_mouse_event;
+    in->hmouse.sync = xenfb_legacy_mouse_sync;
+    in->qmouse = qemu_input_handler_register((DeviceState *)in,
+                                             &in->hmouse);
+    qemu_input_handler_activate(in->qmouse);
 }
 
 static void input_disconnect(struct XenDevice *xendev)
 {
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
 
+    if (in->qkbd) {
+        qemu_input_handler_unregister(in->qkbd);
+        in->qkbd = NULL;
+    }
     if (in->qmouse) {
-	qemu_remove_mouse_event_handler(in->qmouse);
+        qemu_input_handler_unregister(in->qmouse);
 	in->qmouse = NULL;
     }
-    qemu_add_kbd_event_handler(NULL, NULL);
     common_unbind(&in->c);
 }
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [PATCH 2/2 v2] xenfb: Allow vkbd to connect without a DisplayState

[Owen Smith]

If the vkbd device model is registered and the vfb device model
is not registered, the backend will not transition to connected.
If there is no DisplayState, then the absolute coordinates cannot
be scaled, and will remain in the range [0, 0x7fff].
Backend writes "feature-raw-pointer" to indicate that the backend
supports reporting absolute position without rescaling.
The frontend uses "request-raw-pointer" to request raw unscaled
pointer values. If there is no DisplayState, the absolute values
are always raw unscaled values.

Signed-off-by: Owen Smith <owen.smith@citrix.com>
---
 hw/display/xenfb.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 88815df..d40af6e 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -53,6 +53,7 @@ struct common {
 struct XenInput {
     struct common c;
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
+    int raw_pointer_wanted; /* Whether guest supports unscaled pointer */
     int button_state;       /* Last seen pointer button state */
     int extended;
     /* kbd */
@@ -329,18 +330,22 @@ static void xenfb_mouse_event(void *opaque,
 			      int dx, int dy, int dz, int button_state)
 {
     struct XenInput *xenfb = opaque;
-    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
-    int dw = surface_width(surface);
-    int dh = surface_height(surface);
-    int i;
+    int i, x, y;
+    if (xenfb->c.con && xenfb->raw_pointer_wanted != 1) {
+        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
+        int dw = surface_width(surface);
+        int dh = surface_height(surface);
+        x = dx * (dw - 1) / 0x7fff;
+        y = dy * (dh - 1) / 0x7fff;
+    } else {
+        x = dx;
+        y = dy;
+    }
 
     trace_xenfb_mouse_event(opaque, dx, dy, dz, button_state,
                             xenfb->abs_pointer_wanted);
     if (xenfb->abs_pointer_wanted)
-	xenfb_send_position(xenfb,
-			    dx * (dw - 1) / 0x7fff,
-			    dy * (dh - 1) / 0x7fff,
-			    dz);
+        xenfb_send_position(xenfb, x, y, dz);
     else
 	xenfb_send_motion(xenfb, dx, dy, dz);
 
@@ -423,6 +428,7 @@ static void xenfb_legacy_mouse_sync(DeviceState *dev)
 static int input_init(struct XenDevice *xendev)
 {
     xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
+    xenstore_write_be_int(xendev, "feature-raw-pointer", 1);
     return 0;
 }
 
@@ -432,8 +438,14 @@ static int input_initialise(struct XenDevice *xendev)
     int rc;
 
     if (!in->c.con) {
-        xen_pv_printf(xendev, 1, "ds not set (yet)\n");
-        return -1;
+        char *vfb = xenstore_read_str(NULL, "device/vfb");
+        if (vfb == NULL) {
+            /* there is no vfb, run vkbd on its own */
+        } else {
+            free(vfb);
+            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
+            return -1;
+        }
     }
 
     rc = common_bind(&in->c);
@@ -451,6 +463,10 @@ static void input_connected(struct XenDevice *xendev)
                              &in->abs_pointer_wanted) == -1) {
         in->abs_pointer_wanted = 0;
     }
+    if (xenstore_read_fe_int(xendev, "request-raw-pointer",
+                             &in->raw_pointer_wanted) == -1) {
+        in->raw_pointer_wanted = 0;
+    }
 
     if (in->qkbd) {
         qemu_input_handler_unregister(in->qkbd);
-- 
2.1.4

[PATCH 2/2 v2] xenfb: Allow vkbd to connect without a DisplayState

[Owen Smith]

If the vkbd device model is registered and the vfb device model
is not registered, the backend will not transition to connected.
If there is no DisplayState, then the absolute coordinates cannot
be scaled, and will remain in the range [0, 0x7fff].
Backend writes "feature-raw-pointer" to indicate that the backend
supports reporting absolute position without rescaling.
The frontend uses "request-raw-pointer" to request raw unscaled
pointer values. If there is no DisplayState, the absolute values
are always raw unscaled values.

Signed-off-by: Owen Smith <owen.smith@citrix.com>
---
 hw/display/xenfb.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 88815df..d40af6e 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -53,6 +53,7 @@ struct common {
 struct XenInput {
     struct common c;
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
+    int raw_pointer_wanted; /* Whether guest supports unscaled pointer */
     int button_state;       /* Last seen pointer button state */
     int extended;
     /* kbd */
@@ -329,18 +330,22 @@ static void xenfb_mouse_event(void *opaque,
 			      int dx, int dy, int dz, int button_state)
 {
     struct XenInput *xenfb = opaque;
-    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
-    int dw = surface_width(surface);
-    int dh = surface_height(surface);
-    int i;
+    int i, x, y;
+    if (xenfb->c.con && xenfb->raw_pointer_wanted != 1) {
+        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
+        int dw = surface_width(surface);
+        int dh = surface_height(surface);
+        x = dx * (dw - 1) / 0x7fff;
+        y = dy * (dh - 1) / 0x7fff;
+    } else {
+        x = dx;
+        y = dy;
+    }
 
     trace_xenfb_mouse_event(opaque, dx, dy, dz, button_state,
                             xenfb->abs_pointer_wanted);
     if (xenfb->abs_pointer_wanted)
-	xenfb_send_position(xenfb,
-			    dx * (dw - 1) / 0x7fff,
-			    dy * (dh - 1) / 0x7fff,
-			    dz);
+        xenfb_send_position(xenfb, x, y, dz);
     else
 	xenfb_send_motion(xenfb, dx, dy, dz);
 
@@ -423,6 +428,7 @@ static void xenfb_legacy_mouse_sync(DeviceState *dev)
 static int input_init(struct XenDevice *xendev)
 {
     xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
+    xenstore_write_be_int(xendev, "feature-raw-pointer", 1);
     return 0;
 }
 
@@ -432,8 +438,14 @@ static int input_initialise(struct XenDevice *xendev)
     int rc;
 
     if (!in->c.con) {
-        xen_pv_printf(xendev, 1, "ds not set (yet)\n");
-        return -1;
+        char *vfb = xenstore_read_str(NULL, "device/vfb");
+        if (vfb == NULL) {
+            /* there is no vfb, run vkbd on its own */
+        } else {
+            free(vfb);
+            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
+            return -1;
+        }
     }
 
     rc = common_bind(&in->c);
@@ -451,6 +463,10 @@ static void input_connected(struct XenDevice *xendev)
                              &in->abs_pointer_wanted) == -1) {
         in->abs_pointer_wanted = 0;
     }
+    if (xenstore_read_fe_int(xendev, "request-raw-pointer",
+                             &in->raw_pointer_wanted) == -1) {
+        in->raw_pointer_wanted = 0;
+    }
 
     if (in->qkbd) {
         qemu_input_handler_unregister(in->qkbd);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2 v2] xenfb: Allow vkbd to connect without a DisplayState

[Oleksandr Grytsov]

On Mon, Jul 3, 2017 at 4:17 PM, Owen Smith <owen.smith@citrix.com> wrote:
> If the vkbd device model is registered and the vfb device model
> is not registered, the backend will not transition to connected.
> If there is no DisplayState, then the absolute coordinates cannot
> be scaled, and will remain in the range [0, 0x7fff].
> Backend writes "feature-raw-pointer" to indicate that the backend
> supports reporting absolute position without rescaling.
> The frontend uses "request-raw-pointer" to request raw unscaled
> pointer values. If there is no DisplayState, the absolute values
> are always raw unscaled values.
>
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 88815df..d40af6e 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -53,6 +53,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int raw_pointer_wanted; /* Whether guest supports unscaled pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      /* kbd */
> @@ -329,18 +330,22 @@ static void xenfb_mouse_event(void *opaque,
>                               int dx, int dy, int dz, int button_state)
>  {
>      struct XenInput *xenfb = opaque;
> -    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> -    int dw = surface_width(surface);
> -    int dh = surface_height(surface);
> -    int i;
> +    int i, x, y;
> +    if (xenfb->c.con && xenfb->raw_pointer_wanted != 1) {
> +        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> +        int dw = surface_width(surface);
> +        int dh = surface_height(surface);
> +        x = dx * (dw - 1) / 0x7fff;
> +        y = dy * (dh - 1) / 0x7fff;
> +    } else {
> +        x = dx;
> +        y = dy;
> +    }
>
>      trace_xenfb_mouse_event(opaque, dx, dy, dz, button_state,
>                              xenfb->abs_pointer_wanted);
>      if (xenfb->abs_pointer_wanted)
> -       xenfb_send_position(xenfb,
> -                           dx * (dw - 1) / 0x7fff,
> -                           dy * (dh - 1) / 0x7fff,
> -                           dz);
> +        xenfb_send_position(xenfb, x, y, dz);
>      else
>         xenfb_send_motion(xenfb, dx, dy, dz);
>
> @@ -423,6 +428,7 @@ static void xenfb_legacy_mouse_sync(DeviceState *dev)
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> +    xenstore_write_be_int(xendev, "feature-raw-pointer", 1);
>      return 0;
>  }
>
> @@ -432,8 +438,14 @@ static int input_initialise(struct XenDevice *xendev)
>      int rc;
>
>      if (!in->c.con) {

It doesn't look like proper way. If I understand this condition
correctly, you are trying to
launch the vkbd backend even if there is no fb configured. It means
the vkbd backend
will be always startedeven if it is not needed.

I'm working on the patch which allows to launch vkbd backend separately from fb.
We need it to be able to launch our own backend in user space [1].

I think proper way is:
1. Add standalone vkbd configuration to xl.cfg.
2. Redesign xen_init_display in way it allows to start vkbd without vfb.

The item 1. will be in my patch.

> -        xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> -        return -1;
> +        char *vfb = xenstore_read_str(NULL, "device/vfb");

This xenstore_read_str will not work as expected. I guess
this line will try to read entry from "(null)/device/vfb" which is
always doesn't exist. See xenstore_read_str implementation.

> +        if (vfb == NULL) {
> +            /* there is no vfb, run vkbd on its own */
> +        } else {
> +            free(vfb);
> +            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> +            return -1;
> +        }
>      }
>
>      rc = common_bind(&in->c);
> @@ -451,6 +463,10 @@ static void input_connected(struct XenDevice *xendev)
>                               &in->abs_pointer_wanted) == -1) {
>          in->abs_pointer_wanted = 0;
>      }
> +    if (xenstore_read_fe_int(xendev, "request-raw-pointer",
> +                             &in->raw_pointer_wanted) == -1) {
> +        in->raw_pointer_wanted = 0;
> +    }
>
>      if (in->qkbd) {
>          qemu_input_handler_unregister(in->qkbd);
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

[1] http://marc.info/?l=qemu-devel&m=149266892429889&w=2

Re: [PATCH 2/2 v2] xenfb: Allow vkbd to connect without a DisplayState

[Oleksandr Grytsov]

On Mon, Jul 3, 2017 at 4:17 PM, Owen Smith <owen.smith@citrix.com> wrote:
> If the vkbd device model is registered and the vfb device model
> is not registered, the backend will not transition to connected.
> If there is no DisplayState, then the absolute coordinates cannot
> be scaled, and will remain in the range [0, 0x7fff].
> Backend writes "feature-raw-pointer" to indicate that the backend
> supports reporting absolute position without rescaling.
> The frontend uses "request-raw-pointer" to request raw unscaled
> pointer values. If there is no DisplayState, the absolute values
> are always raw unscaled values.
>
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 88815df..d40af6e 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -53,6 +53,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int raw_pointer_wanted; /* Whether guest supports unscaled pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      /* kbd */
> @@ -329,18 +330,22 @@ static void xenfb_mouse_event(void *opaque,
>                               int dx, int dy, int dz, int button_state)
>  {
>      struct XenInput *xenfb = opaque;
> -    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> -    int dw = surface_width(surface);
> -    int dh = surface_height(surface);
> -    int i;
> +    int i, x, y;
> +    if (xenfb->c.con && xenfb->raw_pointer_wanted != 1) {
> +        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> +        int dw = surface_width(surface);
> +        int dh = surface_height(surface);
> +        x = dx * (dw - 1) / 0x7fff;
> +        y = dy * (dh - 1) / 0x7fff;
> +    } else {
> +        x = dx;
> +        y = dy;
> +    }
>
>      trace_xenfb_mouse_event(opaque, dx, dy, dz, button_state,
>                              xenfb->abs_pointer_wanted);
>      if (xenfb->abs_pointer_wanted)
> -       xenfb_send_position(xenfb,
> -                           dx * (dw - 1) / 0x7fff,
> -                           dy * (dh - 1) / 0x7fff,
> -                           dz);
> +        xenfb_send_position(xenfb, x, y, dz);
>      else
>         xenfb_send_motion(xenfb, dx, dy, dz);
>
> @@ -423,6 +428,7 @@ static void xenfb_legacy_mouse_sync(DeviceState *dev)
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> +    xenstore_write_be_int(xendev, "feature-raw-pointer", 1);
>      return 0;
>  }
>
> @@ -432,8 +438,14 @@ static int input_initialise(struct XenDevice *xendev)
>      int rc;
>
>      if (!in->c.con) {

It doesn't look like proper way. If I understand this condition
correctly, you are trying to
launch the vkbd backend even if there is no fb configured. It means
the vkbd backend
will be always startedeven if it is not needed.

I'm working on the patch which allows to launch vkbd backend separately from fb.
We need it to be able to launch our own backend in user space [1].

I think proper way is:
1. Add standalone vkbd configuration to xl.cfg.
2. Redesign xen_init_display in way it allows to start vkbd without vfb.

The item 1. will be in my patch.

> -        xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> -        return -1;
> +        char *vfb = xenstore_read_str(NULL, "device/vfb");

This xenstore_read_str will not work as expected. I guess
this line will try to read entry from "(null)/device/vfb" which is
always doesn't exist. See xenstore_read_str implementation.

> +        if (vfb == NULL) {
> +            /* there is no vfb, run vkbd on its own */
> +        } else {
> +            free(vfb);
> +            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> +            return -1;
> +        }
>      }
>
>      rc = common_bind(&in->c);
> @@ -451,6 +463,10 @@ static void input_connected(struct XenDevice *xendev)
>                               &in->abs_pointer_wanted) == -1) {
>          in->abs_pointer_wanted = 0;
>      }
> +    if (xenstore_read_fe_int(xendev, "request-raw-pointer",
> +                             &in->raw_pointer_wanted) == -1) {
> +        in->raw_pointer_wanted = 0;
> +    }
>
>      if (in->qkbd) {
>          qemu_input_handler_unregister(in->qkbd);
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

[1] http://marc.info/?l=qemu-devel&m=149266892429889&w=2

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 1/2 v2] xenfb: Use qemu_input_handler_* calls directly

[Stefano Stabellini]

On Mon, 3 Jul 2017, Owen Smith wrote:
> The xenvkbd input device uses functions from input-legacy.c
> Use the appropriate qemu_input_handler_* functions instead
> of calling functions in input-legacy.c that in turn call
> the correct functions.
> The bulk of this patch removes the extra layer of calls
> by moving the required structure members into the XenInput
> struct.
> 
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 113 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index e76c0d8..88815df 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -27,6 +27,7 @@
>  #include "qemu/osdep.h"
>  
>  #include "hw/hw.h"
> +#include "ui/input.h"
>  #include "ui/console.h"
>  #include "hw/xen/xen_backend.h"
>  
> @@ -54,7 +55,14 @@ struct XenInput {
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
> -    QEMUPutMouseEntry *qmouse;
> +    /* kbd */
> +    QemuInputHandler hkbd;
> +    QemuInputHandlerState *qkbd;
> +    /* mouse */
> +    QemuInputHandler hmouse;
> +    QemuInputHandlerState *qmouse;
> +    int axis[INPUT_AXIS__MAX];
> +    int buttons;
>  };
>  
>  #define UP_QUEUE 8
> @@ -293,6 +301,21 @@ static void xenfb_key_event(void *opaque, int scancode)
>      xenfb_send_key(xenfb, down, scancode2linux[scancode]);
>  }
>  
> +static void xenfb_legacy_key_event(DeviceState *dev, QemuConsole *src,
> +                                   InputEvent *evt)
> +{
> +    struct XenInput *in = (struct XenInput *)dev;
> +    int scancodes[3], i, count;
> +    InputKeyEvent *key = evt->u.key.data;
> +
> +    count = qemu_input_key_value_to_scancode(key->key,
> +                                             key->down,
> +                                             scancodes);
> +    for (i = 0; i < count; ++i) {
> +        xenfb_key_event(in, scancodes[i]);
> +    }
> +}
>  /*
>   * Send a mouse event from the client to the guest OS
>   *
> @@ -333,6 +356,70 @@ static void xenfb_mouse_event(void *opaque,
>      xenfb->button_state = button_state;
>  }
>  
> +static void xenfb_legacy_mouse_event(DeviceState *dev, QemuConsole *src,
> +                                     InputEvent *evt)
> +{
> +    static const int bmap[INPUT_BUTTON__MAX] = {
> +        [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
> +        [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
> +        [INPUT_BUTTON_RIGHT]  = MOUSE_EVENT_RBUTTON,
> +    };
> +    struct XenInput *in = (struct XenInput *)dev;
> +    InputBtnEvent *btn;
> +    InputMoveEvent *move;
> +
> +    switch (evt->type) {
> +    case INPUT_EVENT_KIND_BTN:
> +        btn = evt->u.btn.data;
> +        if (btn->down) {
> +            in->buttons |= bmap[btn->button];
> +        } else {
> +            in->buttons &= ~bmap[btn->button];
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_UP) {
> +            xenfb_mouse_event(in,
> +                              in->axis[INPUT_AXIS_X],
> +                              in->axis[INPUT_AXIS_Y],
> +                              -1,
> +                              in->buttons);
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_DOWN) {
> +            xenfb_mouse_event(in,
> +                              in->axis[INPUT_AXIS_X],
> +                              in->axis[INPUT_AXIS_Y],
> +                              1,
> +                              in->buttons);
> +        }

Why are we sending the WHEEL events from here rather than from
xenfb_legacy_mouse_sync?

Can't we add WHEEL_UP/DOWN to bmap? Unless it is due to a quirk
somewhere, I would store the wheel events in in->buttons or a new field,
then I would send the event to the other end from
xenfb_legacy_mouse_sync. You might have to reset the wheel event in
xenfb_legacy_mouse_sync, because, differently from the buttons, I don't
think we are going to get a corresponding "up" event.


> +        break;
> +    case INPUT_EVENT_KIND_ABS:
> +        move = evt->u.abs.data;
> +        in->axis[move->axis] = move->value;
> +        break;
> +    case INPUT_EVENT_KIND_REL:
> +        move = evt->u.rel.data;
> +        in->axis[move->axis] += move->value;
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void xenfb_legacy_mouse_sync(DeviceState *dev)
> +{
> +    struct XenInput *in = (struct XenInput *)dev;
> +
> +    xenfb_mouse_event(in,
> +                      in->axis[INPUT_AXIS_X],
> +                      in->axis[INPUT_AXIS_Y],
> +                      0,
> +                      in->buttons);
> +
> +    if (!in->abs_pointer_wanted) {
> +        in->axis[INPUT_AXIS_X] = 0;
> +        in->axis[INPUT_AXIS_Y] = 0;
> +    }

I think we should take the opportunity to rework and simplify
xenfb_mouse_event: we shouldn't keep track of the button state twice, in
in->buttons and in->button_state. I would get rid of the logic in
xenfb_mouse_event that attempts to figure out if a button was already
down before and just send the event.


> +}
> +
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> @@ -353,7 +440,6 @@ static int input_initialise(struct XenDevice *xendev)
>      if (rc != 0)
>  	return rc;
>  
> -    qemu_add_kbd_event_handler(xenfb_key_event, in);
>      return 0;
>  }
>  
> @@ -366,24 +452,43 @@ static void input_connected(struct XenDevice *xendev)
>          in->abs_pointer_wanted = 0;
>      }
>  
> +    if (in->qkbd) {
> +        qemu_input_handler_unregister(in->qkbd);
> +    }
>      if (in->qmouse) {
> -        qemu_remove_mouse_event_handler(in->qmouse);
> +        qemu_input_handler_unregister(in->qmouse);
>      }

Is there a reason for these checks? I realize we already had one here,
but shouldn't input_disconnect already take care of removing the
handlers?


>      trace_xenfb_input_connected(xendev, in->abs_pointer_wanted);
> -    in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
> -					      in->abs_pointer_wanted,
> -					      "Xen PVFB Mouse");
> +
> +    in->hkbd.name = "legacy-kbd";
> +    in->hkbd.mask = INPUT_EVENT_MASK_KEY;
> +    in->hkbd.event = xenfb_legacy_key_event;
> +    in->qkbd = qemu_input_handler_register((DeviceState *)in,
> +                                           &in->hkbd);
> +    qemu_input_handler_activate(in->qkbd);
> +
> +    in->hmouse.name = "Xen PVFB Mouse";
> +    in->hmouse.mask = INPUT_EVENT_MASK_BTN |
> +        (in->abs_pointer_wanted ? INPUT_EVENT_MASK_ABS : INPUT_EVENT_MASK_REL);
> +    in->hmouse.event = xenfb_legacy_mouse_event;
> +    in->hmouse.sync = xenfb_legacy_mouse_sync;
> +    in->qmouse = qemu_input_handler_register((DeviceState *)in,
> +                                             &in->hmouse);
> +    qemu_input_handler_activate(in->qmouse);
>  }
>  
>  static void input_disconnect(struct XenDevice *xendev)
>  {
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>  
> +    if (in->qkbd) {
> +        qemu_input_handler_unregister(in->qkbd);
> +        in->qkbd = NULL;
> +    }
>      if (in->qmouse) {
> -	qemu_remove_mouse_event_handler(in->qmouse);
> +        qemu_input_handler_unregister(in->qmouse);
>  	in->qmouse = NULL;

Please align this correctly since you are at it


>      }
> -    qemu_add_kbd_event_handler(NULL, NULL);
>      common_unbind(&in->c);
>  }
>  
> -- 
> 2.1.4
> 

Re: [PATCH 1/2 v2] xenfb: Use qemu_input_handler_* calls directly

[Stefano Stabellini]

On Mon, 3 Jul 2017, Owen Smith wrote:
> The xenvkbd input device uses functions from input-legacy.c
> Use the appropriate qemu_input_handler_* functions instead
> of calling functions in input-legacy.c that in turn call
> the correct functions.
> The bulk of this patch removes the extra layer of calls
> by moving the required structure members into the XenInput
> struct.
> 
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 113 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index e76c0d8..88815df 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -27,6 +27,7 @@
>  #include "qemu/osdep.h"
>  
>  #include "hw/hw.h"
> +#include "ui/input.h"
>  #include "ui/console.h"
>  #include "hw/xen/xen_backend.h"
>  
> @@ -54,7 +55,14 @@ struct XenInput {
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
> -    QEMUPutMouseEntry *qmouse;
> +    /* kbd */
> +    QemuInputHandler hkbd;
> +    QemuInputHandlerState *qkbd;
> +    /* mouse */
> +    QemuInputHandler hmouse;
> +    QemuInputHandlerState *qmouse;
> +    int axis[INPUT_AXIS__MAX];
> +    int buttons;
>  };
>  
>  #define UP_QUEUE 8
> @@ -293,6 +301,21 @@ static void xenfb_key_event(void *opaque, int scancode)
>      xenfb_send_key(xenfb, down, scancode2linux[scancode]);
>  }
>  
> +static void xenfb_legacy_key_event(DeviceState *dev, QemuConsole *src,
> +                                   InputEvent *evt)
> +{
> +    struct XenInput *in = (struct XenInput *)dev;
> +    int scancodes[3], i, count;
> +    InputKeyEvent *key = evt->u.key.data;
> +
> +    count = qemu_input_key_value_to_scancode(key->key,
> +                                             key->down,
> +                                             scancodes);
> +    for (i = 0; i < count; ++i) {
> +        xenfb_key_event(in, scancodes[i]);
> +    }
> +}
>  /*
>   * Send a mouse event from the client to the guest OS
>   *
> @@ -333,6 +356,70 @@ static void xenfb_mouse_event(void *opaque,
>      xenfb->button_state = button_state;
>  }
>  
> +static void xenfb_legacy_mouse_event(DeviceState *dev, QemuConsole *src,
> +                                     InputEvent *evt)
> +{
> +    static const int bmap[INPUT_BUTTON__MAX] = {
> +        [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
> +        [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
> +        [INPUT_BUTTON_RIGHT]  = MOUSE_EVENT_RBUTTON,
> +    };
> +    struct XenInput *in = (struct XenInput *)dev;
> +    InputBtnEvent *btn;
> +    InputMoveEvent *move;
> +
> +    switch (evt->type) {
> +    case INPUT_EVENT_KIND_BTN:
> +        btn = evt->u.btn.data;
> +        if (btn->down) {
> +            in->buttons |= bmap[btn->button];
> +        } else {
> +            in->buttons &= ~bmap[btn->button];
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_UP) {
> +            xenfb_mouse_event(in,
> +                              in->axis[INPUT_AXIS_X],
> +                              in->axis[INPUT_AXIS_Y],
> +                              -1,
> +                              in->buttons);
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_DOWN) {
> +            xenfb_mouse_event(in,
> +                              in->axis[INPUT_AXIS_X],
> +                              in->axis[INPUT_AXIS_Y],
> +                              1,
> +                              in->buttons);
> +        }

Why are we sending the WHEEL events from here rather than from
xenfb_legacy_mouse_sync?

Can't we add WHEEL_UP/DOWN to bmap? Unless it is due to a quirk
somewhere, I would store the wheel events in in->buttons or a new field,
then I would send the event to the other end from
xenfb_legacy_mouse_sync. You might have to reset the wheel event in
xenfb_legacy_mouse_sync, because, differently from the buttons, I don't
think we are going to get a corresponding "up" event.


> +        break;
> +    case INPUT_EVENT_KIND_ABS:
> +        move = evt->u.abs.data;
> +        in->axis[move->axis] = move->value;
> +        break;
> +    case INPUT_EVENT_KIND_REL:
> +        move = evt->u.rel.data;
> +        in->axis[move->axis] += move->value;
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void xenfb_legacy_mouse_sync(DeviceState *dev)
> +{
> +    struct XenInput *in = (struct XenInput *)dev;
> +
> +    xenfb_mouse_event(in,
> +                      in->axis[INPUT_AXIS_X],
> +                      in->axis[INPUT_AXIS_Y],
> +                      0,
> +                      in->buttons);
> +
> +    if (!in->abs_pointer_wanted) {
> +        in->axis[INPUT_AXIS_X] = 0;
> +        in->axis[INPUT_AXIS_Y] = 0;
> +    }

I think we should take the opportunity to rework and simplify
xenfb_mouse_event: we shouldn't keep track of the button state twice, in
in->buttons and in->button_state. I would get rid of the logic in
xenfb_mouse_event that attempts to figure out if a button was already
down before and just send the event.


> +}
> +
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> @@ -353,7 +440,6 @@ static int input_initialise(struct XenDevice *xendev)
>      if (rc != 0)
>  	return rc;
>  
> -    qemu_add_kbd_event_handler(xenfb_key_event, in);
>      return 0;
>  }
>  
> @@ -366,24 +452,43 @@ static void input_connected(struct XenDevice *xendev)
>          in->abs_pointer_wanted = 0;
>      }
>  
> +    if (in->qkbd) {
> +        qemu_input_handler_unregister(in->qkbd);
> +    }
>      if (in->qmouse) {
> -        qemu_remove_mouse_event_handler(in->qmouse);
> +        qemu_input_handler_unregister(in->qmouse);
>      }

Is there a reason for these checks? I realize we already had one here,
but shouldn't input_disconnect already take care of removing the
handlers?


>      trace_xenfb_input_connected(xendev, in->abs_pointer_wanted);
> -    in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
> -					      in->abs_pointer_wanted,
> -					      "Xen PVFB Mouse");
> +
> +    in->hkbd.name = "legacy-kbd";
> +    in->hkbd.mask = INPUT_EVENT_MASK_KEY;
> +    in->hkbd.event = xenfb_legacy_key_event;
> +    in->qkbd = qemu_input_handler_register((DeviceState *)in,
> +                                           &in->hkbd);
> +    qemu_input_handler_activate(in->qkbd);
> +
> +    in->hmouse.name = "Xen PVFB Mouse";
> +    in->hmouse.mask = INPUT_EVENT_MASK_BTN |
> +        (in->abs_pointer_wanted ? INPUT_EVENT_MASK_ABS : INPUT_EVENT_MASK_REL);
> +    in->hmouse.event = xenfb_legacy_mouse_event;
> +    in->hmouse.sync = xenfb_legacy_mouse_sync;
> +    in->qmouse = qemu_input_handler_register((DeviceState *)in,
> +                                             &in->hmouse);
> +    qemu_input_handler_activate(in->qmouse);
>  }
>  
>  static void input_disconnect(struct XenDevice *xendev)
>  {
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>  
> +    if (in->qkbd) {
> +        qemu_input_handler_unregister(in->qkbd);
> +        in->qkbd = NULL;
> +    }
>      if (in->qmouse) {
> -	qemu_remove_mouse_event_handler(in->qmouse);
> +        qemu_input_handler_unregister(in->qmouse);
>  	in->qmouse = NULL;

Please align this correctly since you are at it


>      }
> -    qemu_add_kbd_event_handler(NULL, NULL);
>      common_unbind(&in->c);
>  }
>  
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 2/2 v2] xenfb: Allow vkbd to connect without a DisplayState

[Stefano Stabellini]

On Mon, 3 Jul 2017, Owen Smith wrote:
> If the vkbd device model is registered and the vfb device model
> is not registered, the backend will not transition to connected.
> If there is no DisplayState, then the absolute coordinates cannot
> be scaled, and will remain in the range [0, 0x7fff].
> Backend writes "feature-raw-pointer" to indicate that the backend
> supports reporting absolute position without rescaling.
> The frontend uses "request-raw-pointer" to request raw unscaled
> pointer values. If there is no DisplayState, the absolute values
> are always raw unscaled values.
> 
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 88815df..d40af6e 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -53,6 +53,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int raw_pointer_wanted; /* Whether guest supports unscaled pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      /* kbd */
> @@ -329,18 +330,22 @@ static void xenfb_mouse_event(void *opaque,
>  			      int dx, int dy, int dz, int button_state)
>  {
>      struct XenInput *xenfb = opaque;
> -    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> -    int dw = surface_width(surface);
> -    int dh = surface_height(surface);
> -    int i;
> +    int i, x, y;
> +    if (xenfb->c.con && xenfb->raw_pointer_wanted != 1) {
> +        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> +        int dw = surface_width(surface);
> +        int dh = surface_height(surface);
> +        x = dx * (dw - 1) / 0x7fff;
> +        y = dy * (dh - 1) / 0x7fff;
> +    } else {
> +        x = dx;
> +        y = dy;
> +    }
>  
>      trace_xenfb_mouse_event(opaque, dx, dy, dz, button_state,
>                              xenfb->abs_pointer_wanted);
>      if (xenfb->abs_pointer_wanted)

Shouldn't this be:

  if (xenfb->abs_pointer_wanted || xenfb->raw_pointer_wanted)

?
It it possible to have raw_pointer_wanted && !abs_pointer_wanted? If
not, we should check at connection or initialization time.


> -	xenfb_send_position(xenfb,
> -			    dx * (dw - 1) / 0x7fff,
> -			    dy * (dh - 1) / 0x7fff,
> -			    dz);
> +        xenfb_send_position(xenfb, x, y, dz);
>      else
>  	xenfb_send_motion(xenfb, dx, dy, dz);
>  
> @@ -423,6 +428,7 @@ static void xenfb_legacy_mouse_sync(DeviceState *dev)
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> +    xenstore_write_be_int(xendev, "feature-raw-pointer", 1);
>      return 0;
>  }
>  
> @@ -432,8 +438,14 @@ static int input_initialise(struct XenDevice *xendev)
>      int rc;
>  
>      if (!in->c.con) {
> -        xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> -        return -1;
> +        char *vfb = xenstore_read_str(NULL, "device/vfb");

Isn't it better to do xenstore_read_str("device", "vfb") ?


> +        if (vfb == NULL) {
> +            /* there is no vfb, run vkbd on its own */
> +        } else {

if (vfb != NULL)


> +            free(vfb);

g_free


> +            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> +            return -1;
> +        }
>      }
>  
>      rc = common_bind(&in->c);
> @@ -451,6 +463,10 @@ static void input_connected(struct XenDevice *xendev)
>                               &in->abs_pointer_wanted) == -1) {
>          in->abs_pointer_wanted = 0;
>      }
> +    if (xenstore_read_fe_int(xendev, "request-raw-pointer",
> +                             &in->raw_pointer_wanted) == -1) {
> +        in->raw_pointer_wanted = 0;
> +    }
>  
>      if (in->qkbd) {
>          qemu_input_handler_unregister(in->qkbd);
> -- 
> 2.1.4
> 

Re: [PATCH 2/2 v2] xenfb: Allow vkbd to connect without a DisplayState

[Stefano Stabellini]

On Mon, 3 Jul 2017, Owen Smith wrote:
> If the vkbd device model is registered and the vfb device model
> is not registered, the backend will not transition to connected.
> If there is no DisplayState, then the absolute coordinates cannot
> be scaled, and will remain in the range [0, 0x7fff].
> Backend writes "feature-raw-pointer" to indicate that the backend
> supports reporting absolute position without rescaling.
> The frontend uses "request-raw-pointer" to request raw unscaled
> pointer values. If there is no DisplayState, the absolute values
> are always raw unscaled values.
> 
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 88815df..d40af6e 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -53,6 +53,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int raw_pointer_wanted; /* Whether guest supports unscaled pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      /* kbd */
> @@ -329,18 +330,22 @@ static void xenfb_mouse_event(void *opaque,
>  			      int dx, int dy, int dz, int button_state)
>  {
>      struct XenInput *xenfb = opaque;
> -    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> -    int dw = surface_width(surface);
> -    int dh = surface_height(surface);
> -    int i;
> +    int i, x, y;
> +    if (xenfb->c.con && xenfb->raw_pointer_wanted != 1) {
> +        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> +        int dw = surface_width(surface);
> +        int dh = surface_height(surface);
> +        x = dx * (dw - 1) / 0x7fff;
> +        y = dy * (dh - 1) / 0x7fff;
> +    } else {
> +        x = dx;
> +        y = dy;
> +    }
>  
>      trace_xenfb_mouse_event(opaque, dx, dy, dz, button_state,
>                              xenfb->abs_pointer_wanted);
>      if (xenfb->abs_pointer_wanted)

Shouldn't this be:

  if (xenfb->abs_pointer_wanted || xenfb->raw_pointer_wanted)

?
It it possible to have raw_pointer_wanted && !abs_pointer_wanted? If
not, we should check at connection or initialization time.


> -	xenfb_send_position(xenfb,
> -			    dx * (dw - 1) / 0x7fff,
> -			    dy * (dh - 1) / 0x7fff,
> -			    dz);
> +        xenfb_send_position(xenfb, x, y, dz);
>      else
>  	xenfb_send_motion(xenfb, dx, dy, dz);
>  
> @@ -423,6 +428,7 @@ static void xenfb_legacy_mouse_sync(DeviceState *dev)
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> +    xenstore_write_be_int(xendev, "feature-raw-pointer", 1);
>      return 0;
>  }
>  
> @@ -432,8 +438,14 @@ static int input_initialise(struct XenDevice *xendev)
>      int rc;
>  
>      if (!in->c.con) {
> -        xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> -        return -1;
> +        char *vfb = xenstore_read_str(NULL, "device/vfb");

Isn't it better to do xenstore_read_str("device", "vfb") ?


> +        if (vfb == NULL) {
> +            /* there is no vfb, run vkbd on its own */
> +        } else {

if (vfb != NULL)


> +            free(vfb);

g_free


> +            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> +            return -1;
> +        }
>      }
>  
>      rc = common_bind(&in->c);
> @@ -451,6 +463,10 @@ static void input_connected(struct XenDevice *xendev)
>                               &in->abs_pointer_wanted) == -1) {
>          in->abs_pointer_wanted = 0;
>      }
> +    if (xenstore_read_fe_int(xendev, "request-raw-pointer",
> +                             &in->raw_pointer_wanted) == -1) {
> +        in->raw_pointer_wanted = 0;
> +    }
>  
>      if (in->qkbd) {
>          qemu_input_handler_unregister(in->qkbd);
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel