[Qemu-devel] [PATCH 0/3] xenfb: Add support for Windows PV frontend

[Qemu-devel] [PATCH 0/3] xenfb: Add support for Windows PV frontend

[Owen smith]

This patch series contains improvments for the Xen vkbd backend
to support a Windows PV frontend mouse and keyboard. This allows 
VNC connections to have an absolute pointer without the USB tablet
device enabled, and any unneccessary polling associated with the USB
devices.

This patch series contains the following patches

1-2) Fixes to the vkbd device model
3) Adds a feature to not scale axes to DisplaySurface size

Owen smith (3):
  xenfb: Unregister keyboard event handler correctly
  xenfb: Add option to use grant ref instead of mfn for shared page.
  xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

 hw/display/xenfb.c | 104 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 27 deletions(-)

-- 
2.1.0

[PATCH 0/3] xenfb: Add support for Windows PV frontend

[Owen smith]

This patch series contains improvments for the Xen vkbd backend
to support a Windows PV frontend mouse and keyboard. This allows 
VNC connections to have an absolute pointer without the USB tablet
device enabled, and any unneccessary polling associated with the USB
devices.

This patch series contains the following patches

1-2) Fixes to the vkbd device model
3) Adds a feature to not scale axes to DisplaySurface size

Owen smith (3):
  xenfb: Unregister keyboard event handler correctly
  xenfb: Add option to use grant ref instead of mfn for shared page.
  xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

 hw/display/xenfb.c | 104 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 27 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] xenfb: Unregister keyboard event handler correctly

[Owen smith]

Keyboard event handler was replaced with a new handler on disconnect.
Use the unregister function to remove keyboard handler.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 07ddc9d..2c39753 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -62,6 +62,7 @@ struct XenInput {
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
     int button_state;       /* Last seen pointer button state */
     int extended;
+    QEMUPutKbdEntry *qkbd;
     QEMUPutMouseEntry *qmouse;
 };
 
@@ -364,7 +365,6 @@ static int input_initialise(struct XenDevice *xendev)
     if (rc != 0)
 	return rc;
 
-    qemu_add_kbd_event_handler(xenfb_key_event, in);
     return 0;
 }
 
@@ -383,17 +383,26 @@ 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);
+
+    if (in->qkbd) {
+        qemu_remove_kbd_event_handler(in->qkbd);
+    }
+    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
 }
 
 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.0

[PATCH 1/3] xenfb: Unregister keyboard event handler correctly

[Owen smith]

Keyboard event handler was replaced with a new handler on disconnect.
Use the unregister function to remove keyboard handler.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 07ddc9d..2c39753 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -62,6 +62,7 @@ struct XenInput {
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
     int button_state;       /* Last seen pointer button state */
     int extended;
+    QEMUPutKbdEntry *qkbd;
     QEMUPutMouseEntry *qmouse;
 };
 
@@ -364,7 +365,6 @@ static int input_initialise(struct XenDevice *xendev)
     if (rc != 0)
 	return rc;
 
-    qemu_add_kbd_event_handler(xenfb_key_event, in);
     return 0;
 }
 
@@ -383,17 +383,26 @@ 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);
+
+    if (in->qkbd) {
+        qemu_remove_kbd_event_handler(in->qkbd);
+    }
+    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
 }
 
 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.0

[Qemu-devel] [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Owen smith]

The vkbd device should be able to use a grant reference to map the
shared page.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 2c39753..c4375c8 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -54,6 +54,7 @@
 struct common {
     struct XenDevice  xendev;  /* must be first */
     void              *page;
+    int               page_gref;
     QemuConsole       *con;
 };
 
@@ -96,22 +97,36 @@ static int common_bind(struct common *c)
 {
     uint64_t mfn;
 
-    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
-	return -1;
-    assert(mfn == (xen_pfn_t)mfn);
-
     if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
 	return -1;
 
-    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
-				   XC_PAGE_SIZE,
-				   PROT_READ | PROT_WRITE, mfn);
+    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
+        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref) == -1) {
+            return -1;
+        }
+        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
+                                          c->xendev.dom,
+                                          c->page_gref,
+                                          PROT_READ | PROT_WRITE);
+    } else {
+        assert(mfn == (xen_pfn_t)mfn);
+
+        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
+                                       XC_PAGE_SIZE,
+                                       PROT_READ | PROT_WRITE, mfn);
+    }
     if (c->page == NULL)
 	return -1;
 
     xen_be_bind_evtchn(&c->xendev);
-    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
-		  mfn, c->xendev.remote_port, c->xendev.local_port);
+
+    if (c->page_gref) {
+        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-port %d\n",
+                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
+    } else {
+        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
+                      mfn, c->xendev.remote_port, c->xendev.local_port);
+    }
 
     return 0;
 }
@@ -120,8 +135,13 @@ static void common_unbind(struct common *c)
 {
     xen_be_unbind_evtchn(&c->xendev);
     if (c->page) {
-	munmap(c->page, XC_PAGE_SIZE);
+        if (c->page_gref) {
+            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
+        } else {
+            munmap(c->page, XC_PAGE_SIZE);
+        }
 	c->page = NULL;
+        c->page_gref = 0;
     }
 }
 
@@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
 
 struct XenDevOps xen_kbdmouse_ops = {
     .size       = sizeof(struct XenInput),
+    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
     .init       = input_init,
     .initialise = input_initialise,
     .connected  = input_connected,
-- 
2.1.0

[PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Owen smith]

The vkbd device should be able to use a grant reference to map the
shared page.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 2c39753..c4375c8 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -54,6 +54,7 @@
 struct common {
     struct XenDevice  xendev;  /* must be first */
     void              *page;
+    int               page_gref;
     QemuConsole       *con;
 };
 
@@ -96,22 +97,36 @@ static int common_bind(struct common *c)
 {
     uint64_t mfn;
 
-    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
-	return -1;
-    assert(mfn == (xen_pfn_t)mfn);
-
     if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
 	return -1;
 
-    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
-				   XC_PAGE_SIZE,
-				   PROT_READ | PROT_WRITE, mfn);
+    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
+        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref) == -1) {
+            return -1;
+        }
+        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
+                                          c->xendev.dom,
+                                          c->page_gref,
+                                          PROT_READ | PROT_WRITE);
+    } else {
+        assert(mfn == (xen_pfn_t)mfn);
+
+        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
+                                       XC_PAGE_SIZE,
+                                       PROT_READ | PROT_WRITE, mfn);
+    }
     if (c->page == NULL)
 	return -1;
 
     xen_be_bind_evtchn(&c->xendev);
-    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
-		  mfn, c->xendev.remote_port, c->xendev.local_port);
+
+    if (c->page_gref) {
+        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-port %d\n",
+                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
+    } else {
+        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
+                      mfn, c->xendev.remote_port, c->xendev.local_port);
+    }
 
     return 0;
 }
@@ -120,8 +135,13 @@ static void common_unbind(struct common *c)
 {
     xen_be_unbind_evtchn(&c->xendev);
     if (c->page) {
-	munmap(c->page, XC_PAGE_SIZE);
+        if (c->page_gref) {
+            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
+        } else {
+            munmap(c->page, XC_PAGE_SIZE);
+        }
 	c->page = NULL;
+        c->page_gref = 0;
     }
 }
 
@@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
 
 struct XenDevOps xen_kbdmouse_ops = {
     .size       = sizeof(struct XenInput),
+    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
     .init       = input_init,
     .initialise = input_initialise,
     .connected  = input_connected,
-- 
2.1.0

[Qemu-devel] [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Owen smith]

Windows PV frontend requires absolute mouse values in the range
[0, 0x7fff]. Add a feature to not rescale the axes to DisplaySurface
size. Also allows Windows PV frontend to connect without a vfb
device. Moves the rescaling of axes to a seperate function with
additional null-pointer checks.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index c4375c8..65c373b 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -61,6 +61,7 @@ struct common {
 struct XenInput {
     struct common c;
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
+    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
     int button_state;       /* Last seen pointer button state */
     int extended;
     QEMUPutKbdEntry *qkbd;
@@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int scancode)
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
 }
 
+static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int *dy)
+{
+    DisplaySurface *surface;
+    int dw, dh;
+
+    if (xenfb->c.con == NULL) {
+        return;
+    }
+    surface = qemu_console_surface(xenfb->c.con);
+    if (surface == NULL) {
+        return;
+    }
+    dw = surface_width(surface);
+    dh = surface_height(surface);
+
+    *dx = *dx * (dw - 1) / 0x7fff;
+    *dy = *dy * (dh - 1) / 0x7fff;
+}
+
 /*
  * Send a mouse event from the client to the guest OS
  *
@@ -338,18 +358,16 @@ 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;
 
-    if (xenfb->abs_pointer_wanted)
-	xenfb_send_position(xenfb,
-			    dx * (dw - 1) / 0x7fff,
-			    dy * (dh - 1) / 0x7fff,
-			    dz);
-    else
+    if (xenfb->abs_pointer_wanted) {
+        if (!xenfb->no_abs_rescale) {
+            xenfb_rescale_mouse(xenfb, &dx, &dy);
+        }
+        xenfb_send_position(xenfb, dx, dy, dz);
+    } else {
 	xenfb_send_motion(xenfb, dx, dy, dz);
+    }
 
     for (i = 0 ; i < 8 ; i++) {
 	int lastDown = xenfb->button_state & (1 << i);
@@ -366,6 +382,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-no-abs-rescale", 1);
     return 0;
 }
 
@@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice *xendev)
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
     int rc;
 
-    if (!in->c.con) {
+    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
+                             &in->abs_pointer_wanted) == -1) {
+        in->abs_pointer_wanted = 0;
+    }
+
+    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
+                             &in->no_abs_rescale) == -1) {
+        in->no_abs_rescale = 0;
+    }
+
+    if (!in->c.con && !in->no_abs_rescale) {
         xen_be_printf(xendev, 1, "ds not set (yet)\n");
         return -1;
     }
@@ -390,11 +417,6 @@ static void input_connected(struct XenDevice *xendev)
 {
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
 
-    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
-                             &in->abs_pointer_wanted) == -1) {
-        in->abs_pointer_wanted = 0;
-    }
-
     if (in->qmouse) {
         qemu_remove_mouse_event_handler(in->qmouse);
     }
-- 
2.1.0

[PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Owen smith]

Windows PV frontend requires absolute mouse values in the range
[0, 0x7fff]. Add a feature to not rescale the axes to DisplaySurface
size. Also allows Windows PV frontend to connect without a vfb
device. Moves the rescaling of axes to a seperate function with
additional null-pointer checks.

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

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index c4375c8..65c373b 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -61,6 +61,7 @@ struct common {
 struct XenInput {
     struct common c;
     int abs_pointer_wanted; /* Whether guest supports absolute pointer */
+    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
     int button_state;       /* Last seen pointer button state */
     int extended;
     QEMUPutKbdEntry *qkbd;
@@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int scancode)
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
 }
 
+static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int *dy)
+{
+    DisplaySurface *surface;
+    int dw, dh;
+
+    if (xenfb->c.con == NULL) {
+        return;
+    }
+    surface = qemu_console_surface(xenfb->c.con);
+    if (surface == NULL) {
+        return;
+    }
+    dw = surface_width(surface);
+    dh = surface_height(surface);
+
+    *dx = *dx * (dw - 1) / 0x7fff;
+    *dy = *dy * (dh - 1) / 0x7fff;
+}
+
 /*
  * Send a mouse event from the client to the guest OS
  *
@@ -338,18 +358,16 @@ 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;
 
-    if (xenfb->abs_pointer_wanted)
-	xenfb_send_position(xenfb,
-			    dx * (dw - 1) / 0x7fff,
-			    dy * (dh - 1) / 0x7fff,
-			    dz);
-    else
+    if (xenfb->abs_pointer_wanted) {
+        if (!xenfb->no_abs_rescale) {
+            xenfb_rescale_mouse(xenfb, &dx, &dy);
+        }
+        xenfb_send_position(xenfb, dx, dy, dz);
+    } else {
 	xenfb_send_motion(xenfb, dx, dy, dz);
+    }
 
     for (i = 0 ; i < 8 ; i++) {
 	int lastDown = xenfb->button_state & (1 << i);
@@ -366,6 +382,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-no-abs-rescale", 1);
     return 0;
 }
 
@@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice *xendev)
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
     int rc;
 
-    if (!in->c.con) {
+    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
+                             &in->abs_pointer_wanted) == -1) {
+        in->abs_pointer_wanted = 0;
+    }
+
+    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
+                             &in->no_abs_rescale) == -1) {
+        in->no_abs_rescale = 0;
+    }
+
+    if (!in->c.con && !in->no_abs_rescale) {
         xen_be_printf(xendev, 1, "ds not set (yet)\n");
         return -1;
     }
@@ -390,11 +417,6 @@ static void input_connected(struct XenDevice *xendev)
 {
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
 
-    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
-                             &in->abs_pointer_wanted) == -1) {
-        in->abs_pointer_wanted = 0;
-    }
-
     if (in->qmouse) {
         qemu_remove_mouse_event_handler(in->qmouse);
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/3] xenfb: Unregister keyboard event handler correctly

[Stefano Stabellini]

On Wed, 10 Sep 2014, Owen smith wrote:
> Keyboard event handler was replaced with a new handler on disconnect.
> Use the unregister function to remove keyboard handler.
> 
> Signed-off-by: Owen smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 07ddc9d..2c39753 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -62,6 +62,7 @@ struct XenInput {
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
> +    QEMUPutKbdEntry *qkbd;
>      QEMUPutMouseEntry *qmouse;
>  };
>  
> @@ -364,7 +365,6 @@ static int input_initialise(struct XenDevice *xendev)
>      if (rc != 0)
>  	return rc;
>  
> -    qemu_add_kbd_event_handler(xenfb_key_event, in);
>      return 0;
>  }
>  
> @@ -383,17 +383,26 @@ 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);

This change is not described in the commit message.
Why is this necessary? Is it related to the keyboard change?


> +    if (in->qkbd) {
> +        qemu_remove_kbd_event_handler(in->qkbd);
> +    }
> +    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
>  }
>  
>  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.0
> 

Re: [PATCH 1/3] xenfb: Unregister keyboard event handler correctly

[Stefano Stabellini]

On Wed, 10 Sep 2014, Owen smith wrote:
> Keyboard event handler was replaced with a new handler on disconnect.
> Use the unregister function to remove keyboard handler.
> 
> Signed-off-by: Owen smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 07ddc9d..2c39753 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -62,6 +62,7 @@ struct XenInput {
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
> +    QEMUPutKbdEntry *qkbd;
>      QEMUPutMouseEntry *qmouse;
>  };
>  
> @@ -364,7 +365,6 @@ static int input_initialise(struct XenDevice *xendev)
>      if (rc != 0)
>  	return rc;
>  
> -    qemu_add_kbd_event_handler(xenfb_key_event, in);
>      return 0;
>  }
>  
> @@ -383,17 +383,26 @@ 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);

This change is not described in the commit message.
Why is this necessary? Is it related to the keyboard change?


> +    if (in->qkbd) {
> +        qemu_remove_kbd_event_handler(in->qkbd);
> +    }
> +    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
>  }
>  
>  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.0
> 

Re: [Qemu-devel] [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Stefano Stabellini]

On Wed, 10 Sep 2014, Owen smith wrote:
> The vkbd device should be able to use a grant reference to map the
> shared page.
> 
> Signed-off-by: Owen smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 2c39753..c4375c8 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -54,6 +54,7 @@
>  struct common {
>      struct XenDevice  xendev;  /* must be first */
>      void              *page;
> +    int               page_gref;
>      QemuConsole       *con;
>  };
>  
> @@ -96,22 +97,36 @@ static int common_bind(struct common *c)
>  {
>      uint64_t mfn;
>  
> -    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
> -	return -1;
> -    assert(mfn == (xen_pfn_t)mfn);
> -
>      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
>  	return -1;
>  
> -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> -				   XC_PAGE_SIZE,
> -				   PROT_READ | PROT_WRITE, mfn);
> +    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
> +        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref) == -1) {
> +            return -1;
> +        }
> +        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
> +                                          c->xendev.dom,
> +                                          c->page_gref,
> +                                          PROT_READ | PROT_WRITE);
> +    } else {
> +        assert(mfn == (xen_pfn_t)mfn);
> +
> +        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> +                                       XC_PAGE_SIZE,
> +                                       PROT_READ | PROT_WRITE, mfn);
> +    }

This is an extension of the protocol. It should be documented somewhere.
At least in the commit message and somewhere under docs/misc or
xen/include/public/io/fbif.h.


>      if (c->page == NULL)
>  	return -1;
>  
>      xen_be_bind_evtchn(&c->xendev);
> -    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
> -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> +
> +    if (c->page_gref) {
> +        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-port %d\n",
> +                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
> +    } else {
> +        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
> +                      mfn, c->xendev.remote_port, c->xendev.local_port);
> +    }
>  
>      return 0;
>  }
> @@ -120,8 +135,13 @@ static void common_unbind(struct common *c)
>  {
>      xen_be_unbind_evtchn(&c->xendev);
>      if (c->page) {
> -	munmap(c->page, XC_PAGE_SIZE);
> +        if (c->page_gref) {
> +            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
> +        } else {
> +            munmap(c->page, XC_PAGE_SIZE);
> +        }
>  	c->page = NULL;
> +        c->page_gref = 0;
>      }
>  }
>  
> @@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
>  
>  struct XenDevOps xen_kbdmouse_ops = {
>      .size       = sizeof(struct XenInput),
> +    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
>      .init       = input_init,
>      .initialise = input_initialise,
>      .connected  = input_connected,
> -- 
> 2.1.0
> 

Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Stefano Stabellini]

On Wed, 10 Sep 2014, Owen smith wrote:
> The vkbd device should be able to use a grant reference to map the
> shared page.
> 
> Signed-off-by: Owen smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 2c39753..c4375c8 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -54,6 +54,7 @@
>  struct common {
>      struct XenDevice  xendev;  /* must be first */
>      void              *page;
> +    int               page_gref;
>      QemuConsole       *con;
>  };
>  
> @@ -96,22 +97,36 @@ static int common_bind(struct common *c)
>  {
>      uint64_t mfn;
>  
> -    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
> -	return -1;
> -    assert(mfn == (xen_pfn_t)mfn);
> -
>      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
>  	return -1;
>  
> -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> -				   XC_PAGE_SIZE,
> -				   PROT_READ | PROT_WRITE, mfn);
> +    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
> +        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref) == -1) {
> +            return -1;
> +        }
> +        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
> +                                          c->xendev.dom,
> +                                          c->page_gref,
> +                                          PROT_READ | PROT_WRITE);
> +    } else {
> +        assert(mfn == (xen_pfn_t)mfn);
> +
> +        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> +                                       XC_PAGE_SIZE,
> +                                       PROT_READ | PROT_WRITE, mfn);
> +    }

This is an extension of the protocol. It should be documented somewhere.
At least in the commit message and somewhere under docs/misc or
xen/include/public/io/fbif.h.


>      if (c->page == NULL)
>  	return -1;
>  
>      xen_be_bind_evtchn(&c->xendev);
> -    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
> -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> +
> +    if (c->page_gref) {
> +        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-port %d\n",
> +                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
> +    } else {
> +        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
> +                      mfn, c->xendev.remote_port, c->xendev.local_port);
> +    }
>  
>      return 0;
>  }
> @@ -120,8 +135,13 @@ static void common_unbind(struct common *c)
>  {
>      xen_be_unbind_evtchn(&c->xendev);
>      if (c->page) {
> -	munmap(c->page, XC_PAGE_SIZE);
> +        if (c->page_gref) {
> +            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
> +        } else {
> +            munmap(c->page, XC_PAGE_SIZE);
> +        }
>  	c->page = NULL;
> +        c->page_gref = 0;
>      }
>  }
>  
> @@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
>  
>  struct XenDevOps xen_kbdmouse_ops = {
>      .size       = sizeof(struct XenInput),
> +    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
>      .init       = input_init,
>      .initialise = input_initialise,
>      .connected  = input_connected,
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Stefano Stabellini]

On Wed, 10 Sep 2014, Owen smith wrote:
> Windows PV frontend requires absolute mouse values in the range
> [0, 0x7fff]. Add a feature to not rescale the axes to DisplaySurface
> size. Also allows Windows PV frontend to connect without a vfb
> device. Moves the rescaling of axes to a seperate function with
> additional null-pointer checks.
> 
> Signed-off-by: Owen smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 52 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index c4375c8..65c373b 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -61,6 +61,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      QEMUPutKbdEntry *qkbd;
> @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int scancode)
>      xenfb_send_key(xenfb, down, scancode2linux[scancode]);
>  }
>  
> +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int *dy)
> +{
> +    DisplaySurface *surface;
> +    int dw, dh;
> +
> +    if (xenfb->c.con == NULL) {
> +        return;
> +    }
> +    surface = qemu_console_surface(xenfb->c.con);
> +    if (surface == NULL) {
> +        return;
> +    }
> +    dw = surface_width(surface);
> +    dh = surface_height(surface);
> +
> +    *dx = *dx * (dw - 1) / 0x7fff;
> +    *dy = *dy * (dh - 1) / 0x7fff;
> +}
> +
>  /*
>   * Send a mouse event from the client to the guest OS
>   *
> @@ -338,18 +358,16 @@ 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;
>  
> -    if (xenfb->abs_pointer_wanted)
> -	xenfb_send_position(xenfb,
> -			    dx * (dw - 1) / 0x7fff,
> -			    dy * (dh - 1) / 0x7fff,
> -			    dz);
> -    else
> +    if (xenfb->abs_pointer_wanted) {
> +        if (!xenfb->no_abs_rescale) {
> +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> +        }
> +        xenfb_send_position(xenfb, dx, dy, dz);
> +    } else {
>  	xenfb_send_motion(xenfb, dx, dy, dz);
> +    }
>  
>      for (i = 0 ; i < 8 ; i++) {
>  	int lastDown = xenfb->button_state & (1 << i);
> @@ -366,6 +382,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-no-abs-rescale", 1);
>      return 0;
>  }
>  
> @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice *xendev)
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>      int rc;
>  
> -    if (!in->c.con) {
> +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> +                             &in->abs_pointer_wanted) == -1) {
> +        in->abs_pointer_wanted = 0;
> +    }
> +
> +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> +                             &in->no_abs_rescale) == -1) {
> +        in->no_abs_rescale = 0;
> +    }

I don't think is safe to move checking for "request-abs-pointer" so
early: what if the guest writes request-abs-pointer after initialise but
before connect?

Also it would be nice to document this protocol extension somewhere. I
do realize that the existing documentation on vfb is somewhat lacking.


> +    if (!in->c.con && !in->no_abs_rescale) {

Why this change?


>          xen_be_printf(xendev, 1, "ds not set (yet)\n");
>          return -1;
>      }
> @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice *xendev)
>  {
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>  
> -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> -                             &in->abs_pointer_wanted) == -1) {
> -        in->abs_pointer_wanted = 0;
> -    }
> -
>      if (in->qmouse) {
>          qemu_remove_mouse_event_handler(in->qmouse);
>      }
> -- 
> 2.1.0
> 

Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Stefano Stabellini]

On Wed, 10 Sep 2014, Owen smith wrote:
> Windows PV frontend requires absolute mouse values in the range
> [0, 0x7fff]. Add a feature to not rescale the axes to DisplaySurface
> size. Also allows Windows PV frontend to connect without a vfb
> device. Moves the rescaling of axes to a seperate function with
> additional null-pointer checks.
> 
> Signed-off-by: Owen smith <owen.smith@citrix.com>
> ---
>  hw/display/xenfb.c | 52 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index c4375c8..65c373b 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -61,6 +61,7 @@ struct common {
>  struct XenInput {
>      struct common c;
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
>      QEMUPutKbdEntry *qkbd;
> @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int scancode)
>      xenfb_send_key(xenfb, down, scancode2linux[scancode]);
>  }
>  
> +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int *dy)
> +{
> +    DisplaySurface *surface;
> +    int dw, dh;
> +
> +    if (xenfb->c.con == NULL) {
> +        return;
> +    }
> +    surface = qemu_console_surface(xenfb->c.con);
> +    if (surface == NULL) {
> +        return;
> +    }
> +    dw = surface_width(surface);
> +    dh = surface_height(surface);
> +
> +    *dx = *dx * (dw - 1) / 0x7fff;
> +    *dy = *dy * (dh - 1) / 0x7fff;
> +}
> +
>  /*
>   * Send a mouse event from the client to the guest OS
>   *
> @@ -338,18 +358,16 @@ 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;
>  
> -    if (xenfb->abs_pointer_wanted)
> -	xenfb_send_position(xenfb,
> -			    dx * (dw - 1) / 0x7fff,
> -			    dy * (dh - 1) / 0x7fff,
> -			    dz);
> -    else
> +    if (xenfb->abs_pointer_wanted) {
> +        if (!xenfb->no_abs_rescale) {
> +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> +        }
> +        xenfb_send_position(xenfb, dx, dy, dz);
> +    } else {
>  	xenfb_send_motion(xenfb, dx, dy, dz);
> +    }
>  
>      for (i = 0 ; i < 8 ; i++) {
>  	int lastDown = xenfb->button_state & (1 << i);
> @@ -366,6 +382,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-no-abs-rescale", 1);
>      return 0;
>  }
>  
> @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice *xendev)
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>      int rc;
>  
> -    if (!in->c.con) {
> +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> +                             &in->abs_pointer_wanted) == -1) {
> +        in->abs_pointer_wanted = 0;
> +    }
> +
> +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> +                             &in->no_abs_rescale) == -1) {
> +        in->no_abs_rescale = 0;
> +    }

I don't think is safe to move checking for "request-abs-pointer" so
early: what if the guest writes request-abs-pointer after initialise but
before connect?

Also it would be nice to document this protocol extension somewhere. I
do realize that the existing documentation on vfb is somewhat lacking.


> +    if (!in->c.con && !in->no_abs_rescale) {

Why this change?


>          xen_be_printf(xendev, 1, "ds not set (yet)\n");
>          return -1;
>      }
> @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice *xendev)
>  {
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>  
> -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> -                             &in->abs_pointer_wanted) == -1) {
> -        in->abs_pointer_wanted = 0;
> -    }
> -
>      if (in->qmouse) {
>          qemu_remove_mouse_event_handler(in->qmouse);
>      }
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH 1/3] xenfb: Unregister keyboard event handler correctly

[Owen Smith]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 11 September 2014 01:33
> To: Owen Smith
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [PATCH 1/3] xenfb: Unregister keyboard event handler correctly
> 
> On Wed, 10 Sep 2014, Owen smith wrote:
> > Keyboard event handler was replaced with a new handler on disconnect.
> > Use the unregister function to remove keyboard handler.
> >
> > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > ---
> >  hw/display/xenfb.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > 07ddc9d..2c39753 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -62,6 +62,7 @@ struct XenInput {
> >      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> >      int button_state;       /* Last seen pointer button state */
> >      int extended;
> > +    QEMUPutKbdEntry *qkbd;
> >      QEMUPutMouseEntry *qmouse;
> >  };
> >
> > @@ -364,7 +365,6 @@ static int input_initialise(struct XenDevice *xendev)
> >      if (rc != 0)
> >  	return rc;
> >
> > -    qemu_add_kbd_event_handler(xenfb_key_event, in);
> >      return 0;
> >  }
> >
> > @@ -383,17 +383,26 @@ 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);
> 
> This change is not described in the commit message.
> Why is this necessary? Is it related to the keyboard change?
> 

I'll split this change into a separate patch for v2.
Without this, the xenfb_mouse_event callback was not being called in response
to mouse events, as events are only delivered to the first matching handler in the
input chain.
Calling qemu_add_kbd_event_handler activates the handler internally, but
qemu_add_mouse_event_handler does not activate the handler.

> 
> > +    if (in->qkbd) {
> > +        qemu_remove_kbd_event_handler(in->qkbd);
> > +    }
> > +    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
> >  }
> >
> >  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.0
> >

Re: [PATCH 1/3] xenfb: Unregister keyboard event handler correctly

[Owen Smith]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 11 September 2014 01:33
> To: Owen Smith
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [PATCH 1/3] xenfb: Unregister keyboard event handler correctly
> 
> On Wed, 10 Sep 2014, Owen smith wrote:
> > Keyboard event handler was replaced with a new handler on disconnect.
> > Use the unregister function to remove keyboard handler.
> >
> > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > ---
> >  hw/display/xenfb.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > 07ddc9d..2c39753 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -62,6 +62,7 @@ struct XenInput {
> >      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
> >      int button_state;       /* Last seen pointer button state */
> >      int extended;
> > +    QEMUPutKbdEntry *qkbd;
> >      QEMUPutMouseEntry *qmouse;
> >  };
> >
> > @@ -364,7 +365,6 @@ static int input_initialise(struct XenDevice *xendev)
> >      if (rc != 0)
> >  	return rc;
> >
> > -    qemu_add_kbd_event_handler(xenfb_key_event, in);
> >      return 0;
> >  }
> >
> > @@ -383,17 +383,26 @@ 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);
> 
> This change is not described in the commit message.
> Why is this necessary? Is it related to the keyboard change?
> 

I'll split this change into a separate patch for v2.
Without this, the xenfb_mouse_event callback was not being called in response
to mouse events, as events are only delivered to the first matching handler in the
input chain.
Calling qemu_add_kbd_event_handler activates the handler internally, but
qemu_add_mouse_event_handler does not activate the handler.

> 
> > +    if (in->qkbd) {
> > +        qemu_remove_kbd_event_handler(in->qkbd);
> > +    }
> > +    in->qkbd = qemu_add_kbd_event_handler(xenfb_key_event, in);
> >  }
> >
> >  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.0
> >

Re: [Qemu-devel] [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Owen Smith]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 11 September 2014 02:01
> To: Owen Smith
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.
> 
> On Wed, 10 Sep 2014, Owen smith wrote:
> > The vkbd device should be able to use a grant reference to map the
> > shared page.
> >
> > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > ---
> >  hw/display/xenfb.c | 39 +++++++++++++++++++++++++++++----------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > 2c39753..c4375c8 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -54,6 +54,7 @@
> >  struct common {
> >      struct XenDevice  xendev;  /* must be first */
> >      void              *page;
> > +    int               page_gref;
> >      QemuConsole       *con;
> >  };
> >
> > @@ -96,22 +97,36 @@ static int common_bind(struct common *c)  {
> >      uint64_t mfn;
> >
> > -    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
> > -	return -1;
> > -    assert(mfn == (xen_pfn_t)mfn);
> > -
> >      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c-
> >xendev.remote_port) == -1)
> >  	return -1;
> >
> > -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > -				   XC_PAGE_SIZE,
> > -				   PROT_READ | PROT_WRITE, mfn);
> > +    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
> > +        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref)
> == -1) {
> > +            return -1;
> > +        }
> > +        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
> > +                                          c->xendev.dom,
> > +                                          c->page_gref,
> > +                                          PROT_READ | PROT_WRITE);
> > +    } else {
> > +        assert(mfn == (xen_pfn_t)mfn);
> > +
> > +        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > +                                       XC_PAGE_SIZE,
> > +                                       PROT_READ | PROT_WRITE, mfn);
> > +    }
> 
> This is an extension of the protocol. It should be documented somewhere.
> At least in the commit message and somewhere under docs/misc or
> xen/include/public/io/fbif.h.
> 

V2 will contain a better commit message, and I'll add a patch to the headers
to document the changes.
The same protocol extension docs should be in xen/include/public/io/fbif.h 
and xen/include/public/io/kdbif.h, as this change covers the vfb and vkbd, or 
would it be better to change the patch to only affect the vkbd?


> 
> >      if (c->page == NULL)
> >  	return -1;
> >
> >      xen_be_bind_evtchn(&c->xendev);
> > -    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> local-port %d\n",
> > -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> > +
> > +    if (c->page_gref) {
> > +        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-
> port %d\n",
> > +                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
> > +    } else {
> > +        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> local-port %d\n",
> > +                      mfn, c->xendev.remote_port, c->xendev.local_port);
> > +    }
> >
> >      return 0;
> >  }
> > @@ -120,8 +135,13 @@ static void common_unbind(struct common *c)  {
> >      xen_be_unbind_evtchn(&c->xendev);
> >      if (c->page) {
> > -	munmap(c->page, XC_PAGE_SIZE);
> > +        if (c->page_gref) {
> > +            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
> > +        } else {
> > +            munmap(c->page, XC_PAGE_SIZE);
> > +        }
> >  	c->page = NULL;
> > +        c->page_gref = 0;
> >      }
> >  }
> >
> > @@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
> >
> >  struct XenDevOps xen_kbdmouse_ops = {
> >      .size       = sizeof(struct XenInput),
> > +    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
> >      .init       = input_init,
> >      .initialise = input_initialise,
> >      .connected  = input_connected,
> > --
> > 2.1.0
> >

Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Owen Smith]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 11 September 2014 02:01
> To: Owen Smith
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.
> 
> On Wed, 10 Sep 2014, Owen smith wrote:
> > The vkbd device should be able to use a grant reference to map the
> > shared page.
> >
> > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > ---
> >  hw/display/xenfb.c | 39 +++++++++++++++++++++++++++++----------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > 2c39753..c4375c8 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -54,6 +54,7 @@
> >  struct common {
> >      struct XenDevice  xendev;  /* must be first */
> >      void              *page;
> > +    int               page_gref;
> >      QemuConsole       *con;
> >  };
> >
> > @@ -96,22 +97,36 @@ static int common_bind(struct common *c)  {
> >      uint64_t mfn;
> >
> > -    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
> > -	return -1;
> > -    assert(mfn == (xen_pfn_t)mfn);
> > -
> >      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c-
> >xendev.remote_port) == -1)
> >  	return -1;
> >
> > -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > -				   XC_PAGE_SIZE,
> > -				   PROT_READ | PROT_WRITE, mfn);
> > +    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
> > +        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref)
> == -1) {
> > +            return -1;
> > +        }
> > +        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
> > +                                          c->xendev.dom,
> > +                                          c->page_gref,
> > +                                          PROT_READ | PROT_WRITE);
> > +    } else {
> > +        assert(mfn == (xen_pfn_t)mfn);
> > +
> > +        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > +                                       XC_PAGE_SIZE,
> > +                                       PROT_READ | PROT_WRITE, mfn);
> > +    }
> 
> This is an extension of the protocol. It should be documented somewhere.
> At least in the commit message and somewhere under docs/misc or
> xen/include/public/io/fbif.h.
> 

V2 will contain a better commit message, and I'll add a patch to the headers
to document the changes.
The same protocol extension docs should be in xen/include/public/io/fbif.h 
and xen/include/public/io/kdbif.h, as this change covers the vfb and vkbd, or 
would it be better to change the patch to only affect the vkbd?


> 
> >      if (c->page == NULL)
> >  	return -1;
> >
> >      xen_be_bind_evtchn(&c->xendev);
> > -    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> local-port %d\n",
> > -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> > +
> > +    if (c->page_gref) {
> > +        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-
> port %d\n",
> > +                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
> > +    } else {
> > +        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> local-port %d\n",
> > +                      mfn, c->xendev.remote_port, c->xendev.local_port);
> > +    }
> >
> >      return 0;
> >  }
> > @@ -120,8 +135,13 @@ static void common_unbind(struct common *c)  {
> >      xen_be_unbind_evtchn(&c->xendev);
> >      if (c->page) {
> > -	munmap(c->page, XC_PAGE_SIZE);
> > +        if (c->page_gref) {
> > +            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
> > +        } else {
> > +            munmap(c->page, XC_PAGE_SIZE);
> > +        }
> >  	c->page = NULL;
> > +        c->page_gref = 0;
> >      }
> >  }
> >
> > @@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
> >
> >  struct XenDevOps xen_kbdmouse_ops = {
> >      .size       = sizeof(struct XenInput),
> > +    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
> >      .init       = input_init,
> >      .initialise = input_initialise,
> >      .connected  = input_connected,
> > --
> > 2.1.0
> >

Re: [Qemu-devel] [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Owen Smith]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 11 September 2014 02:23
> To: Owen Smith
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows
> PV frontend
> 
> On Wed, 10 Sep 2014, Owen smith wrote:
> > Windows PV frontend requires absolute mouse values in the range [0,
> > 0x7fff]. Add a feature to not rescale the axes to DisplaySurface size.
> > Also allows Windows PV frontend to connect without a vfb device. Moves
> > the rescaling of axes to a seperate function with additional
> > null-pointer checks.
> >
> > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > ---
> >  hw/display/xenfb.c | 52
> > +++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 37 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > c4375c8..65c373b 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -61,6 +61,7 @@ struct common {
> >  struct XenInput {
> >      struct common c;
> >      int abs_pointer_wanted; /* Whether guest supports absolute
> > pointer */
> > +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
> >      int button_state;       /* Last seen pointer button state */
> >      int extended;
> >      QEMUPutKbdEntry *qkbd;
> > @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int
> scancode)
> >      xenfb_send_key(xenfb, down, scancode2linux[scancode]);  }
> >
> > +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int
> > +*dy) {
> > +    DisplaySurface *surface;
> > +    int dw, dh;
> > +
> > +    if (xenfb->c.con == NULL) {
> > +        return;
> > +    }
> > +    surface = qemu_console_surface(xenfb->c.con);
> > +    if (surface == NULL) {
> > +        return;
> > +    }
> > +    dw = surface_width(surface);
> > +    dh = surface_height(surface);
> > +
> > +    *dx = *dx * (dw - 1) / 0x7fff;
> > +    *dy = *dy * (dh - 1) / 0x7fff;
> > +}
> > +
> >  /*
> >   * Send a mouse event from the client to the guest OS
> >   *
> > @@ -338,18 +358,16 @@ 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;
> >
> > -    if (xenfb->abs_pointer_wanted)
> > -	xenfb_send_position(xenfb,
> > -			    dx * (dw - 1) / 0x7fff,
> > -			    dy * (dh - 1) / 0x7fff,
> > -			    dz);
> > -    else
> > +    if (xenfb->abs_pointer_wanted) {
> > +        if (!xenfb->no_abs_rescale) {
> > +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> > +        }
> > +        xenfb_send_position(xenfb, dx, dy, dz);
> > +    } else {
> >  	xenfb_send_motion(xenfb, dx, dy, dz);
> > +    }
> >
> >      for (i = 0 ; i < 8 ; i++) {
> >  	int lastDown = xenfb->button_state & (1 << i); @@ -366,6 +382,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-no-abs-rescale", 1);
> >      return 0;
> >  }
> >
> > @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice
> *xendev)
> >      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
> >      int rc;
> >
> > -    if (!in->c.con) {
> > +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > +                             &in->abs_pointer_wanted) == -1) {
> > +        in->abs_pointer_wanted = 0;
> > +    }
> > +
> > +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> > +                             &in->no_abs_rescale) == -1) {
> > +        in->no_abs_rescale = 0;
> > +    }
> 
> I don't think is safe to move checking for "request-abs-pointer" so
> early: what if the guest writes request-abs-pointer after initialise but before
> connect?
> 
> Also it would be nice to document this protocol extension somewhere. I do
> realize that the existing documentation on vfb is somewhat lacking.
> 

I'll change this to read the feature flags during the input_connected for v2. 
I've spotted that neither feature has anything in the xen header, so I'll include 
a patch to document this with v2.

> 
> > +    if (!in->c.con && !in->no_abs_rescale) {
> 
> Why this change?
> 

Without an additional check here, the frontend would not connect. in->c.con is
not initialised without a vfb device. Overloading no_abs_rescale here is not right,
I'll add an option that the frontend can set to ignore the absence of the console.


> 
> >          xen_be_printf(xendev, 1, "ds not set (yet)\n");
> >          return -1;
> >      }
> > @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice
> > *xendev)  {
> >      struct XenInput *in = container_of(xendev, struct XenInput,
> > c.xendev);
> >
> > -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > -                             &in->abs_pointer_wanted) == -1) {
> > -        in->abs_pointer_wanted = 0;
> > -    }
> > -
> >      if (in->qmouse) {
> >          qemu_remove_mouse_event_handler(in->qmouse);
> >      }
> > --
> > 2.1.0
> >

Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Owen Smith]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 11 September 2014 02:23
> To: Owen Smith
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows
> PV frontend
> 
> On Wed, 10 Sep 2014, Owen smith wrote:
> > Windows PV frontend requires absolute mouse values in the range [0,
> > 0x7fff]. Add a feature to not rescale the axes to DisplaySurface size.
> > Also allows Windows PV frontend to connect without a vfb device. Moves
> > the rescaling of axes to a seperate function with additional
> > null-pointer checks.
> >
> > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > ---
> >  hw/display/xenfb.c | 52
> > +++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 37 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > c4375c8..65c373b 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -61,6 +61,7 @@ struct common {
> >  struct XenInput {
> >      struct common c;
> >      int abs_pointer_wanted; /* Whether guest supports absolute
> > pointer */
> > +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
> >      int button_state;       /* Last seen pointer button state */
> >      int extended;
> >      QEMUPutKbdEntry *qkbd;
> > @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int
> scancode)
> >      xenfb_send_key(xenfb, down, scancode2linux[scancode]);  }
> >
> > +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int
> > +*dy) {
> > +    DisplaySurface *surface;
> > +    int dw, dh;
> > +
> > +    if (xenfb->c.con == NULL) {
> > +        return;
> > +    }
> > +    surface = qemu_console_surface(xenfb->c.con);
> > +    if (surface == NULL) {
> > +        return;
> > +    }
> > +    dw = surface_width(surface);
> > +    dh = surface_height(surface);
> > +
> > +    *dx = *dx * (dw - 1) / 0x7fff;
> > +    *dy = *dy * (dh - 1) / 0x7fff;
> > +}
> > +
> >  /*
> >   * Send a mouse event from the client to the guest OS
> >   *
> > @@ -338,18 +358,16 @@ 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;
> >
> > -    if (xenfb->abs_pointer_wanted)
> > -	xenfb_send_position(xenfb,
> > -			    dx * (dw - 1) / 0x7fff,
> > -			    dy * (dh - 1) / 0x7fff,
> > -			    dz);
> > -    else
> > +    if (xenfb->abs_pointer_wanted) {
> > +        if (!xenfb->no_abs_rescale) {
> > +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> > +        }
> > +        xenfb_send_position(xenfb, dx, dy, dz);
> > +    } else {
> >  	xenfb_send_motion(xenfb, dx, dy, dz);
> > +    }
> >
> >      for (i = 0 ; i < 8 ; i++) {
> >  	int lastDown = xenfb->button_state & (1 << i); @@ -366,6 +382,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-no-abs-rescale", 1);
> >      return 0;
> >  }
> >
> > @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice
> *xendev)
> >      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
> >      int rc;
> >
> > -    if (!in->c.con) {
> > +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > +                             &in->abs_pointer_wanted) == -1) {
> > +        in->abs_pointer_wanted = 0;
> > +    }
> > +
> > +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> > +                             &in->no_abs_rescale) == -1) {
> > +        in->no_abs_rescale = 0;
> > +    }
> 
> I don't think is safe to move checking for "request-abs-pointer" so
> early: what if the guest writes request-abs-pointer after initialise but before
> connect?
> 
> Also it would be nice to document this protocol extension somewhere. I do
> realize that the existing documentation on vfb is somewhat lacking.
> 

I'll change this to read the feature flags during the input_connected for v2. 
I've spotted that neither feature has anything in the xen header, so I'll include 
a patch to document this with v2.

> 
> > +    if (!in->c.con && !in->no_abs_rescale) {
> 
> Why this change?
> 

Without an additional check here, the frontend would not connect. in->c.con is
not initialised without a vfb device. Overloading no_abs_rescale here is not right,
I'll add an option that the frontend can set to ignore the absence of the console.


> 
> >          xen_be_printf(xendev, 1, "ds not set (yet)\n");
> >          return -1;
> >      }
> > @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice
> > *xendev)  {
> >      struct XenInput *in = container_of(xendev, struct XenInput,
> > c.xendev);
> >
> > -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > -                             &in->abs_pointer_wanted) == -1) {
> > -        in->abs_pointer_wanted = 0;
> > -    }
> > -
> >      if (in->qmouse) {
> >          qemu_remove_mouse_event_handler(in->qmouse);
> >      }
> > --
> > 2.1.0
> >

Re: [Qemu-devel] [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Stefano Stabellini]

On Thu, 11 Sep 2014, Owen Smith wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 11 September 2014 02:01
> > To: Owen Smith
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> > Subject: Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.
> > 
> > On Wed, 10 Sep 2014, Owen smith wrote:
> > > The vkbd device should be able to use a grant reference to map the
> > > shared page.
> > >
> > > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > > ---
> > >  hw/display/xenfb.c | 39 +++++++++++++++++++++++++++++----------
> > >  1 file changed, 29 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > > 2c39753..c4375c8 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -54,6 +54,7 @@
> > >  struct common {
> > >      struct XenDevice  xendev;  /* must be first */
> > >      void              *page;
> > > +    int               page_gref;
> > >      QemuConsole       *con;
> > >  };
> > >
> > > @@ -96,22 +97,36 @@ static int common_bind(struct common *c)  {
> > >      uint64_t mfn;
> > >
> > > -    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
> > > -	return -1;
> > > -    assert(mfn == (xen_pfn_t)mfn);
> > > -
> > >      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c-
> > >xendev.remote_port) == -1)
> > >  	return -1;
> > >
> > > -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > > -				   XC_PAGE_SIZE,
> > > -				   PROT_READ | PROT_WRITE, mfn);
> > > +    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
> > > +        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref)
> > == -1) {
> > > +            return -1;
> > > +        }
> > > +        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
> > > +                                          c->xendev.dom,
> > > +                                          c->page_gref,
> > > +                                          PROT_READ | PROT_WRITE);
> > > +    } else {
> > > +        assert(mfn == (xen_pfn_t)mfn);
> > > +
> > > +        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > > +                                       XC_PAGE_SIZE,
> > > +                                       PROT_READ | PROT_WRITE, mfn);
> > > +    }
> > 
> > This is an extension of the protocol. It should be documented somewhere.
> > At least in the commit message and somewhere under docs/misc or
> > xen/include/public/io/fbif.h.
> > 
> 
> V2 will contain a better commit message, and I'll add a patch to the headers
> to document the changes.
> The same protocol extension docs should be in xen/include/public/io/fbif.h 
> and xen/include/public/io/kdbif.h, as this change covers the vfb and vkbd, or 
> would it be better to change the patch to only affect the vkbd?

I am OK with changing both vfb and vkbd but in that case you need to
document both.


> > 
> > >      if (c->page == NULL)
> > >  	return -1;
> > >
> > >      xen_be_bind_evtchn(&c->xendev);
> > > -    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> > local-port %d\n",
> > > -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> > > +
> > > +    if (c->page_gref) {
> > > +        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-
> > port %d\n",
> > > +                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
> > > +    } else {
> > > +        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> > local-port %d\n",
> > > +                      mfn, c->xendev.remote_port, c->xendev.local_port);
> > > +    }
> > >
> > >      return 0;
> > >  }
> > > @@ -120,8 +135,13 @@ static void common_unbind(struct common *c)  {
> > >      xen_be_unbind_evtchn(&c->xendev);
> > >      if (c->page) {
> > > -	munmap(c->page, XC_PAGE_SIZE);
> > > +        if (c->page_gref) {
> > > +            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
> > > +        } else {
> > > +            munmap(c->page, XC_PAGE_SIZE);
> > > +        }
> > >  	c->page = NULL;
> > > +        c->page_gref = 0;
> > >      }
> > >  }
> > >
> > > @@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
> > >
> > >  struct XenDevOps xen_kbdmouse_ops = {
> > >      .size       = sizeof(struct XenInput),
> > > +    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
> > >      .init       = input_init,
> > >      .initialise = input_initialise,
> > >      .connected  = input_connected,
> > > --
> > > 2.1.0
> > >
> 

Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.

[Stefano Stabellini]

On Thu, 11 Sep 2014, Owen Smith wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 11 September 2014 02:01
> > To: Owen Smith
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> > Subject: Re: [PATCH 2/3] xenfb: Add option to use grant ref for shared page.
> > 
> > On Wed, 10 Sep 2014, Owen smith wrote:
> > > The vkbd device should be able to use a grant reference to map the
> > > shared page.
> > >
> > > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > > ---
> > >  hw/display/xenfb.c | 39 +++++++++++++++++++++++++++++----------
> > >  1 file changed, 29 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > > 2c39753..c4375c8 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -54,6 +54,7 @@
> > >  struct common {
> > >      struct XenDevice  xendev;  /* must be first */
> > >      void              *page;
> > > +    int               page_gref;
> > >      QemuConsole       *con;
> > >  };
> > >
> > > @@ -96,22 +97,36 @@ static int common_bind(struct common *c)  {
> > >      uint64_t mfn;
> > >
> > > -    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
> > > -	return -1;
> > > -    assert(mfn == (xen_pfn_t)mfn);
> > > -
> > >      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c-
> > >xendev.remote_port) == -1)
> > >  	return -1;
> > >
> > > -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > > -				   XC_PAGE_SIZE,
> > > -				   PROT_READ | PROT_WRITE, mfn);
> > > +    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) {
> > > +        if (xenstore_read_fe_int(&c->xendev, "page-gref", &c->page_gref)
> > == -1) {
> > > +            return -1;
> > > +        }
> > > +        c->page = xc_gnttab_map_grant_ref(c->xendev.gnttabdev,
> > > +                                          c->xendev.dom,
> > > +                                          c->page_gref,
> > > +                                          PROT_READ | PROT_WRITE);
> > > +    } else {
> > > +        assert(mfn == (xen_pfn_t)mfn);
> > > +
> > > +        c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > > +                                       XC_PAGE_SIZE,
> > > +                                       PROT_READ | PROT_WRITE, mfn);
> > > +    }
> > 
> > This is an extension of the protocol. It should be documented somewhere.
> > At least in the commit message and somewhere under docs/misc or
> > xen/include/public/io/fbif.h.
> > 
> 
> V2 will contain a better commit message, and I'll add a patch to the headers
> to document the changes.
> The same protocol extension docs should be in xen/include/public/io/fbif.h 
> and xen/include/public/io/kdbif.h, as this change covers the vfb and vkbd, or 
> would it be better to change the patch to only affect the vkbd?

I am OK with changing both vfb and vkbd but in that case you need to
document both.


> > 
> > >      if (c->page == NULL)
> > >  	return -1;
> > >
> > >      xen_be_bind_evtchn(&c->xendev);
> > > -    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> > local-port %d\n",
> > > -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> > > +
> > > +    if (c->page_gref) {
> > > +        xen_be_printf(&c->xendev, 1, "ring gref %d, remote-port %d, local-
> > port %d\n",
> > > +                      c->page_gref, c->xendev.remote_port, c->xendev.local_port);
> > > +    } else {
> > > +        xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d,
> > local-port %d\n",
> > > +                      mfn, c->xendev.remote_port, c->xendev.local_port);
> > > +    }
> > >
> > >      return 0;
> > >  }
> > > @@ -120,8 +135,13 @@ static void common_unbind(struct common *c)  {
> > >      xen_be_unbind_evtchn(&c->xendev);
> > >      if (c->page) {
> > > -	munmap(c->page, XC_PAGE_SIZE);
> > > +        if (c->page_gref) {
> > > +            xc_gnttab_munmap(c->xendev.gnttabdev, c->page, 1);
> > > +        } else {
> > > +            munmap(c->page, XC_PAGE_SIZE);
> > > +        }
> > >  	c->page = NULL;
> > > +        c->page_gref = 0;
> > >      }
> > >  }
> > >
> > > @@ -954,6 +974,7 @@ static void fb_event(struct XenDevice *xendev)
> > >
> > >  struct XenDevOps xen_kbdmouse_ops = {
> > >      .size       = sizeof(struct XenInput),
> > > +    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
> > >      .init       = input_init,
> > >      .initialise = input_initialise,
> > >      .connected  = input_connected,
> > > --
> > > 2.1.0
> > >
> 

Re: [Qemu-devel] [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Stefano Stabellini]

On Thu, 11 Sep 2014, Owen Smith wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 11 September 2014 02:23
> > To: Owen Smith
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> > Subject: Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows
> > PV frontend
> > 
> > On Wed, 10 Sep 2014, Owen smith wrote:
> > > Windows PV frontend requires absolute mouse values in the range [0,
> > > 0x7fff]. Add a feature to not rescale the axes to DisplaySurface size.
> > > Also allows Windows PV frontend to connect without a vfb device. Moves
> > > the rescaling of axes to a seperate function with additional
> > > null-pointer checks.
> > >
> > > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > > ---
> > >  hw/display/xenfb.c | 52
> > > +++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 37 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > > c4375c8..65c373b 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -61,6 +61,7 @@ struct common {
> > >  struct XenInput {
> > >      struct common c;
> > >      int abs_pointer_wanted; /* Whether guest supports absolute
> > > pointer */
> > > +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
> > >      int button_state;       /* Last seen pointer button state */
> > >      int extended;
> > >      QEMUPutKbdEntry *qkbd;
> > > @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int
> > scancode)
> > >      xenfb_send_key(xenfb, down, scancode2linux[scancode]);  }
> > >
> > > +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int
> > > +*dy) {
> > > +    DisplaySurface *surface;
> > > +    int dw, dh;
> > > +
> > > +    if (xenfb->c.con == NULL) {
> > > +        return;
> > > +    }
> > > +    surface = qemu_console_surface(xenfb->c.con);
> > > +    if (surface == NULL) {
> > > +        return;
> > > +    }
> > > +    dw = surface_width(surface);
> > > +    dh = surface_height(surface);
> > > +
> > > +    *dx = *dx * (dw - 1) / 0x7fff;
> > > +    *dy = *dy * (dh - 1) / 0x7fff;
> > > +}
> > > +
> > >  /*
> > >   * Send a mouse event from the client to the guest OS
> > >   *
> > > @@ -338,18 +358,16 @@ 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;
> > >
> > > -    if (xenfb->abs_pointer_wanted)
> > > -	xenfb_send_position(xenfb,
> > > -			    dx * (dw - 1) / 0x7fff,
> > > -			    dy * (dh - 1) / 0x7fff,
> > > -			    dz);
> > > -    else
> > > +    if (xenfb->abs_pointer_wanted) {
> > > +        if (!xenfb->no_abs_rescale) {
> > > +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> > > +        }
> > > +        xenfb_send_position(xenfb, dx, dy, dz);
> > > +    } else {
> > >  	xenfb_send_motion(xenfb, dx, dy, dz);
> > > +    }
> > >
> > >      for (i = 0 ; i < 8 ; i++) {
> > >  	int lastDown = xenfb->button_state & (1 << i); @@ -366,6 +382,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-no-abs-rescale", 1);
> > >      return 0;
> > >  }
> > >
> > > @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice
> > *xendev)
> > >      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
> > >      int rc;
> > >
> > > -    if (!in->c.con) {
> > > +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > > +                             &in->abs_pointer_wanted) == -1) {
> > > +        in->abs_pointer_wanted = 0;
> > > +    }
> > > +
> > > +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> > > +                             &in->no_abs_rescale) == -1) {
> > > +        in->no_abs_rescale = 0;
> > > +    }
> > 
> > I don't think is safe to move checking for "request-abs-pointer" so
> > early: what if the guest writes request-abs-pointer after initialise but before
> > connect?
> > 
> > Also it would be nice to document this protocol extension somewhere. I do
> > realize that the existing documentation on vfb is somewhat lacking.
> > 
> 
> I'll change this to read the feature flags during the input_connected for v2. 
> I've spotted that neither feature has anything in the xen header, so I'll include 
> a patch to document this with v2.

Thanks


> > > +    if (!in->c.con && !in->no_abs_rescale) {
> > 
> > Why this change?
> > 
> 
> Without an additional check here, the frontend would not connect. in->c.con is
> not initialised without a vfb device. Overloading no_abs_rescale here is not right,
> I'll add an option that the frontend can set to ignore the absence of the console.

You don't need a new option for that. We wouldn't be introducing any
regression by letting vkdb connect without a vfb device.  You could just
remove the check.


> 
> > 
> > >          xen_be_printf(xendev, 1, "ds not set (yet)\n");
> > >          return -1;
> > >      }
> > > @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice
> > > *xendev)  {
> > >      struct XenInput *in = container_of(xendev, struct XenInput,
> > > c.xendev);
> > >
> > > -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > > -                             &in->abs_pointer_wanted) == -1) {
> > > -        in->abs_pointer_wanted = 0;
> > > -    }
> > > -
> > >      if (in->qmouse) {
> > >          qemu_remove_mouse_event_handler(in->qmouse);
> > >      }
> > > --
> > > 2.1.0
> > >
> 

Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Stefano Stabellini]

On Thu, 11 Sep 2014, Owen Smith wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 11 September 2014 02:23
> > To: Owen Smith
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> > Subject: Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows
> > PV frontend
> > 
> > On Wed, 10 Sep 2014, Owen smith wrote:
> > > Windows PV frontend requires absolute mouse values in the range [0,
> > > 0x7fff]. Add a feature to not rescale the axes to DisplaySurface size.
> > > Also allows Windows PV frontend to connect without a vfb device. Moves
> > > the rescaling of axes to a seperate function with additional
> > > null-pointer checks.
> > >
> > > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > > ---
> > >  hw/display/xenfb.c | 52
> > > +++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 37 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > > c4375c8..65c373b 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -61,6 +61,7 @@ struct common {
> > >  struct XenInput {
> > >      struct common c;
> > >      int abs_pointer_wanted; /* Whether guest supports absolute
> > > pointer */
> > > +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
> > >      int button_state;       /* Last seen pointer button state */
> > >      int extended;
> > >      QEMUPutKbdEntry *qkbd;
> > > @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int
> > scancode)
> > >      xenfb_send_key(xenfb, down, scancode2linux[scancode]);  }
> > >
> > > +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int
> > > +*dy) {
> > > +    DisplaySurface *surface;
> > > +    int dw, dh;
> > > +
> > > +    if (xenfb->c.con == NULL) {
> > > +        return;
> > > +    }
> > > +    surface = qemu_console_surface(xenfb->c.con);
> > > +    if (surface == NULL) {
> > > +        return;
> > > +    }
> > > +    dw = surface_width(surface);
> > > +    dh = surface_height(surface);
> > > +
> > > +    *dx = *dx * (dw - 1) / 0x7fff;
> > > +    *dy = *dy * (dh - 1) / 0x7fff;
> > > +}
> > > +
> > >  /*
> > >   * Send a mouse event from the client to the guest OS
> > >   *
> > > @@ -338,18 +358,16 @@ 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;
> > >
> > > -    if (xenfb->abs_pointer_wanted)
> > > -	xenfb_send_position(xenfb,
> > > -			    dx * (dw - 1) / 0x7fff,
> > > -			    dy * (dh - 1) / 0x7fff,
> > > -			    dz);
> > > -    else
> > > +    if (xenfb->abs_pointer_wanted) {
> > > +        if (!xenfb->no_abs_rescale) {
> > > +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> > > +        }
> > > +        xenfb_send_position(xenfb, dx, dy, dz);
> > > +    } else {
> > >  	xenfb_send_motion(xenfb, dx, dy, dz);
> > > +    }
> > >
> > >      for (i = 0 ; i < 8 ; i++) {
> > >  	int lastDown = xenfb->button_state & (1 << i); @@ -366,6 +382,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-no-abs-rescale", 1);
> > >      return 0;
> > >  }
> > >
> > > @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice
> > *xendev)
> > >      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
> > >      int rc;
> > >
> > > -    if (!in->c.con) {
> > > +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > > +                             &in->abs_pointer_wanted) == -1) {
> > > +        in->abs_pointer_wanted = 0;
> > > +    }
> > > +
> > > +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> > > +                             &in->no_abs_rescale) == -1) {
> > > +        in->no_abs_rescale = 0;
> > > +    }
> > 
> > I don't think is safe to move checking for "request-abs-pointer" so
> > early: what if the guest writes request-abs-pointer after initialise but before
> > connect?
> > 
> > Also it would be nice to document this protocol extension somewhere. I do
> > realize that the existing documentation on vfb is somewhat lacking.
> > 
> 
> I'll change this to read the feature flags during the input_connected for v2. 
> I've spotted that neither feature has anything in the xen header, so I'll include 
> a patch to document this with v2.

Thanks


> > > +    if (!in->c.con && !in->no_abs_rescale) {
> > 
> > Why this change?
> > 
> 
> Without an additional check here, the frontend would not connect. in->c.con is
> not initialised without a vfb device. Overloading no_abs_rescale here is not right,
> I'll add an option that the frontend can set to ignore the absence of the console.

You don't need a new option for that. We wouldn't be introducing any
regression by letting vkdb connect without a vfb device.  You could just
remove the check.


> 
> > 
> > >          xen_be_printf(xendev, 1, "ds not set (yet)\n");
> > >          return -1;
> > >      }
> > > @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice
> > > *xendev)  {
> > >      struct XenInput *in = container_of(xendev, struct XenInput,
> > > c.xendev);
> > >
> > > -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > > -                             &in->abs_pointer_wanted) == -1) {
> > > -        in->abs_pointer_wanted = 0;
> > > -    }
> > > -
> > >      if (in->qmouse) {
> > >          qemu_remove_mouse_event_handler(in->qmouse);
> > >      }
> > > --
> > > 2.1.0
> > >
> 

Re: [Qemu-devel] [Xen-devel] [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Ian Campbell]

On Thu, 2014-09-11 at 02:23 +0100, Stefano Stabellini wrote:

Stefano,

In the context of this patch could you also take a look at Owen's xenfb
documentation patches[0], I think you probably know the most about this
particular protocol.

Ian.

[0] <1411376699-8175-1-git-send-email-owen.smith@citrix.com> and
friends.

Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Ian Campbell]

On Thu, 2014-09-11 at 02:23 +0100, Stefano Stabellini wrote:

Stefano,

In the context of this patch could you also take a look at Owen's xenfb
documentation patches[0], I think you probably know the most about this
particular protocol.

Ian.

[0] <1411376699-8175-1-git-send-email-owen.smith@citrix.com> and
friends.