xenbus_backend_client.c / xenbus_client.c merger

xenbus_backend_client.c / xenbus_client.c merger

[Kieran Mansley]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

Although I don't know the full history, it looks like at some point
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_backend_client.c was
created to hold "backend only" code that would otherwise be in
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c.

However, the code in xenbus_backend_client.c isn't currently specific to
the backend - it just happens to be that no frontends use it.  At least
that's how it looks to me.

A frontend I'm working on (and have submitted here as a patch a few
times) does use this "backend only" code.  In particular the
xenbus_map_ring family of functions.  I would therefore like to move
this from xenbus_backend_client.c into xenbus_client.c.  This would
leave xenbus_backend_client.c containing just xenbus_dev_is_online()
which also looks potentially useful to a frontend, and so I propose
moving that too and then removing the xenbus_backend_client.c file
altogether.

Does anyone have any comments on this change?  I'm happy to submit a
patch for it (e.g. see attached).  In particular, if anyone is aware of
the reason for this code split that would be interesting as I like to
understand things I'm changing!

Functionally this only affects those using separate domU/dom0 kernels.
If using a unified kernel the symbols from both files were available in
both domains regardless.

Thanks

Kieran

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

Merge xenbus_backend_client.c with xenbus_client.c

diff -r b5fc88aad1b0 linux-2.6-xen-sparse/drivers/xen/xenbus/Makefile
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/Makefile	Sun Feb 18 15:29:40 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/Makefile	Mon Feb 19 13:56:52 2007 +0000
@@ -1,8 +1,5 @@ obj-y += xenbus_client.o xenbus_comms.o 
 obj-y += xenbus_client.o xenbus_comms.o xenbus_xs.o xenbus_probe.o
 obj-$(CONFIG_XEN_BACKEND) += xenbus_be.o
-
-xenbus_be-objs =
-xenbus_be-objs += xenbus_backend_client.o
 
 xenbus-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
 obj-y += $(xenbus-y) $(xenbus-m)
diff -r b5fc88aad1b0 linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c	Sun Feb 18 15:29:40 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c	Mon Feb 19 13:56:54 2007 +0000
@@ -281,3 +281,113 @@ enum xenbus_state xenbus_read_driver_sta
 	return result;
 }
 EXPORT_SYMBOL_GPL(xenbus_read_driver_state);
+
+
+/* Based on Rusty Russell's skeleton driver's map_page */
+struct vm_struct *xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref)
+{
+	struct gnttab_map_grant_ref op;
+	struct vm_struct *area;
+
+	area = alloc_vm_area(PAGE_SIZE);
+	if (!area)
+		return ERR_PTR(-ENOMEM);
+
+	gnttab_set_map_op(&op, (unsigned long)area->addr, GNTMAP_host_map,
+			  gnt_ref, dev->otherend_id);
+	
+	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
+		BUG();
+
+	if (op.status != GNTST_okay) {
+		free_vm_area(area);
+		xenbus_dev_fatal(dev, op.status,
+				 "mapping in shared page %d from domain %d",
+				 gnt_ref, dev->otherend_id);
+		BUG_ON(!IS_ERR(ERR_PTR(op.status)));
+		return ERR_PTR(op.status);
+	}
+
+	/* Stuff the handle in an unused field */
+	area->phys_addr = (unsigned long)op.handle;
+
+	return area;
+}
+EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
+
+
+int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
+		   grant_handle_t *handle, void *vaddr)
+{
+	struct gnttab_map_grant_ref op;
+	
+	gnttab_set_map_op(&op, (unsigned long)vaddr, GNTMAP_host_map,
+			  gnt_ref, dev->otherend_id);
+	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
+		BUG();
+
+	if (op.status != GNTST_okay) {
+		xenbus_dev_fatal(dev, op.status,
+				 "mapping in shared page %d from domain %d",
+				 gnt_ref, dev->otherend_id);
+	} else
+		*handle = op.handle;
+
+	return op.status;
+}
+EXPORT_SYMBOL_GPL(xenbus_map_ring);
+
+
+/* Based on Rusty Russell's skeleton driver's unmap_page */
+int xenbus_unmap_ring_vfree(struct xenbus_device *dev, struct vm_struct *area)
+{
+	struct gnttab_unmap_grant_ref op;
+
+	gnttab_set_unmap_op(&op, (unsigned long)area->addr, GNTMAP_host_map,
+			    (grant_handle_t)area->phys_addr);
+
+	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
+		BUG();
+
+	if (op.status == GNTST_okay)
+		free_vm_area(area);
+	else
+		xenbus_dev_error(dev, op.status,
+				 "unmapping page at handle %d error %d",
+				 (int16_t)area->phys_addr, op.status);
+
+	return op.status;
+}
+EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
+
+
+int xenbus_unmap_ring(struct xenbus_device *dev,
+		     grant_handle_t handle, void *vaddr)
+{
+	struct gnttab_unmap_grant_ref op;
+
+	gnttab_set_unmap_op(&op, (unsigned long)vaddr, GNTMAP_host_map,
+			    handle);
+	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
+		BUG();
+
+	if (op.status != GNTST_okay)
+		xenbus_dev_error(dev, op.status,
+				 "unmapping page at handle %d error %d",
+				 handle, op.status);
+
+	return op.status;
+}
+EXPORT_SYMBOL_GPL(xenbus_unmap_ring);
+
+int xenbus_dev_is_online(struct xenbus_device *dev)
+{
+	int rc, val;
+
+	rc = xenbus_scanf(XBT_NIL, dev->nodename, "online", "%d", &val);
+	if (rc != 1)
+		val = 0; /* no online node present */
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(xenbus_dev_is_online);
diff -r b5fc88aad1b0 linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_backend_client.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_backend_client.c	Sun Feb 18 15:29:40 2007 +0000
+++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
@@ -1,147 +0,0 @@
-/******************************************************************************
- * Backend-client-facing interface for the Xenbus driver.  In other words, the
- * interface between the Xenbus and the device-specific code in the backend
- * driver.
- *
- * Copyright (C) 2005-2006 XenSource Ltd
- * 
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation; or, when distributed
- * separately from the Linux kernel or incorporated into other
- * software packages, subject to the following license:
- * 
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this source file (the "Software"), to deal in the Software without
- * restriction, including without limitation the rights to use, copy, modify,
- * merge, publish, distribute, sublicense, and/or sell copies of the Software,
- * and to permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- * 
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- * 
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- */
-
-#include <linux/err.h>
-#include <xen/gnttab.h>
-#include <xen/xenbus.h>
-#include <xen/driver_util.h>
-
-/* Based on Rusty Russell's skeleton driver's map_page */
-struct vm_struct *xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref)
-{
-	struct gnttab_map_grant_ref op;
-	struct vm_struct *area;
-
-	area = alloc_vm_area(PAGE_SIZE);
-	if (!area)
-		return ERR_PTR(-ENOMEM);
-
-	gnttab_set_map_op(&op, (unsigned long)area->addr, GNTMAP_host_map,
-			  gnt_ref, dev->otherend_id);
-	
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status != GNTST_okay) {
-		free_vm_area(area);
-		xenbus_dev_fatal(dev, op.status,
-				 "mapping in shared page %d from domain %d",
-				 gnt_ref, dev->otherend_id);
-		BUG_ON(!IS_ERR(ERR_PTR(op.status)));
-		return ERR_PTR(op.status);
-	}
-
-	/* Stuff the handle in an unused field */
-	area->phys_addr = (unsigned long)op.handle;
-
-	return area;
-}
-EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
-
-
-int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
-		   grant_handle_t *handle, void *vaddr)
-{
-	struct gnttab_map_grant_ref op;
-	
-	gnttab_set_map_op(&op, (unsigned long)vaddr, GNTMAP_host_map,
-			  gnt_ref, dev->otherend_id);
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status != GNTST_okay) {
-		xenbus_dev_fatal(dev, op.status,
-				 "mapping in shared page %d from domain %d",
-				 gnt_ref, dev->otherend_id);
-	} else
-		*handle = op.handle;
-
-	return op.status;
-}
-EXPORT_SYMBOL_GPL(xenbus_map_ring);
-
-
-/* Based on Rusty Russell's skeleton driver's unmap_page */
-int xenbus_unmap_ring_vfree(struct xenbus_device *dev, struct vm_struct *area)
-{
-	struct gnttab_unmap_grant_ref op;
-
-	gnttab_set_unmap_op(&op, (unsigned long)area->addr, GNTMAP_host_map,
-			    (grant_handle_t)area->phys_addr);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status == GNTST_okay)
-		free_vm_area(area);
-	else
-		xenbus_dev_error(dev, op.status,
-				 "unmapping page at handle %d error %d",
-				 (int16_t)area->phys_addr, op.status);
-
-	return op.status;
-}
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
-
-
-int xenbus_unmap_ring(struct xenbus_device *dev,
-		     grant_handle_t handle, void *vaddr)
-{
-	struct gnttab_unmap_grant_ref op;
-
-	gnttab_set_unmap_op(&op, (unsigned long)vaddr, GNTMAP_host_map,
-			    handle);
-	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status != GNTST_okay)
-		xenbus_dev_error(dev, op.status,
-				 "unmapping page at handle %d error %d",
-				 handle, op.status);
-
-	return op.status;
-}
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring);
-
-int xenbus_dev_is_online(struct xenbus_device *dev)
-{
-	int rc, val;
-
-	rc = xenbus_scanf(XBT_NIL, dev->nodename, "online", "%d", &val);
-	if (rc != 1)
-		val = 0; /* no online node present */
-
-	return val;
-}
-EXPORT_SYMBOL_GPL(xenbus_dev_is_online);
-
-MODULE_LICENSE("Dual BSD/GPL");

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: xenbus_backend_client.c / xenbus_client.c merger

[Keir Fraser]

On 19/2/07 14:00, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> Although I don't know the full history, it looks like at some point
> linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_backend_client.c was
> created to hold "backend only" code that would otherwise be in
> linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c.
> 
> However, the code in xenbus_backend_client.c isn't currently specific to
> the backend - it just happens to be that no frontends use it.  At least
> that's how it looks to me.

Currently we have a model that frontends supply memory for ring buffers,
which backends map into their kernel address space. Where is the memory
coming from that your frontend maps? Is it appropriate to be using grant
table entries to refer to and map that memory?

 -- Keir

Re: xenbus_backend_client.c / xenbus_client.c merger

[Kieran Mansley]

On Mon, 2007-02-19 at 15:17 +0000, Keir Fraser wrote:
> On 19/2/07 14:00, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> > Although I don't know the full history, it looks like at some point
> > linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_backend_client.c was
> > created to hold "backend only" code that would otherwise be in
> > linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c.
> > 
> > However, the code in xenbus_backend_client.c isn't currently specific to
> > the backend - it just happens to be that no frontends use it.  At least
> > that's how it looks to me.
> 
> Currently we have a model that frontends supply memory for ring buffers,
> which backends map into their kernel address space.

Which is on the whole a very good idea.

> Where is the memory
> coming from that your frontend maps?

It's host memory in dom0 which is also passed to our virtualisable
network interface cards.  The reason it's allocated by the backend in
dom0 rather than using the model above is that we need to be able to
allocate two physically contiguous pages, and I this would be tricky
from domU.  If you know of a way of doing this, that would be an
excellent alternative to needing to use the xenbus_backend_client code
in the frontend.

Technically we're not using the mapped memory as a ring buffer, but
xenbus_map_ring and friends are a convenient API to grants and mapping
them.

> Is it appropriate to be using grant
> table entries to refer to and map that memory?

I wasn't aware of an alternative mechanism so I'll hazard a "yes, it is
appropriate".  However, if given the above information you think
otherwise, let me know.

Kieran

Re: xenbus_backend_client.c / xenbus_client.c merger

[Keir Fraser]

On 19/2/07 16:08, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> It's host memory in dom0 which is also passed to our virtualisable
> network interface cards.  The reason it's allocated by the backend in
> dom0 rather than using the model above is that we need to be able to
> allocate two physically contiguous pages, and I this would be tricky
> from domU.  If you know of a way of doing this, that would be an
> excellent alternative to needing to use the xenbus_backend_client code
> in the frontend.


Okay, if we go this way then the functions probably need to be given more
general names (since they are not used just for descriptor rings any more)
and also will you not need them to generalise to accepting more than one
grant reference (since you want to map two grant references into a two-page
vm area)?

 -- Keir

Re: xenbus_backend_client.c / xenbus_client.c merger

[Kieran Mansley]

On Mon, 2007-02-19 at 16:50 +0000, Keir Fraser wrote:
> On 19/2/07 16:08, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> > It's host memory in dom0 which is also passed to our virtualisable
> > network interface cards.  The reason it's allocated by the backend in
> > dom0 rather than using the model above is that we need to be able to
> > allocate two physically contiguous pages, and I this would be tricky
> > from domU.  If you know of a way of doing this, that would be an
> > excellent alternative to needing to use the xenbus_backend_client code
> > in the frontend.
> 
> 
> Okay, if we go this way then the functions probably need to be given more
> general names (since they are not used just for descriptor rings any more)
> and also will you not need them to generalise to accepting more than one
> grant reference (since you want to map two grant references into a two-page
> vm area)?

I'm happy to make that change.  An alternative (and much smaller) change
would be to leave the existing map_ring API alone and augment with a
functionally similar version that could be used by the front end, and
was called something else to avoid confusion.  This would be my
preferred option I think, and would remove the need to move code out of
xenbus_backend_client. 

Accepting one grant reference is not a big deal - I can just get a grant
per page and pass all the grants, then allocate a two page vm_area map
the individual grants at the appropriate offsets into that area to get
them virtually contiguous.

Kieran

Re: xenbus_backend_client.c / xenbus_client.c merger

[Keir Fraser]

On 19/2/07 17:07, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> Accepting one grant reference is not a big deal - I can just get a grant
> per page and pass all the grants, then allocate a two page vm_area map
> the individual grants at the appropriate offsets into that area to get
> them virtually contiguous.

Then it doesn't seem that a new helper function is very useful! You'll just
end up with alloc_vm_area() followed by open coding of requesting mapping of
two grant references, won't you? A general function that did all that would
be reasonable, and then I'd get rid of the special-case 'ring' functions.

  -- Keir