diff mbox

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

Message ID 1410356546-22264-3-git-send-email-owen.smith@citrix.com
State New
Headers show

Commit Message

Owen Smith Sept. 10, 2014, 1:42 p.m. UTC 
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(-)

Comments

Stefano Stabellini Sept. 11, 2014, 1:01 a.m. UTC | #1 
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
>
Owen Smith Sept. 11, 2014, 11:33 a.m. UTC | #2 
> -----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
> >
Stefano Stabellini Sept. 11, 2014, 6:01 p.m. UTC | #3 
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
> > >
>
diff mbox

Patch

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,