[PATCH 0/4] xenfb: Add vkbd-only option

[PATCH 0/4] xenfb: Add vkbd-only option

[Owen Smith]

Adds the ability for a vkbd device to connect without the
QemuConsole, in order to support a standalone PV mouse and
keyboard frontend.
This series adds a new feature flag, which will need adding
to the xen's include/public/io/kbdif.h
"feature-vkbd-standalone" is set to 1 by backends that allow 
the vkbd device model to connect without requiring a vfb device
connected. The vkbd device will only bypass the check for
the vfb device if the frontend sets "request-vkbd-standalone"
to 1.
The last 2 patches add a couple of missing input handler
functions, and uses these to remove a leak in the vkbd device
model.

Owen Smith (4):
  xenfb: Add feature-vkbd-standalone
  xenfb: Activate mouse handler
  ui/input: Add activate/remove for keyboard handlers
  xenfb: Fix leak by adding/removing keyboard handler

 hw/display/xenfb.c   | 44 ++++++++++++++++++++++++++++++++------------
 include/ui/console.h |  2 ++
 ui/input-legacy.c    | 12 ++++++++++++
 3 files changed, 46 insertions(+), 12 deletions(-)

-- 
2.1.4


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

[PATCH 1/4] xenfb: Add feature-vkbd-standalone

[Owen Smith]

Advertise "feature-vkbd-standalone" to indicate the backend
can connect without a vfb device connection.
When "request-vkbd-standalone" is set to 1, the backend does
not wait for a QemuConsole to be setup before connecting the 
vkbd device. This also means that absolute coordinates cannot
be scaled to the non-existent QemuConsole's sizes, and remain
unscaled, in the range [0, 0x7FFF].

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index e76c0d8..2ebc81b 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -52,6 +52,7 @@ struct common {
 struct XenInput {
     struct common c;
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
+    int vkbd_standalone;    /* Guest supports vkbd without vfb device */
     int button_state;       /* Last seen pointer button state */
     int extended;
     QEMUPutMouseEntry *qmouse;
@@ -306,18 +307,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 != NULL) {
+        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
+        int dw = surface_width(surface);
+        int dh = surface_height(surface);
+        x = dx * (dh - 1) / 0x7fff;
+        y = dy * (dw - 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);
 
@@ -336,6 +341,7 @@ static void xenfb_mouse_event(void *opaque,
 static int input_init(struct XenDevice *xendev)
 {
     xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
+    xenstore_write_be_int(xendev, "feature-vkbd-standalone", 1);
     return 0;
 }
 
@@ -345,8 +351,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;
+        if (xenstore_read_fe_int(xendev, "request-vkbd-standalone",
+                                 &in->vkbd_standalone) == -1) {
+            in->vkbd_standalone = 0;
+        }
+        if (in->vkbd_standalone == 0) {
+            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
+            return -1;
+        }
     }
 
     rc = common_bind(&in->c);
-- 
2.1.4


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

[PATCH 2/4] xenfb: Activate mouse handler

[Owen Smith]

Mouse events are only delivered to the first handler in the chain.
Activating the xenfb mouse event handler so that mouse events can
be passed over the shared ring protocol.
Note: The keyboard handler is activated internally by the add
call.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 2ebc81b..b0a5726 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -385,6 +385,7 @@ static void input_connected(struct XenDevice *xendev)
     in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
 					      in->abs_pointer_wanted,
 					      "Xen PVFB Mouse");
+    qemu_activate_mouse_event_handler(in->qmouse);
 }
 
 static void input_disconnect(struct XenDevice *xendev)
-- 
2.1.4


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

[PATCH 3/4] ui/input: Add activate/remove for keyboard handlers

[Owen Smith]

Adds missing functions to activate and remove keyboard handlers
* qemu_activate_kbd_event_handler
* qemu_remove_kbd_event_handler

Signed-off-by: Owen Smith <owen.smith@citrix.com>
---
 include/ui/console.h |  2 ++
 ui/input-legacy.c    | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 7262bef..f5045e1 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -55,6 +55,8 @@ typedef struct QEMUPutLEDEntry QEMUPutLEDEntry;
 
 QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func,
                                             void *opaque);
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry);
+void qemu_activate_kbd_event_handler(QEMUPutKbdEntry *entry);
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name);
diff --git a/ui/input-legacy.c b/ui/input-legacy.c
index 7159747..fbe1ce7 100644
--- a/ui/input-legacy.c
+++ b/ui/input-legacy.c
@@ -142,6 +142,18 @@ QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
     return entry;
 }
 
+void qemu_activate_kbd_event_handler(QEMUPutKbdEntry *entry)
+{
+    qemu_input_handler_activate(entry->s);
+}
+
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry)
+{
+    qemu_input_handler_unregister(entry->s);
+
+    g_free(entry);
+}
+
 static void legacy_mouse_event(DeviceState *dev, QemuConsole *src,
                                InputEvent *evt)
 {
-- 
2.1.4


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

[PATCH 4/4] xenfb: Fix leak by adding/removing keyboard handler

[Owen Smith]

Calls qemu_remove_kbd_event_handler that frees its memory, instead of
adding (and allocating some memory) a new NULL handler.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index b0a5726..570ecdb 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -55,6 +55,7 @@ struct XenInput {
     int vkbd_standalone;    /* Guest supports vkbd without vfb device */
     int button_state;       /* Last seen pointer button state */
     int extended;
+    QEMUPutKbdEntry *qkbd;
     QEMUPutMouseEntry *qmouse;
 };
 
@@ -365,7 +366,6 @@ static int input_initialise(struct XenDevice *xendev)
     if (rc != 0)
 	return rc;
 
-    qemu_add_kbd_event_handler(xenfb_key_event, in);
     return 0;
 }
 
@@ -378,10 +378,14 @@ static void input_connected(struct XenDevice *xendev)
         in->abs_pointer_wanted = 0;
     }
 
+    if (in->qkbd) {
+        qemu_remove_kbd_event_handler(in->qkbd);
+    }
     if (in->qmouse) {
         qemu_remove_mouse_event_handler(in->qmouse);
     }
     trace_xenfb_input_connected(xendev, in->abs_pointer_wanted);
+    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
     in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
 					      in->abs_pointer_wanted,
 					      "Xen PVFB Mouse");
@@ -392,11 +396,14 @@ static void input_disconnect(struct XenDevice *xendev)
 {
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
 
+    if (in->qkbd) {
+        qemu_remove_kbd_event_handler(in->qkbd);
+        in->qkbd = NULL;
+    }
     if (in->qmouse) {
 	qemu_remove_mouse_event_handler(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

Re: [PATCH 3/4] ui/input: Add activate/remove for keyboard handlers

[Gerd Hoffmann]

diff --git a/ui/input-legacy.c b/ui/input-legacy.c
> index 7159747..fbe1ce7 100644
> --- a/ui/input-legacy.c
> +++ b/ui/input-legacy.c
> @@ -142,6 +142,18 @@ QEMUPutKbdEntry
> *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
>      return entry;
>  }
>  
> +void qemu_activate_kbd_event_handler(QEMUPutKbdEntry *entry)

Please don't add new code to input-legacy.c please.

Switch your code to use the new qemu_input_handler_*() functions
directly instead.

cheers,
  Gerd

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

Re: [PATCH 0/4] xenfb: Add vkbd-only option

[Stefano Stabellini]

On Thu, 8 Jun 2017, Owen Smith wrote:
> Adds the ability for a vkbd device to connect without the
> QemuConsole, in order to support a standalone PV mouse and
> keyboard frontend.
> This series adds a new feature flag, which will need adding
> to the xen's include/public/io/kbdif.h

Please do so, I would like that change to be applied to xen before this
series is applied to QEMU.


> "feature-vkbd-standalone" is set to 1 by backends that allow 
> the vkbd device model to connect without requiring a vfb device
> connected. The vkbd device will only bypass the check for
> the vfb device if the frontend sets "request-vkbd-standalone"
> to 1.
> The last 2 patches add a couple of missing input handler
> functions, and uses these to remove a leak in the vkbd device
> model.
> 
> Owen Smith (4):
>   xenfb: Add feature-vkbd-standalone
>   xenfb: Activate mouse handler
>   ui/input: Add activate/remove for keyboard handlers
>   xenfb: Fix leak by adding/removing keyboard handler
> 
>  hw/display/xenfb.c   | 44 ++++++++++++++++++++++++++++++++------------
>  include/ui/console.h |  2 ++
>  ui/input-legacy.c    | 12 ++++++++++++
>  3 files changed, 46 insertions(+), 12 deletions(-)
> 
> -- 
> 2.1.4
> 

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

Re: [PATCH 1/4] xenfb: Add feature-vkbd-standalone

[Stefano Stabellini]

On Thu, 8 Jun 2017, Owen Smith wrote:
> Advertise "feature-vkbd-standalone" to indicate the backend
> can connect without a vfb device connection.
> When "request-vkbd-standalone" is set to 1, the backend does
> not wait for a QemuConsole to be setup before connecting the 
> vkbd device. This also means that absolute coordinates cannot
> be scaled to the non-existent QemuConsole's sizes, and remain
> unscaled, in the range [0, 0x7FFF].
> 
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index e76c0d8..2ebc81b 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -52,6 +52,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int vkbd_standalone;    /* Guest supports vkbd without vfb device */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      QEMUPutMouseEntry *qmouse;
> @@ -306,18 +307,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 != NULL) {
> +        DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> +        int dw = surface_width(surface);
> +        int dh = surface_height(surface);
> +        x = dx * (dh - 1) / 0x7fff;
> +        y = dy * (dw - 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);
>  
> @@ -336,6 +341,7 @@ static void xenfb_mouse_event(void *opaque,
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> +    xenstore_write_be_int(xendev, "feature-vkbd-standalone", 1);
>      return 0;
>  }
>  
> @@ -345,8 +351,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;
> +        if (xenstore_read_fe_int(xendev, "request-vkbd-standalone",
> +                                 &in->vkbd_standalone) == -1) {
> +            in->vkbd_standalone = 0;
> +        }
> +        if (in->vkbd_standalone == 0) {
> +            xen_pv_printf(xendev, 1, "ds not set (yet)\n");
> +            return -1;
> +        }

In your changes to include/public/io/kbdif.h, make sure to write when
(at what xenstore status stage) the frontend needs to write
request-vkbd-standalone. 

This patch looks good:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>



>      }
>  
>      rc = common_bind(&in->c);

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

Re: [PATCH 2/4] xenfb: Activate mouse handler

[Stefano Stabellini]

On Thu, 8 Jun 2017, Owen Smith wrote:
> Mouse events are only delivered to the first handler in the chain.
> Activating the xenfb mouse event handler so that mouse events can
> be passed over the shared ring protocol.
> Note: The keyboard handler is activated internally by the add
> call.

I am not sure I follow: why do we need this now? How is it working
today?


> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 2ebc81b..b0a5726 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -385,6 +385,7 @@ static void input_connected(struct XenDevice *xendev)
>      in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
>  					      in->abs_pointer_wanted,
>  					      "Xen PVFB Mouse");
> +    qemu_activate_mouse_event_handler(in->qmouse);
>  }
>  
>  static void input_disconnect(struct XenDevice *xendev)
> -- 
> 2.1.4
> 

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

Re: [PATCH 3/4] ui/input: Add activate/remove for keyboard handlers

[Owen Smith]

[-- Attachment #1.1: Type: text/plain, Size: 1123 bytes --]

Noted, I have had a look at porting the xenfb input handlers to the

qemu_input_handler_*() functions, and will post an update that combines patch

3 and 4 of this series.



From: Gerd Hoffmann<mailto:kraxel@redhat.com>
Sent: 08 June 2017 14:39
To: Owen Smith<mailto:owen.smith@citrix.com>; qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org>
Cc: sstabellini@kernel.org<mailto:sstabellini@kernel.org>; Anthony Perard<mailto:anthony.perard@citrix.com>; xen-devel@lists.xenproject.org<mailto:xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 3/4] ui/input: Add activate/remove for keyboard handlers



diff --git a/ui/input-legacy.c b/ui/input-legacy.c
> index 7159747..fbe1ce7 100644
> --- a/ui/input-legacy.c
> +++ b/ui/input-legacy.c
> @@ -142,6 +142,18 @@ QEMUPutKbdEntry
> *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
>      return entry;
>  }
>
> +void qemu_activate_kbd_event_handler(QEMUPutKbdEntry *entry)

Please don't add new code to input-legacy.c please.

Switch your code to use the new qemu_input_handler_*() functions
directly instead.

cheers,
  Gerd

[-- Attachment #1.2: Type: text/html, Size: 2656 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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