* Re: [PATCH] skeleton frontend/backend examples and a deadlock
2005-11-02 5:22 [PATCH] skeleton frontend/backend examples and a deadlock Rusty Russell
@ 2005-11-02 12:37 ` Harry Butterworth
2005-11-03 1:36 ` Rusty Russell
2005-11-03 2:17 ` Mark Williamson
2005-11-02 12:52 ` Mark Ryden
2005-11-15 0:16 ` Ewan Mellor
2 siblings, 2 replies; 7+ messages in thread
From: Harry Butterworth @ 2005-11-02 12:37 UTC (permalink / raw)
To: Rusty Russell; +Cc: Xen Mailing List, mark.williamson
[-- Attachment #1: Type: text/plain, Size: 925 bytes --]
On Wed, 2005-11-02 at 16:22 +1100, Rusty Russell wrote:
> Here are example frontend and backend driver skeletons. They're
> *designed* to handle driver restart and module unloading. However,
> device_unregister deadlocks. I guess noone tried testing
> unregister_xenbus_watch when not called from a watch callback.
Thanks, that'll be useful for me to know when it comes to testing my
code.
For comparison, I've knocked up a skeleton FE/BE driver based on the
xenidc API that I'm using for my USB driver.
You'll see that my patch is smaller, simpler and provides significantly
more functionality: enough to send messages and transactions
bi-directionally between the FE and BE. For a fair comparison, yours
would require interrupt handlers, use of the RING macros, correct memory
barriers etc.
Hopefully this helps to explain why I've split out the IDC functionality
from my USB driver into a generic service.
Harry.
[-- Attachment #2: xenidc-skeleton-patch --]
[-- Type: text/x-patch, Size: 18699 bytes --]
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/arch/xen/Kconfig
--- a/linux-2.6-xen-sparse/arch/xen/Kconfig Wed Nov 2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/arch/xen/Kconfig Wed Nov 2 12:01:27 2005
@@ -210,6 +210,22 @@
If security is not a concern then you may increase performance by
saying N.
+config XEN_SKELETON_FE
+ tristate "Compile Xen skeleton example frontend driver code"
+ default m
+ help
+ There is an example skeleton driver frontend in drivers/xen/skeleton
+ which you can use as a basis for your own xenbus-aware
+ drivers.
+
+config XEN_SKELETON_BE
+ tristate "Compile Xen skeleton example backend driver code"
+ default m
+ help
+ There is an example skeleton driver backend in drivers/xen/skeleton
+ which you can use as a basis for your own xenbus-aware
+ drivers.
+
choice
prompt "Processor Type"
default XEN_X86
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/Makefile
--- a/linux-2.6-xen-sparse/drivers/xen/Makefile Wed Nov 2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/Makefile Wed Nov 2 12:01:27 2005
@@ -6,6 +6,7 @@
obj-y += privcmd/
obj-y += xenbus/
obj-y += xenidc/
+obj-y += skeleton/
obj-$(CONFIG_XEN_BLKDEV_BACKEND) += blkback/
obj-$(CONFIG_XEN_NETDEV_BACKEND) += netback/
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/skeleton/Makefile
--- /dev/null Wed Nov 2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/Makefile Wed Nov 2 12:01:27 2005
@@ -0,0 +1,2 @@
+obj-$(CONFIG_XEN_SKELETON_FE) += skeleton_fe.o
+obj-$(CONFIG_XEN_SKELETON_BE) += skeleton_be.o
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c
--- /dev/null Wed Nov 2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c Wed Nov 2 12:01:27 2005
@@ -0,0 +1,299 @@
+/*****************************************************************************/
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* This program is free software; you can redistribute it and/or modify it */
+/* under the terms of the GNU General Public License as published by the */
+/* Free Software Foundation; either version 2 of the License, or (at your */
+/* option) any later version. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+#include <linux/module.h>
+#include <asm-xen/xenbus.h>
+#include <asm-xen/xenidc.h>
+
+/* Skeleton driver doesn't actually send any messages or transactions. */
+
+#define SKELETON_BE_INITIATOR_QUOTA 0
+#define SKELETON_BE_INITIATOR_MAXIMUM_BYTE_COUNT 0
+#define SKELETON_BE_TARGET_QUOTA 0
+#define SKELETON_BE_TARGET_MAXIMUM_BYTE_COUNT 0
+
+struct skeleton_be_device
+{
+ struct xenbus_device * dev;
+ xenidc_address address;
+ long int frontend_id;
+ char * frontend;
+ xenidc_endpoint endpoint;
+ xenidc_callback * endpoint_disconnect_callback;
+};
+
+static inline struct skeleton_be_device * skeleton_be_device_endpoint_to
+ ( xenidc_endpoint * endpoint )
+{
+ return container_of( endpoint, struct skeleton_be_device, endpoint );
+}
+
+static void skeleton_be_device_endpoint_connect( xenidc_endpoint * endpoint )
+{
+ /* Between connect and completion of the disconnect callback we are */
+ /* allowed to issue messages and transactions. */
+}
+
+static void skeleton_be_device_endpoint_disconnect
+ ( xenidc_endpoint * endpoint, xenidc_callback * callback )
+{
+ /* We must stop issuing messages and transactions and complete the */
+ /* callback once all of the messages and transactions we are issuing */
+ /* have completed or failed back to us. */
+
+ /* The skeleton driver doesn't issue any messages or transactions so can */
+ /* complete the callback immediately. */
+
+ xenidc_callback_success( callback );
+}
+
+static void skeleton_be_device_endpoint_message
+ ( xenidc_endpoint * endpoint, xenidc_endpoint_message * message )
+{
+ /* Handle inbound message. */
+
+ xenidc_callback_success( xenidc_endpoint_message_to_callback( message ) );
+}
+
+static void skeleton_be_device_endpoint_transaction
+ ( xenidc_endpoint * endpoint, xenidc_endpoint_transaction * transaction )
+{
+ /* Handle inbound transaction. */
+
+ xenidc_callback_success
+ ( xenidc_endpoint_transaction_to_callback( transaction ) );
+}
+
+static int skeleton_be_device_suspend( struct xenbus_device * dev )
+{
+ struct skeleton_be_device * device = dev->data;
+
+ xenidc_endpoint_destroy( &device->endpoint );
+
+ kfree( device->frontend );
+
+ return 0;
+}
+
+static int skeleton_be_device_resume( struct xenbus_device * dev )
+{
+ struct skeleton_be_device * device = dev->data;
+
+ int return_value = xenbus_gather
+ (
+ NULL, /* FIXME? */
+ dev->nodename,
+ "frontend-id", "%li", &device->frontend_id,
+ "frontend", NULL, &device->frontend,
+ NULL
+ );
+
+ if( XENBUS_EXIST_ERR( return_value ) )
+ {
+ goto EXIT_NO_FRONTEND;
+ }
+
+ if( return_value < 0 )
+ {
+ xenbus_dev_error
+ (
+ dev,
+ return_value,
+ "reading %s/frontend",
+ dev->nodename
+ );
+
+ goto EXIT_NO_FRONTEND;
+ }
+
+ xenidc_address_init
+ (
+ &device->address,
+ dev->nodename,
+ device->frontend,
+ device->frontend_id
+ );
+
+ xenidc_endpoint_create( &device->endpoint, device->address );
+
+ return 0;
+
+ EXIT_NO_FRONTEND:
+
+ return return_value;
+}
+
+static int skeleton_be_device_init_or_exit
+ ( struct skeleton_be_device * device, struct xenbus_device * dev, int exit )
+{
+ int return_value = 0;
+
+ if( exit )
+ {
+ goto EXIT;
+ }
+
+ memset( device, 0, sizeof( *device ) );
+
+ device->dev = dev;
+
+ return_value = xenidc_endpoint_init
+ (
+ &device->endpoint,
+ skeleton_be_device_endpoint_connect,
+ skeleton_be_device_endpoint_message,
+ skeleton_be_device_endpoint_transaction,
+ skeleton_be_device_endpoint_disconnect,
+ SKELETON_BE_INITIATOR_QUOTA,
+ SKELETON_BE_INITIATOR_MAXIMUM_BYTE_COUNT,
+ SKELETON_BE_TARGET_QUOTA,
+ SKELETON_BE_TARGET_MAXIMUM_BYTE_COUNT
+ );
+
+ if( return_value != 0 )
+ {
+ goto EXIT_NO_ENDPOINT;
+ }
+
+ return_value = skeleton_be_device_resume( dev );
+
+ if( return_value != 0 )
+ {
+ goto EXIT_NO_RESUME;
+ }
+
+ return 0;
+
+ EXIT:
+
+ skeleton_be_device_suspend( device->dev );
+
+ EXIT_NO_RESUME:
+
+ xenidc_endpoint_exit( &device->endpoint );
+
+ EXIT_NO_ENDPOINT:
+
+ return return_value;
+}
+
+static int skeleton_be_device_init
+ ( struct skeleton_be_device * device, struct xenbus_device * dev )
+{
+ return skeleton_be_device_init_or_exit( device, dev, 0 );
+}
+
+static void skeleton_be_device_exit( struct skeleton_be_device * device )
+{
+ (void)skeleton_be_device_init_or_exit( device, NULL, 1 );
+}
+
+static int skeleton_be_device_probe_or_remove
+ ( struct xenbus_device * dev, int remove )
+{
+ int return_value = 0;
+
+ struct skeleton_be_device * device;
+
+ if( remove )
+ {
+ goto REMOVE;
+ }
+
+ device = kmalloc( sizeof( *device ), GFP_KERNEL );
+
+ if( device == NULL )
+ {
+ xenbus_dev_error( dev, -ENOMEM, "allocating backend structure" );
+
+ return_value = -ENOMEM;
+
+ goto EXIT_NO_DEVICE;
+ }
+
+ dev->data = device;
+
+ return_value = skeleton_be_device_init( device, dev );
+
+ if( return_value != 0 )
+ {
+ goto EXIT_INIT_FAILED;
+ }
+
+ return 0;
+
+ REMOVE:
+
+ device = dev->data;
+
+ skeleton_be_device_exit( device );
+
+ EXIT_INIT_FAILED:
+
+ dev->data = NULL;
+
+ kfree( device );
+
+ EXIT_NO_DEVICE:
+
+ return return_value;
+}
+
+static int skeleton_be_device_probe
+ ( struct xenbus_device * dev, const struct xenbus_device_id * id )
+{
+ return skeleton_be_device_probe_or_remove( dev, 0 );
+}
+
+static int skeleton_be_device_remove( struct xenbus_device * dev )
+{
+ return skeleton_be_device_probe_or_remove( dev, 1 );
+}
+
+static struct xenbus_device_id skeleton_be_device_ids[] =
+{
+ { "skeleton" },
+ { "" }
+};
+
+static struct xenbus_driver skeleton_be_device_driver =
+{
+ .name = "skeleton_be",
+ .owner = THIS_MODULE,
+ .ids = skeleton_be_device_ids,
+ .probe = skeleton_be_device_probe,
+ .remove = skeleton_be_device_remove,
+ .suspend = skeleton_be_device_suspend,
+ .resume = skeleton_be_device_resume,
+};
+
+static int __init skeleton_be_module_init( void )
+{
+ return xenbus_register_driver( &skeleton_be_device_driver );
+}
+
+static void __exit skeleton_be_module_exit( void )
+{
+ xenbus_unregister_driver( &skeleton_be_device_driver );
+}
+
+module_init( skeleton_be_module_init );
+module_exit( skeleton_be_module_exit );
+
+MODULE_LICENSE( "GPL" );
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c
--- /dev/null Wed Nov 2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c Wed Nov 2 12:01:27 2005
@@ -0,0 +1,299 @@
+/*****************************************************************************/
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* This program is free software; you can redistribute it and/or modify it */
+/* under the terms of the GNU General Public License as published by the */
+/* Free Software Foundation; either version 2 of the License, or (at your */
+/* option) any later version. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+#include <linux/module.h>
+#include <asm-xen/xenbus.h>
+#include <asm-xen/xenidc.h>
+
+/* Skeleton driver doesn't actually send any messages or transactions. */
+
+#define SKELETON_FE_INITIATOR_QUOTA 0
+#define SKELETON_FE_INITIATOR_MAXIMUM_BYTE_COUNT 0
+#define SKELETON_FE_TARGET_QUOTA 0
+#define SKELETON_FE_TARGET_MAXIMUM_BYTE_COUNT 0
+
+struct skeleton_fe_device
+{
+ struct xenbus_device * dev;
+ xenidc_address address;
+ long int backend_id;
+ char * backend;
+ xenidc_endpoint endpoint;
+ xenidc_callback * endpoint_disconnect_callback;
+};
+
+static inline struct skeleton_fe_device * skeleton_fe_device_endpoint_to
+ ( xenidc_endpoint * endpoint )
+{
+ return container_of( endpoint, struct skeleton_fe_device, endpoint );
+}
+
+static void skeleton_fe_device_endpoint_connect( xenidc_endpoint * endpoint )
+{
+ /* Between connect and completion of the disconnect callback we are */
+ /* allowed to issue messages and transactions. */
+}
+
+static void skeleton_fe_device_endpoint_disconnect
+ ( xenidc_endpoint * endpoint, xenidc_callback * callback )
+{
+ /* We must stop issuing messages and transactions and complete the */
+ /* callback once all of the messages and transactions we are issuing */
+ /* have completed or failed back to us. */
+
+ /* The skeleton driver doesn't issue any messages or transactions so can */
+ /* complete the callback immediately. */
+
+ xenidc_callback_success( callback );
+}
+
+static void skeleton_fe_device_endpoint_message
+ ( xenidc_endpoint * endpoint, xenidc_endpoint_message * message )
+{
+ /* Handle inbound message. */
+
+ xenidc_callback_success( xenidc_endpoint_message_to_callback( message ) );
+}
+
+static void skeleton_fe_device_endpoint_transaction
+ ( xenidc_endpoint * endpoint, xenidc_endpoint_transaction * transaction )
+{
+ /* Handle inbound transaction. */
+
+ xenidc_callback_success
+ ( xenidc_endpoint_transaction_to_callback( transaction ) );
+}
+
+static int skeleton_fe_device_suspend( struct xenbus_device * dev )
+{
+ struct skeleton_fe_device * device = dev->data;
+
+ xenidc_endpoint_destroy( &device->endpoint );
+
+ kfree( device->backend );
+
+ return 0;
+}
+
+static int skeleton_fe_device_resume( struct xenbus_device * dev )
+{
+ struct skeleton_fe_device * device = dev->data;
+
+ int return_value = xenbus_gather
+ (
+ NULL, /* FIXME? */
+ dev->nodename,
+ "backend-id", "%li", &device->backend_id,
+ "backend", NULL, &device->backend,
+ NULL
+ );
+
+ if( XENBUS_EXIST_ERR( return_value ) )
+ {
+ goto EXIT_NO_BACKEND;
+ }
+
+ if( return_value < 0 )
+ {
+ xenbus_dev_error
+ (
+ dev,
+ return_value,
+ "reading %s/backend",
+ dev->nodename
+ );
+
+ goto EXIT_NO_BACKEND;
+ }
+
+ xenidc_address_init
+ (
+ &device->address,
+ dev->nodename,
+ device->backend,
+ device->backend_id
+ );
+
+ xenidc_endpoint_create( &device->endpoint, device->address );
+
+ return 0;
+
+ EXIT_NO_BACKEND:
+
+ return return_value;
+}
+
+static int skeleton_fe_device_init_or_exit
+ ( struct skeleton_fe_device * device, struct xenbus_device * dev, int exit )
+{
+ int return_value = 0;
+
+ if( exit )
+ {
+ goto EXIT;
+ }
+
+ memset( device, 0, sizeof( *device ) );
+
+ device->dev = dev;
+
+ return_value = xenidc_endpoint_init
+ (
+ &device->endpoint,
+ skeleton_fe_device_endpoint_connect,
+ skeleton_fe_device_endpoint_message,
+ skeleton_fe_device_endpoint_transaction,
+ skeleton_fe_device_endpoint_disconnect,
+ SKELETON_FE_INITIATOR_QUOTA,
+ SKELETON_FE_INITIATOR_MAXIMUM_BYTE_COUNT,
+ SKELETON_FE_TARGET_QUOTA,
+ SKELETON_FE_TARGET_MAXIMUM_BYTE_COUNT
+ );
+
+ if( return_value != 0 )
+ {
+ goto EXIT_NO_ENDPOINT;
+ }
+
+ return_value = skeleton_fe_device_resume( dev );
+
+ if( return_value != 0 )
+ {
+ goto EXIT_NO_RESUME;
+ }
+
+ return 0;
+
+ EXIT:
+
+ skeleton_fe_device_suspend( device->dev );
+
+ EXIT_NO_RESUME:
+
+ xenidc_endpoint_exit( &device->endpoint );
+
+ EXIT_NO_ENDPOINT:
+
+ return return_value;
+}
+
+static int skeleton_fe_device_init
+ ( struct skeleton_fe_device * device, struct xenbus_device * dev )
+{
+ return skeleton_fe_device_init_or_exit( device, dev, 0 );
+}
+
+static void skeleton_fe_device_exit( struct skeleton_fe_device * device )
+{
+ (void)skeleton_fe_device_init_or_exit( device, NULL, 1 );
+}
+
+static int skeleton_fe_device_probe_or_remove
+ ( struct xenbus_device * dev, int remove )
+{
+ int return_value = 0;
+
+ struct skeleton_fe_device * device;
+
+ if( remove )
+ {
+ goto REMOVE;
+ }
+
+ device = kmalloc( sizeof( *device ), GFP_KERNEL );
+
+ if( device == NULL )
+ {
+ xenbus_dev_error( dev, -ENOMEM, "allocating frontend structure" );
+
+ return_value = -ENOMEM;
+
+ goto EXIT_NO_DEVICE;
+ }
+
+ dev->data = device;
+
+ return_value = skeleton_fe_device_init( device, dev );
+
+ if( return_value != 0 )
+ {
+ goto EXIT_INIT_FAILED;
+ }
+
+ return 0;
+
+ REMOVE:
+
+ device = dev->data;
+
+ skeleton_fe_device_exit( device );
+
+ EXIT_INIT_FAILED:
+
+ dev->data = NULL;
+
+ kfree( device );
+
+ EXIT_NO_DEVICE:
+
+ return return_value;
+}
+
+static int skeleton_fe_device_probe
+ ( struct xenbus_device * dev, const struct xenbus_device_id * id )
+{
+ return skeleton_fe_device_probe_or_remove( dev, 0 );
+}
+
+static int skeleton_fe_device_remove( struct xenbus_device * dev )
+{
+ return skeleton_fe_device_probe_or_remove( dev, 1 );
+}
+
+static struct xenbus_device_id skeleton_fe_device_ids[] =
+{
+ { "skeleton" },
+ { "" }
+};
+
+static struct xenbus_driver skeleton_fe_device_driver =
+{
+ .name = "skeleton_fe",
+ .owner = THIS_MODULE,
+ .ids = skeleton_fe_device_ids,
+ .probe = skeleton_fe_device_probe,
+ .remove = skeleton_fe_device_remove,
+ .suspend = skeleton_fe_device_suspend,
+ .resume = skeleton_fe_device_resume,
+};
+
+static int __init skeleton_fe_module_init( void )
+{
+ return xenbus_register_driver( &skeleton_fe_device_driver );
+}
+
+static void __exit skeleton_fe_module_exit( void )
+{
+ xenbus_unregister_driver( &skeleton_fe_device_driver );
+}
+
+module_init( skeleton_fe_module_init );
+module_exit( skeleton_fe_module_exit );
+
+MODULE_LICENSE( "GPL" );
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] skeleton frontend/backend examples and a deadlock
2005-11-02 12:37 ` Harry Butterworth
@ 2005-11-03 1:36 ` Rusty Russell
2005-11-03 2:17 ` Mark Williamson
1 sibling, 0 replies; 7+ messages in thread
From: Rusty Russell @ 2005-11-03 1:36 UTC (permalink / raw)
To: Harry Butterworth; +Cc: Xen Mailing List, mark.williamson
On Wed, 2005-11-02 at 12:37 +0000, Harry Butterworth wrote:
> On Wed, 2005-11-02 at 16:22 +1100, Rusty Russell wrote:
> > Here are example frontend and backend driver skeletons. They're
> > *designed* to handle driver restart and module unloading. However,
> > device_unregister deadlocks. I guess noone tried testing
> > unregister_xenbus_watch when not called from a watch callback.
>
> Thanks, that'll be useful for me to know when it comes to testing my
> code.
>
> For comparison, I've knocked up a skeleton FE/BE driver based on the
> xenidc API that I'm using for my USB driver.
>
> You'll see that my patch is smaller, simpler and provides significantly
> more functionality: enough to send messages and transactions
> bi-directionally between the FE and BE. For a fair comparison, yours
> would require interrupt handlers, use of the RING macros, correct memory
> barriers etc.
Agreed! These drivers are a clear demonstration that the current xenbus
device API is too low-level.
More obvious than putting a layer on top of the xenbus API was to change
the xenbus API to be more convenient. But when I tried to rewrite the
xenbus API, I realised I would have had to rewrite every driver, and it
doesn't get much nicer anyway.
A connection-oriented API seems an obviously-good idea to me.
Rusty.
--
A bad analogy is like a leaky screwdriver -- Richard Braakman
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] skeleton frontend/backend examples and a deadlock
2005-11-02 12:37 ` Harry Butterworth
2005-11-03 1:36 ` Rusty Russell
@ 2005-11-03 2:17 ` Mark Williamson
1 sibling, 0 replies; 7+ messages in thread
From: Mark Williamson @ 2005-11-03 2:17 UTC (permalink / raw)
To: Harry Butterworth; +Cc: Rusty Russell, Xen Mailing List
A few random questions:
* Does XenIDC have any performance impact?
* Can it be compatible with the current ring interface, or does it imply
incompatibility with the existing scheme? (i.e. is it an "all or nothing"
patch?)
* Will it be able to leverage page transfers?
Cheers,
Mark
On Wednesday 02 November 2005 12:37, Harry Butterworth wrote:
> On Wed, 2005-11-02 at 16:22 +1100, Rusty Russell wrote:
> > Here are example frontend and backend driver skeletons. They're
> > *designed* to handle driver restart and module unloading. However,
> > device_unregister deadlocks. I guess noone tried testing
> > unregister_xenbus_watch when not called from a watch callback.
>
> Thanks, that'll be useful for me to know when it comes to testing my
> code.
>
> For comparison, I've knocked up a skeleton FE/BE driver based on the
> xenidc API that I'm using for my USB driver.
>
> You'll see that my patch is smaller, simpler and provides significantly
> more functionality: enough to send messages and transactions
> bi-directionally between the FE and BE. For a fair comparison, yours
> would require interrupt handlers, use of the RING macros, correct memory
> barriers etc.
>
> Hopefully this helps to explain why I've split out the IDC functionality
> from my USB driver into a generic service.
>
> Harry.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] skeleton frontend/backend examples and a deadlock
2005-11-02 5:22 [PATCH] skeleton frontend/backend examples and a deadlock Rusty Russell
2005-11-02 12:37 ` Harry Butterworth
@ 2005-11-02 12:52 ` Mark Ryden
2005-11-03 1:23 ` Rusty Russell
2005-11-15 0:16 ` Ewan Mellor
2 siblings, 1 reply; 7+ messages in thread
From: Mark Ryden @ 2005-11-02 12:52 UTC (permalink / raw)
To: Rusty Russell; +Cc: Xen Mailing List
Hello,
One little thing:
If I am not wrong, xenbus_register_backend() is
not exported, and it seems to me that we
should have EXPORT_SYMBOL(xenbus_register_backend) so
it will build.
-- MR
On 11/2/05, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Here are example frontend and backend driver skeletons. They're
> *designed* to handle driver restart and module unloading. However,
> device_unregister deadlocks. I guess noone tried testing
> unregister_xenbus_watch when not called from a watch callback.
>
> The unregistration code in the skeleton driver calls
> unregister_xenbus_watch, which deadlocks on the xenwatch_mutex (against
> dev_changed->device_find->bus_for_each_dev). Bring up a skeleton
> device, then modprobe -r skeleton_fe to see the deadlock.
>
> Here is the helper script which creates the skeleton device:
> #! /bin/sh
>
> if [ $# -ne 2 ]; then
> echo Usage: $0 frontend backend
> exit 1
> fi
>
> xenstore-write /local/domain/$1/device/skeleton/100/backend /local/domain/$2/backend/skeleton/$1/100 /local/domain/$1/device/skeleton/100/backend-id $2 /local/domain/$2/backend/skeleton/$1/100/frontend /local/domain/$1/device/skeleton/100 /local/domain/$2/backend/skeleton/$1/100/frontend-id $1
> # hotplug scripts in backend would normally do this
> xenstore-write /local/domain/$2/backend/skeleton/$1/100/config 777
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/arch/xen/Kconfig
> --- a/linux-2.6-xen-sparse/arch/xen/Kconfig Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/arch/xen/Kconfig Wed Nov 2 15:24:59 2005
> @@ -155,6 +155,22 @@
> If security is not a concern then you may increase performance by
> saying N.
>
> +config XEN_SKELETON_FE
> + tristate "Compile Xen skeleton example frontend driver code"
> + default m
> + help
> + There is an example skeleton driver frontend in drivers/xen/skeleton
> + which you can use as a basis for your own xenbus-aware
> + drivers.
> +
> +config XEN_SKELETON_BE
> + tristate "Compile Xen skeleton example backend driver code"
> + default m
> + help
> + There is an example skeleton driver backend in drivers/xen/skeleton
> + which you can use as a basis for your own xenbus-aware
> + drivers.
> +
> choice
> prompt "Processor Type"
> default XEN_X86
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/drivers/xen/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/Makefile Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/drivers/xen/Makefile Wed Nov 2 15:24:59 2005
> @@ -6,6 +6,7 @@
> obj-y += balloon/
> obj-y += privcmd/
> obj-y += xenbus/
> +obj-y += skeleton/
>
> obj-$(CONFIG_XEN_BLKDEV_BACKEND) += blkback/
> obj-$(CONFIG_XEN_NETDEV_BACKEND) += netback/
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/drivers/xen/skeleton/Makefile
> --- /dev/null Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/Makefile Wed Nov 2 15:24:59 2005
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_XEN_SKELETON_FE) += skeleton_fe.o
> +obj-$(CONFIG_XEN_SKELETON_BE) += skeleton_be.o
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c
> --- /dev/null Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c Wed Nov 2 15:24:59 2005
> @@ -0,0 +1,443 @@
> +/* Example backend driver which simply shares a page with the front end.
> + Copyright (C) 2005 Rusty Russell, IBM Corporation
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +*/
> +
> +/* The "skeleton" device is an example. The store layout looks like:
> + * In the frontend directory:
> + * NAME CREATOR PURPOSE
> + * backend xend Path to backend to get backend data
> + * backend-id xend Domain ID to give evtchn/grantrefs
> + * ring-reference frontend Tells backend about shared page
> + * event-channel frontend Tells backend about event channel
> + *
> + * In the backend directory:
> + * NAME CREATOR PURPOSE
> + * frontend xend Path to frontend to get frontend data
> + * frontend-id xend ID to accept evtchn/grantrefs from
> + * config xend/hotplug Configuration info for backend.
> + * stuff backend Tells frontend about, um, useful stuff
> + *
> + * As the frontend can be saved/restored, it must handle the "backend" fields
> + * changing (after the ->resume callback). As either end's driver could go
> + * away (module unload), both frontend and backend must handle the dynamic
> + * fields (ring-reference & event-channel, or stuff) vanishing, and appearing.
> + */
> +
> +#include <linux/stringify.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <asm-xen/xenbus.h>
> +#include <asm-xen/evtchn.h>
> +#include <asm-xen/driver_util.h>
> +#include <asm-xen/gnttab.h>
> +
> +/* WAITING: our directory exists, fields aren't there yet.
> + * EXISTS: the tools/udev has written fields we need.
> + * READY: we have written our info into the store for the frontend to read.
> + * We can only enter this state once frontend is not "connected", to
> + * cover the case of module reload (frontend might not have noticed us
> + * going away yet).
> + * CONNECTED: we have read frontend information from the store. We create the
> + * "connected" node.
> + */
> +enum state
> +{
> + WAITING,
> + EXISTS,
> + READY,
> + CONNECTED,
> +};
> +
> +/* Private information about this device */
> +struct skeleton_be_info
> +{
> + /* xenbus device we belong to */
> + struct xenbus_device *dev;
> +
> + /* frontend path */
> + char *frontend;
> +
> + /* frontend id */
> + int frontend_id;
> +
> + /* Mapping for frontend page */
> + struct vm_struct *vm;
> + u16 shmem_handle;
> +
> + /* grant table reference to page frontend offered */
> + int ring_ref;
> +
> + /* event channel to send interrupts to frontend */
> + int evtchn;
> + int fe_evtchn;
> +
> + /* Watch we place on frontend */
> + struct xenbus_watch watch;
> +
> + /* Watch we place on ourselves. */
> + struct xenbus_watch be_watch;
> +
> + /* If we are fully connected to backend. */
> + enum state state;
> +
> + /* Device-specific (eg. net, block) stuff. */
> + struct device_specific_info *info;
> +};
> +
> +static const char *state(struct skeleton_be_info *info)
> +{
> + return info->state == WAITING ? "WAITING" :
> + info->state == EXISTS ? "EXISTS" :
> + info->state == READY ? "READY" :
> + info->state == CONNECTED ? "CONNECTED" : "UNKNOWN";
> +}
> +
> +static inline int bind_event_channel(domid_t id, int evtchn)
> +{
> + int err;
> + evtchn_op_t op = {
> + .cmd = EVTCHNOP_bind_interdomain,
> + .u.bind_interdomain.remote_dom = id,
> + .u.bind_interdomain.remote_port = evtchn };
> + err = HYPERVISOR_event_channel_op(&op);
> + if (err)
> + return err;
> + return op.u.bind_interdomain.local_port;
> +}
> +
> +static struct device_specific_info *
> +setup_device_specific_crap(struct xenbus_device *dev)
> +{
> + int ret, dummy;
> +
> + /* Read any local info set up by tools or hotplug/udev
> + * (eg. device to serve) */
> + ret = xenbus_scanf(NULL, dev->nodename, "config", "%i", &dummy);
> + if (ret != 1)
> + return NULL;
> +
> + /* Request net/block/usb/etc device from kernel. */
> + return (void *)1;
> +}
> +
> +static void free_device_specific_crap(struct device_specific_info *crap)
> +{
> + /* Release net/block/usb/etc device from kernel. */
> +}
> +
> +static void stop_device_replies(struct device_specific_info *crap)
> +{
> + /* Frontend has gone away, we should drop outstanding replies. */
> +}
> +
> +/* Write the information out to the store for the frontend to read, and
> + * know we're ready. */
> +static int publish_info(struct skeleton_be_info *info)
> +{
> + return xenbus_printf(NULL, info->dev->nodename, "stuff", "%u", 7);
> +}
> +
> +/* Frontend gone/going away. Clean up. */
> +static void skeleton_stop(struct skeleton_be_info *info)
> +{
> + printk("%s: state %s\n", __func__, state(info));
> + stop_device_replies(info->info);
> +
> + xenbus_rm(NULL, info->dev->nodename, "stuff");
> +}
> +
> +static struct vm_struct *map_page(int ref, domid_t id, u16 *handle)
> +{
> + struct gnttab_map_grant_ref op;
> + struct vm_struct *vm;
> +
> + vm = alloc_vm_area(PAGE_SIZE);
> + if (!vm)
> + return ERR_PTR(-ENOMEM);
> +
> + op.host_addr = (unsigned long)vm->addr;
> + op.flags = GNTMAP_host_map;
> + op.ref = ref;
> + op.dom = id;
> +
> + lock_vm_area(vm);
> + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1));
> + unlock_vm_area(vm);
> +
> + if (op.handle < 0) {
> + free_vm_area(vm);
> + return ERR_PTR(op.handle);
> + }
> +
> + *handle = op.handle;
> + return vm;
> +}
> +
> +static void unmap_page(struct vm_struct *vm, u16 handle)
> +{
> + struct gnttab_unmap_grant_ref op;
> +
> + printk("%s enter\n", __func__);
> + op.host_addr = (unsigned long)vm->addr;
> + op.handle = handle;
> + op.dev_bus_addr = 0;
> +
> + lock_vm_area(vm);
> + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1));
> + unlock_vm_area(vm);
> + printk("%s exit\n", __func__);
> +}
> +
> +/* FIXME: This can be called before skeleton_probe() finished. */
> +static void frontend_changed(struct xenbus_watch *watch,
> + const char **vec, unsigned int len)
> +{
> + struct skeleton_be_info *info;
> + struct xenbus_transaction *trans;
> + int err;
> +
> + info = container_of(watch, struct skeleton_be_info, watch);
> +
> + /* If frontend has gone away, shut down. */
> + if (!xenbus_exists(NULL, info->frontend, "")) {
> + device_unregister(&info->dev->dev);
> + return;
> + }
> +
> + switch (info->state) {
> + case WAITING:
> + break;
> +
> + case EXISTS:
> + if (xenbus_exists(NULL, info->frontend, "connected")) {
> + xenbus_dev_error(info->dev, -EBUSY,
> + "frontend still connected");
> + return;
> + }
> +
> + err = publish_info(info);
> + if (err) {
> + xenbus_dev_error(info->dev, err,
> + "writing information");
> + return;
> + }
> + info->state = READY;
> + /* fall thru */
> +
> + case READY:
> + /* Try to read frontend stuff. */
> + again:
> + trans = xenbus_transaction_start();
> + if (IS_ERR(trans)) {
> + xenbus_dev_error(info->dev, PTR_ERR(trans),
> + "starting transaction");
> + return;
> + }
> + err = xenbus_gather(NULL, info->frontend,
> + "ring-reference", "%u", &info->ring_ref,
> + "event-channel", "%u", &info->fe_evtchn,
> + NULL);
> + if (!err) {
> + err = xenbus_transaction_end(trans, 0);
> + if (err == -EAGAIN)
> + goto again;
> + }
> + if (err) {
> + xenbus_dev_error(info->dev, err, "reading from %s",
> + info->frontend);
> + return;
> + }
> +
> + err = bind_event_channel(info->frontend_id, info->fe_evtchn);
> + if (err < 0) {
> + xenbus_dev_error(info->dev, err,
> + "binding event channel");
> + return;
> + }
> + info->fe_evtchn = err;
> +
> + info->vm = map_page(info->ring_ref, info->frontend_id,
> + &info->shmem_handle);
> + if (IS_ERR(info->vm)) {
> + xenbus_dev_error(info->dev, PTR_ERR(info->vm),
> + "mapping page");
> + return;
> + }
> + info->state = CONNECTED;
> + /* Clear any previous errors. */
> + xenbus_dev_ok(info->dev);
> + xenbus_printf(NULL, info->dev->nodename, "connected", "ok");
> + break;
> +
> + case CONNECTED:
> + /* Did frontend driver shut down? */
> + if (!xenbus_exists(NULL, info->frontend, "ring-reference")) {
> + xenbus_dev_error(info->dev, -ENOENT,
> + "frontend disconnected");
> + xenbus_rm(NULL, info->dev->nodename, "connected");
> + unmap_page(info->vm, info->shmem_handle);
> + skeleton_stop(info);
> + info->state = EXISTS;
> + }
> + }
> +}
> +
> +static int skeleton_watch_front(struct xenbus_device *dev,
> + struct skeleton_be_info *info)
> +{
> + int err;
> +
> + printk("%s: state %s\n", __func__, state(info));
> +
> + /* We need frontend-id and path. */
> + err = xenbus_gather(NULL, dev->nodename,
> + "frontend-id", "%i", &info->frontend_id,
> + "frontend", NULL, &info->frontend,
> + NULL);
> + if (err) {
> + xenbus_dev_error(dev, err, "reading frontend or frontend-id");
> + goto out;
> + }
> +
> + info->watch.node = info->frontend;
> + info->watch.callback = frontend_changed;
> + err = register_xenbus_watch(&info->watch);
> + if (err) {
> + xenbus_dev_error(dev, err, "placing watch on %s",
> + info->frontend);
> + goto free_frontend;
> + }
> + /* frontend_changed called immediately: stuff might be there already.*/
> + frontend_changed(&info->watch, NULL, 0);
> + return 0;
> +
> +free_frontend:
> + kfree(info->frontend);
> +out:
> + return err;
> +}
> +
> +static void self_changed(struct xenbus_watch *watch,
> + const char **vec, unsigned int len)
> +{
> + struct skeleton_be_info *info;
> + int err;
> +
> + info = container_of(watch, struct skeleton_be_info, be_watch);
> + printk("%s: state %s\n", __func__, state(info));
> +
> + /* We only need this while we're waiting for config. */
> + if (info->state != WAITING)
> + return;
> +
> + /* Not there yet? Keep waiting. */
> + info->info = setup_device_specific_crap(info->dev);
> + if (!info->info)
> + return;
> +
> + info->state = EXISTS;
> + err = skeleton_watch_front(info->dev, info);
> + if (err) {
> + free_device_specific_crap(info->info);
> + return;
> + }
> +}
> +
> +static int skeleton_probe(struct xenbus_device *dev,
> + const struct xenbus_device_id *id)
> +{
> + int err;
> + struct skeleton_be_info *info;
> +
> + printk("skeleton_probe_be: %s\n", dev->nodename);
> +
> + dev->data = info = kmalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + xenbus_dev_error(dev, -ENOMEM, "allocating info structure");
> + return -ENOMEM;
> + }
> + info->dev = dev;
> + info->state = WAITING;
> +
> + /* Try for config (might need to wait for udev). */
> + info->be_watch.node = dev->nodename;
> + info->be_watch.callback = self_changed;
> + err = register_xenbus_watch(&info->be_watch);
> + if (err) {
> + xenbus_dev_error(dev, err, "placing watch on self %s",
> + dev->nodename);
> + kfree(info);
> + return err;
> + }
> + self_changed(&info->be_watch, NULL, 0);
> + return 0;
> +}
> +
> +static int skeleton_remove(struct xenbus_device *dev)
> +{
> + struct skeleton_be_info *info = dev->data;
> +
> + switch (info->state) {
> + case CONNECTED:
> + unmap_page(info->vm, info->shmem_handle);
> + /* fall thru */
> + case READY:
> + skeleton_stop(info);
> + /* Must remove this after other fields. */
> + xenbus_rm(NULL, dev->nodename, "connected");
> + /* fall thru */
> + case EXISTS:
> + unregister_xenbus_watch(&info->watch);
> + free_device_specific_crap(info->info);
> + /* fall thru */
> + case WAITING:
> + unregister_xenbus_watch(&info->be_watch);
> + }
> +
> + kfree(info);
> +
> + return 0;
> +}
> +
> +static struct xenbus_device_id skeleton_ids[] = {
> + { "skeleton" },
> + { { 0 } },
> +};
> +
> +/* A xenbus backend driver. */
> +static struct xenbus_driver driver = {
> + /* Makefile defines KBUILD_MODNAME (in this case, skeleton_be) */
> + .name = __stringify(KBUILD_MODNAME),
> + .owner = THIS_MODULE,
> + .ids = skeleton_ids,
> + .probe = skeleton_probe,
> + .remove = skeleton_remove,
> +};
> +
> +static int init(void)
> +{
> + return xenbus_register_backend(&driver);
> +}
> +
> +static void fini(void)
> +{
> + xenbus_unregister_driver(&driver);
> +}
> +
> +module_init(init);
> +module_exit(fini);
> +MODULE_LICENSE("GPL");
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c
> --- /dev/null Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c Wed Nov 2 15:24:59 2005
> @@ -0,0 +1,407 @@
> +/* Example frontend driver which simply shares a page with the back end.
> + Copyright (C) 2005 Rusty Russell, IBM Corporation
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +*/
> +
> +/* The "skeleton" device is an example. The store layout looks like:
> + * In the frontend directory:
> + * NAME CREATOR PURPOSE
> + * backend xend Path to backend to get backend data
> + * backend-id xend Domain ID to give evtchn/grantrefs
> + * ring-reference frontend Tells backend about shared page
> + * event-channel frontend Tells backend about event channel
> + *
> + * In the backend directory:
> + * NAME CREATOR PURPOSE
> + * frontend xend Path to frontend to get frontend data
> + * frontend-id xend ID to accept evtchn/grantrefs from
> + * config xend/hotplug Configuration info for backend.
> + * stuff backend Tells frontend about, um, useful stuff
> + *
> + * As the frontend can be saved/restored, it must handle the "backend" fields
> + * changing (after the ->resume callback). As either end's driver could go
> + * away (module unload), both frontend and backend must handle the dynamic
> + * fields (ring-reference & event-channel, or stuff) vanishing, and appearing.
> + */
> +
> +#include <linux/stringify.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <asm-xen/xenbus.h>
> +#include <asm-xen/evtchn.h>
> +#include <asm-xen/gnttab.h>
> +
> +/* EXISTS: our directory exists, with the tool-written stuff in it.
> + * READY: we have written our info into the store for the backend to read.
> + * We can only enter this state once backend is not "connected", to
> + * cover the case of module reload (backend might not have noticed us
> + * going away yet!).
> + * CONNECTED: we have read backend information from the store. We create the
> + * "connected" node.
> + */
> +enum state
> +{
> + EXISTS,
> + READY,
> + CONNECTED,
> +};
> +
> +/* Private information about this device */
> +struct skeleton_info
> +{
> + /* xenbus device we belong to */
> + struct xenbus_device *dev;
> +
> + /* backend path */
> + char *backend;
> +
> + /* backend id */
> + int backend_id;
> +
> + /* page we offer to share */
> + void *page;
> +
> + /* grant table reference to page we offer to backend */
> + int ring_ref;
> +
> + /* event channel to send interrupts to backend */
> + int evtchn;
> +
> + /* Watch we place on backend */
> + struct xenbus_watch watch;
> +
> + /* If we are fully connected to backend. */
> + enum state state;
> +
> + /* Information given by the backend */
> + int backend_stuff;
> +
> + /* Device-specific (eg. net, block) stuff. */
> + struct device_specific_info *info;
> +};
> +
> +static const char *state(struct skeleton_info *info)
> +{
> + return info->state == EXISTS ? "EXISTS" :
> + info->state == READY ? "READY" :
> + info->state == CONNECTED ? "CONNECTED" : "UNKNOWN";
> +}
> +
> +static inline int allocate_event_channel(domid_t id)
> +{
> + int err;
> + evtchn_op_t op = {
> + .cmd = EVTCHNOP_alloc_unbound,
> + .u.alloc_unbound.dom = DOMID_SELF,
> + .u.alloc_unbound.remote_dom = id };
> +
> + err = HYPERVISOR_event_channel_op(&op);
> + if (err)
> + return err;
> + return op.u.alloc_unbound.port;
> +}
> +
> +static inline void free_event_channel(int evtchn)
> +{
> + evtchn_op_t op = {
> + .cmd = EVTCHNOP_close,
> + .u.close.port = evtchn };
> + HYPERVISOR_event_channel_op(&op);
> +}
> +
> +static struct device_specific_info *
> +setup_device_specific_crap(struct xenbus_device *dev)
> +{
> + /* Read any local info set up by tools (eg. mac address) on creation */
> +
> + /* Register as a net/block/usb/etc device with kernel. */
> +
> + return (void *)1;
> +}
> +
> +static void free_device_specific_crap(struct device_specific_info *crap)
> +{
> + /* Unregister as a net/block/usb/etc device with kernel. */
> +}
> +
> +static void stop_device_requests(struct device_specific_info *crap)
> +{
> + /* Backend has gone away, we should queue requests. */
> +}
> +
> +/* Write the information out to the store for the backend to read, and
> + * know we're ready. */
> +static int publish_info(struct skeleton_info *info)
> +{
> + struct xenbus_transaction *trans;
> + int err;
> +
> + printk("%s: state %s\n", __func__, state(info));
> + /* Transactions can fail spuriously, which means we loop. */
> +again:
> + trans = xenbus_transaction_start();
> + if (IS_ERR(trans))
> + return PTR_ERR(trans);
> +
> + err = xenbus_printf(trans, info->dev->nodename, "ring-reference", "%u",
> + info->ring_ref);
> + if (!err)
> + err = xenbus_printf(trans, info->dev->nodename,
> + "event-channel", "%u", info->evtchn);
> +
> + if (err) {
> + xenbus_transaction_end(trans, 1);
> + return err;
> + }
> + err = xenbus_transaction_end(trans, 0);
> + if (err == -EAGAIN)
> + goto again;
> + return err;
> +}
> +
> +/* Backend gone/going away. Clean up. */
> +static void skeleton_stop(struct skeleton_info *info)
> +{
> + printk("%s: state %s\n", __func__, state(info));
> + stop_device_requests(info->info);
> +
> + /* FIXME: can't use transaction, it requires alloc. */
> + xenbus_rm(NULL, info->dev->nodename, "ring-reference");
> + xenbus_rm(NULL, info->dev->nodename, "event-channel");
> +}
> +
> +/* FIXME: This can be called before skeleton_probe() finished. */
> +static void backend_changed(struct xenbus_watch *watch,
> + const char **vec, unsigned int len)
> +{
> + struct skeleton_info *info;
> + int err;
> +
> + info = container_of(watch, struct skeleton_info, watch);
> +
> + printk("%s: state %s\n", __func__, state(info));
> +
> + switch (info->state) {
> + case EXISTS:
> + if (xenbus_exists(NULL, info->backend, "connected")) {
> + xenbus_dev_error(info->dev, -EBUSY,
> + "backend still connected");
> + return;
> + }
> +
> + err = publish_info(info);
> + if (err) {
> + xenbus_dev_error(info->dev, err,
> + "writing information");
> + return;
> + }
> + info->state = READY;
> + /* fall thru */
> +
> + case READY:
> + /* Try to read backend stuff. */
> + err = xenbus_scanf(NULL, info->backend, "stuff", "%u",
> + &info->backend_stuff);
> + if (err < 0) {
> + xenbus_dev_error(info->dev, err, "reading %s/stuff",
> + info->backend);
> + return;
> + }
> + info->state = CONNECTED;
> + /* Clear any previous errors. */
> + xenbus_dev_ok(info->dev);
> + xenbus_printf(NULL, info->dev->nodename, "connected", "ok");
> + break;
> +
> + case CONNECTED:
> + /* Did backend driver shut down? */
> + if (!xenbus_exists(NULL, info->backend, "stuff")) {
> + xenbus_dev_error(info->dev, -ENOENT,
> + "backend disconnected");
> + xenbus_rm(NULL, info->dev->nodename, "connected");
> + skeleton_stop(info);
> + info->state = EXISTS;
> + }
> + }
> +}
> +
> +static int skeleton_resume(struct xenbus_device *dev)
> +{
> + int err;
> + struct skeleton_info *info = dev->data;
> +
> + printk("%s: state %s\n", __func__, state(info));
> +
> + /* We need backend-id and path. */
> + err = xenbus_gather(NULL, dev->nodename,
> + "backend-id", "%i", &info->backend_id,
> + "backend", NULL, &info->backend,
> + NULL);
> + if (err) {
> + xenbus_dev_error(dev, err, "reading backend or backend-id");
> + goto out;
> + }
> +
> + /* We need to allocate a page and event channel. */
> + info->page = (void *)__get_free_page(GFP_KERNEL);
> + if (!info->page) {
> + err = -ENOMEM;
> + xenbus_dev_error(dev, err, "allocating shared page");
> + goto free_backend;
> + }
> +
> + err = gnttab_grant_foreign_access(info->backend_id,
> + virt_to_mfn(info->page), 0);
> + if (err < 0) {
> + xenbus_dev_error(dev, err, "granting page");
> + goto free_page;
> + }
> + info->ring_ref = err;
> +
> + err = allocate_event_channel(info->backend_id);
> + if (err < 0) {
> + xenbus_dev_error(dev, err, "allocating event channel");
> + goto ungrant_page;
> + }
> + info->evtchn = err;
> +
> + info->watch.node = info->backend;
> + info->watch.callback = backend_changed;
> + err = register_xenbus_watch(&info->watch);
> + if (err) {
> + xenbus_dev_error(dev, err, "placing watch on %s", info->backend);
> + goto free_event_channel;
> + }
> + /* backend_changed called immediately: stuff might be there already. */
> + backend_changed(&info->watch, NULL, 0);
> + return 0;
> +
> +free_event_channel:
> + free_event_channel(info->evtchn);
> +ungrant_page:
> + /* FIXME: Need infrastructure to handle otherside holding onto page. */
> + gnttab_end_foreign_access(info->ring_ref, 0);
> +free_page:
> + free_page((unsigned long)info->page);
> +free_backend:
> + kfree(info->backend);
> +out:
> + return err;
> +}
> +
> +static int skeleton_probe(struct xenbus_device *dev,
> + const struct xenbus_device_id *id)
> +{
> + int err;
> + struct skeleton_info *info;
> +
> + printk("skeleton_probe for %s\n", dev->nodename);
> +
> + dev->data = info = kmalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + xenbus_dev_error(dev, -ENOMEM, "allocating info structure");
> + return -ENOMEM;
> + }
> + info->dev = dev;
> + info->state = EXISTS;
> +
> + info->info = setup_device_specific_crap(dev);
> + if (IS_ERR(info->info)) {
> + err = PTR_ERR(info->info);
> + xenbus_dev_error(dev, err, "setting up device");
> + kfree(info);
> + return err;
> + }
> +
> + err = skeleton_resume(dev);
> + if (err) {
> + free_device_specific_crap(info->info);
> + kfree(info);
> + }
> + return err;
> +}
> +
> +/* Clean up: will re-init and connect to backend on resume. */
> +static int skeleton_suspend(struct xenbus_device *dev)
> +{
> + struct skeleton_info *info = dev->data;
> +
> + printk("%s: state %s\n", __func__, state(info));
> + switch (info->state) {
> + case CONNECTED:
> + xenbus_rm(NULL, dev->nodename, "connected");
> + info->state = READY;
> + /* fall thru */
> + case READY:
> + skeleton_stop(info);
> + info->state = EXISTS;
> + case EXISTS:
> + ; /* Nothing to do */
> + }
> +
> + /* FIXME: Need infrastructure to handle otherside holding onto page. */
> + gnttab_end_foreign_access(info->ring_ref, 0);
> + free_page((unsigned long)info->page);
> + kfree(info->backend);
> + printk("%s:%i\n", __func__, __LINE__);
> + unregister_xenbus_watch(&info->watch);
> + printk("%s:%i\n", __func__, __LINE__);
> + return 0;
> +}
> +
> +static int skeleton_remove(struct xenbus_device *dev)
> +{
> + struct skeleton_info *info = dev->data;
> +
> + printk("%s: state %s\n", __func__, state(info));
> + skeleton_suspend(dev);
> + free_device_specific_crap(info->info);
> + kfree(info);
> + printk("%s exiting\n", __func__);
> +
> + return 0;
> +}
> +
> +static struct xenbus_device_id skeleton_ids[] = {
> + { "skeleton" },
> + { { 0 } },
> +};
> +
> +/* A xenbus driver. */
> +static struct xenbus_driver driver = {
> + /* Makefile defines KBUILD_MODNAME (in this case, skeleton_fe) */
> + .name = __stringify(KBUILD_MODNAME),
> + .owner = THIS_MODULE,
> + .ids = skeleton_ids,
> + .probe = skeleton_probe,
> + .remove = skeleton_remove,
> + .suspend = skeleton_suspend,
> + .resume = skeleton_resume,
> +};
> +
> +static int init(void)
> +{
> + return xenbus_register_driver(&driver);
> +}
> +
> +static void fini(void)
> +{
> + xenbus_unregister_driver(&driver);
> +}
> +
> +module_init(init);
> +module_exit(fini);
> +MODULE_LICENSE("GPL");
>
> --
> A bad analogy is like a leaky screwdriver -- Richard Braakman
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] skeleton frontend/backend examples and a deadlock
2005-11-02 12:52 ` Mark Ryden
@ 2005-11-03 1:23 ` Rusty Russell
0 siblings, 0 replies; 7+ messages in thread
From: Rusty Russell @ 2005-11-03 1:23 UTC (permalink / raw)
To: Mark Ryden; +Cc: Xen Mailing List
On Wed, 2005-11-02 at 14:52 +0200, Mark Ryden wrote:
> Hello,
> One little thing:
> If I am not wrong, xenbus_register_backend() is
> not exported, and it seems to me that we
> should have EXPORT_SYMBOL(xenbus_register_backend) so
> it will build.
Indeed, the backend cannot be built as a module because this, and the
vma functions are not exported. The latter can be solved by providing a
nice grant-id -> mapped page function in the core I think (and the unmap
function)
Also, the skeleton was written against a tree where registering a watch
did not always call the callback immediately. Now this has been fixed,
those explicit calls are removed.
Rusty.
--
A bad analogy is like a leaky screwdriver -- Richard Braakman
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] skeleton frontend/backend examples and a deadlock
2005-11-02 5:22 [PATCH] skeleton frontend/backend examples and a deadlock Rusty Russell
2005-11-02 12:37 ` Harry Butterworth
2005-11-02 12:52 ` Mark Ryden
@ 2005-11-15 0:16 ` Ewan Mellor
2 siblings, 0 replies; 7+ messages in thread
From: Ewan Mellor @ 2005-11-15 0:16 UTC (permalink / raw)
To: Rusty Russell; +Cc: Xen Mailing List
On Wed, Nov 02, 2005 at 04:22:44PM +1100, Rusty Russell wrote:
> Here are example frontend and backend driver skeletons.
Rusty, your skeleton driver has suggested a number of new abstractions that we
could make to the driver layer, as I mentioned in my email just previous.
I've taken on board a lot of the code here, the structure, and so forth. I
have taken many of the ideas into the Xenbus layer, where they can be shared
with other front/back split drivers, and there is plenty of renaming to
reflect that fact, but the general concepts are still there.
I wanted to explain what I've taken, where I've put it, and what I've changed,
so here goes.
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c
> --- /dev/null Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c Wed Nov 2 15:24:59 2005
> [Snip header and comments]
> +/* WAITING: our directory exists, fields aren't there yet.
> + * EXISTS: the tools/udev has written fields we need.
> + * READY: we have written our info into the store for the frontend to read.
> + * We can only enter this state once frontend is not "connected", to
> + * cover the case of module reload (frontend might not have noticed us
> + * going away yet).
> + * CONNECTED: we have read frontend information from the store. We create the
> + * "connected" node.
> + */
> +enum state
> +{
> + WAITING,
> + EXISTS,
> + READY,
> + CONNECTED,
> +};
The general concept of an explicit state machine at the driver level is a good
one, and I've taken that on board. For visibility of the process, I've
actually added a state node to the store in the frontend and backend
directories. This node can be watched by the other end, by Xend, or by other
diagnostic tools.
I've changed the states a little:
Initialising
InitWait == WAITING
Initialised == READY
Connected == CONNECTED
Closing
Closed
As you can see, I've dropped the EXISTS state. I prefer to think of this as
separate from the initialisation of the frontend/backend, because the
conversation is with udev / hotplug / whatever, rather than with the frontend.
Since I've moved this handling into the Xenbus layer, I've left handling
EXISTS or similar with the specific driver.
I've added an Initialising state, for clarity, since this is being written in
the store and we expect diagnostic tools to look for it, and a closedown
exchange, which seems sensible to me in general, and is necessary for
hotplugging block devices in particular.
> +/* Private information about this device */
> +struct skeleton_be_info
> +{
> + /* xenbus device we belong to */
> + struct xenbus_device *dev;
> +
> + /* frontend path */
> + char *frontend;
> +
> + /* frontend id */
> + int frontend_id;
These two have moved into xenbus_device, as otherend and otherend_id. This
allows us to share code between the backend and the frontend as they handle
the connection to their peer.
> +
> + /* Mapping for frontend page */
> + struct vm_struct *vm;
> + u16 shmem_handle;
> +
> + /* grant table reference to page frontend offered */
> + int ring_ref;
> +
> + /* event channel to send interrupts to frontend */
> + int evtchn;
> + int fe_evtchn;
This arrangement is driver-specific. For net, we use two channels, one for
tx and one for rx, but for block we only need the one.
> +
> + /* Watch we place on frontend */
> + struct xenbus_watch watch;
Also moved into xenbus_device.
> +
> + /* Watch we place on ourselves. */
> + struct xenbus_watch be_watch;
This stays where it is. Drivers need not use this, although all the ones that
use hotplug do. Commoning this out might be a useful task for the future, but
I'm not sure it's worth the effort.
> + /* If we are fully connected to backend. */
> + enum state state;
> +
> + /* Device-specific (eg. net, block) stuff. */
> + struct device_specific_info *info;
> +};
> +
> [Snip state() function, simple stringifier]
> +static inline int bind_event_channel(domid_t id, int evtchn)
> +{
> + int err;
> + evtchn_op_t op = {
> + .cmd = EVTCHNOP_bind_interdomain,
> + .u.bind_interdomain.remote_dom = id,
> + .u.bind_interdomain.remote_port = evtchn };
> + err = HYPERVISOR_event_channel_op(&op);
> + if (err)
> + return err;
> + return op.u.bind_interdomain.local_port;
> +}
This I've not taken yet, because in the existing net and blk drivers this code
happens inside interface.c, and I haven't yet decided how I'd like to arrange
that. With luck, this will end up looking very much like
xenbus_client.c:xenbus_alloc_evtchn, mentioned below.
> [Snip {setup,free}_device_specific_crap, stop_device_replies]
Of course, the device-specific stuff stays in the individual drivers!
> +/* Write the information out to the store for the frontend to read, and
> + * know we're ready. */
> +static int publish_info(struct skeleton_be_info *info)
> +{
> + return xenbus_printf(NULL, info->dev->nodename, "stuff", "%u", 7);
> +}
The actual writing is driver-specific, of course. One advantage of having an
explicit state node is that the semantics of "let the frontend know we're
ready" are not required, as immediately following this call you change to
state READY, and in my scheme this state change is visible explicitly.
> +/* Frontend gone/going away. Clean up. */
> +static void skeleton_stop(struct skeleton_be_info *info)
> +{
> + printk("%s: state %s\n", __func__, state(info));
> + stop_device_replies(info->info);
> +
> + xenbus_rm(NULL, info->dev->nodename, "stuff");
> +}
When using hotplug, it's necessary to push the rm into those scripts, because
you generally need the information in the store in order to decide how to do
your teardown. Otherwise, this is fine.
> +static struct vm_struct *map_page(int ref, domid_t id, u16 *handle)
> +{
> + struct gnttab_map_grant_ref op;
> + struct vm_struct *vm;
> +
> + vm = alloc_vm_area(PAGE_SIZE);
> + if (!vm)
> + return ERR_PTR(-ENOMEM);
> +
> + op.host_addr = (unsigned long)vm->addr;
> + op.flags = GNTMAP_host_map;
> + op.ref = ref;
> + op.dom = id;
> +
> + lock_vm_area(vm);
> + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1));
> + unlock_vm_area(vm);
> +
> + if (op.handle < 0) {
> + free_vm_area(vm);
> + return ERR_PTR(op.handle);
> + }
> +
> + *handle = op.handle;
> + return vm;
> +}
> +
> +static void unmap_page(struct vm_struct *vm, u16 handle)
> +{
> + struct gnttab_unmap_grant_ref op;
> +
> + printk("%s enter\n", __func__);
> + op.host_addr = (unsigned long)vm->addr;
> + op.handle = handle;
> + op.dev_bus_addr = 0;
> +
> + lock_vm_area(vm);
> + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1));
> + unlock_vm_area(vm);
> + printk("%s exit\n", __func__);
> +}
This abstraction is not perfect, unfortunately, because the net driver uses
two adjacent pages, because it has it's two ring buffers. It would be worth
making it possible for this abstraction to move into xenbus_client, but for
now it's not there.
> +/* FIXME: This can be called before skeleton_probe() finished. */
> +static void frontend_changed(struct xenbus_watch *watch,
> + const char **vec, unsigned int len)
> +{
> + struct skeleton_be_info *info;
> + struct xenbus_transaction *trans;
> + int err;
> +
> + info = container_of(watch, struct skeleton_be_info, watch);
> +
> + /* If frontend has gone away, shut down. */
> + if (!xenbus_exists(NULL, info->frontend, "")) {
> + device_unregister(&info->dev->dev);
> + return;
> + }
> +
> + switch (info->state) {
> + case WAITING:
> + break;
> +
> + case EXISTS:
> + if (xenbus_exists(NULL, info->frontend, "connected")) {
> + xenbus_dev_error(info->dev, -EBUSY,
> + "frontend still connected");
> + return;
> + }
> +
> + err = publish_info(info);
> + if (err) {
> + xenbus_dev_error(info->dev, err,
> + "writing information");
> + return;
> + }
> + info->state = READY;
> + /* fall thru */
This is publishing driver-specific stuff.
> + case READY:
> + /* Try to read frontend stuff. */
> + again:
> + trans = xenbus_transaction_start();
> + if (IS_ERR(trans)) {
> + xenbus_dev_error(info->dev, PTR_ERR(trans),
> + "starting transaction");
> + return;
> + }
> + err = xenbus_gather(NULL, info->frontend,
> + "ring-reference", "%u", &info->ring_ref,
> + "event-channel", "%u", &info->fe_evtchn,
> + NULL);
> + if (!err) {
> + err = xenbus_transaction_end(trans, 0);
> + if (err == -EAGAIN)
> + goto again;
> + }
> + if (err) {
> + xenbus_dev_error(info->dev, err, "reading from %s",
> + info->frontend);
> + return;
> + }
> +
> + err = bind_event_channel(info->frontend_id, info->fe_evtchn);
> + if (err < 0) {
> + xenbus_dev_error(info->dev, err,
> + "binding event channel");
> + return;
> + }
> + info->fe_evtchn = err;
> +
> + info->vm = map_page(info->ring_ref, info->frontend_id,
> + &info->shmem_handle);
> + if (IS_ERR(info->vm)) {
> + xenbus_dev_error(info->dev, PTR_ERR(info->vm),
> + "mapping page");
> + return;
> + }
> + info->state = CONNECTED;
This is reading driver-specific connection details from the frontend.
> + /* Clear any previous errors. */
> + xenbus_dev_ok(info->dev);
> + xenbus_printf(NULL, info->dev->nodename, "connected", "ok");
> + break;
> +
> + case CONNECTED:
> + /* Did frontend driver shut down? */
> + if (!xenbus_exists(NULL, info->frontend, "ring-reference")) {
> + xenbus_dev_error(info->dev, -ENOENT,
> + "frontend disconnected");
> + xenbus_rm(NULL, info->dev->nodename, "connected");
> + unmap_page(info->vm, info->shmem_handle);
> + skeleton_stop(info);
> + info->state = EXISTS;
And this is pretty standard closedown stuff.
> + }
> + }
> +}
> +static int skeleton_watch_front(struct xenbus_device *dev,
> + struct skeleton_be_info *info)
> +{
> + int err;
> +
> + printk("%s: state %s\n", __func__, state(info));
> +
> + /* We need frontend-id and path. */
> + err = xenbus_gather(NULL, dev->nodename,
> + "frontend-id", "%i", &info->frontend_id,
> + "frontend", NULL, &info->frontend,
> + NULL);
> + if (err) {
> + xenbus_dev_error(dev, err, "reading frontend or frontend-id");
> + goto out;
> + }
> +
> + info->watch.node = info->frontend;
> + info->watch.callback = frontend_changed;
> + err = register_xenbus_watch(&info->watch);
> + if (err) {
> + xenbus_dev_error(dev, err, "placing watch on %s",
> + info->frontend);
> + goto free_frontend;
> + }
This code has gone into xenbus_probe.c:talk_to_otherend. This means that we
can treat the frontend/frontend_id handling in the backend with the same code
as the backend/backend_id handling in the frontend.
I've also changed this so that the watch is on frontend/state, not the whole
frontend directory. This means that only state changes will trigger through
this code.
> + /* frontend_changed called immediately: stuff might be there already.*/
> + frontend_changed(&info->watch, NULL, 0);
This is not necessary, because the a watch is now implicitly fired by
xenstored when registered with it.
> + return 0;
> +
> +free_frontend:
> + kfree(info->frontend);
> +out:
> + return err;
> +}
> +
> +static void self_changed(struct xenbus_watch *watch,
> + const char **vec, unsigned int len)
> +{
> + struct skeleton_be_info *info;
> + int err;
> +
> + info = container_of(watch, struct skeleton_be_info, be_watch);
> + printk("%s: state %s\n", __func__, state(info));
> +
> + /* We only need this while we're waiting for config. */
> + if (info->state != WAITING)
> + return;
> +
> + /* Not there yet? Keep waiting. */
> + info->info = setup_device_specific_crap(info->dev);
> + if (!info->info)
> + return;
> +
> + info->state = EXISTS;
> + err = skeleton_watch_front(info->dev, info);
> + if (err) {
> + free_device_specific_crap(info->info);
> + return;
> + }
> +}
Device-specific, and often required to wait for the hotplug scripts.
> +static int skeleton_probe(struct xenbus_device *dev,
> + const struct xenbus_device_id *id)
> +{
> + int err;
> + struct skeleton_be_info *info;
> +
> + printk("skeleton_probe_be: %s\n", dev->nodename);
> +
> + dev->data = info = kmalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + xenbus_dev_error(dev, -ENOMEM, "allocating info structure");
> + return -ENOMEM;
> + }
> + info->dev = dev;
> + info->state = WAITING;
> +
> + /* Try for config (might need to wait for udev). */
> + info->be_watch.node = dev->nodename;
> + info->be_watch.callback = self_changed;
> + err = register_xenbus_watch(&info->be_watch);
> + if (err) {
> + xenbus_dev_error(dev, err, "placing watch on self %s",
> + dev->nodename);
> + kfree(info);
> + return err;
> + }
> + self_changed(&info->be_watch, NULL, 0);
> + return 0;
> +}
This is driver-specific, so stays where it is.
> +static int skeleton_remove(struct xenbus_device *dev)
> +{
> + struct skeleton_be_info *info = dev->data;
> +
> + switch (info->state) {
> + case CONNECTED:
> + unmap_page(info->vm, info->shmem_handle);
> + /* fall thru */
> + case READY:
> + skeleton_stop(info);
> + /* Must remove this after other fields. */
> + xenbus_rm(NULL, dev->nodename, "connected");
> + /* fall thru */
> + case EXISTS:
> + unregister_xenbus_watch(&info->watch);
> + free_device_specific_crap(info->info);
> + /* fall thru */
> + case WAITING:
> + unregister_xenbus_watch(&info->be_watch);
> + }
> +
> + kfree(info);
> +
> + return 0;
> +}
> +
> [Snip registration stuff, discussed below with the frontend driver]
> diff -r 20d1a79ebe31 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c
> --- /dev/null Wed Oct 26 15:59:13 2005
> +++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c Wed Nov 2 15:24:59 2005
> [Snip comments and includes]
> +/* Private information about this device */
> +struct skeleton_info
> +{
> + /* xenbus device we belong to */
> + struct xenbus_device *dev;
> +
> + /* backend path */
> + char *backend;
> +
> + /* backend id */
> + int backend_id;
> +
> + /* page we offer to share */
> + void *page;
> +
> + /* grant table reference to page we offer to backend */
> + int ring_ref;
> +
> + /* event channel to send interrupts to backend */
> + int evtchn;
> +
> + /* Watch we place on backend */
> + struct xenbus_watch watch;
> +
> + /* If we are fully connected to backend. */
> + enum state state;
> +
> + /* Information given by the backend */
> + int backend_stuff;
> +
> + /* Device-specific (eg. net, block) stuff. */
> + struct device_specific_info *info;
> +};
The treatment of this has been the same as the treatment of skeleton_be_info
-- the features related to the connection to the peer (the backend in this
case) have moved into xenbus_device, and everything else is driver-specific,
so stays.
> [Snip state() function, simple stringifier]
> +static inline int allocate_event_channel(domid_t id)
> +{
> + int err;
> + evtchn_op_t op = {
> + .cmd = EVTCHNOP_alloc_unbound,
> + .u.alloc_unbound.dom = DOMID_SELF,
> + .u.alloc_unbound.remote_dom = id };
> +
> + err = HYPERVISOR_event_channel_op(&op);
> + if (err)
> + return err;
> + return op.u.alloc_unbound.port;
> +}
This has moved into xenbus_client.c:xenbus_alloc_evtchn. I renamed it for
consistency with the command, to avoid confusion.
> +
> +static inline void free_event_channel(int evtchn)
> +{
> + evtchn_op_t op = {
> + .cmd = EVTCHNOP_close,
> + .u.close.port = evtchn };
> + HYPERVISOR_event_channel_op(&op);
> +}
There is no need for this in the real drivers, as they are all using
unbind_from_irqhandler, which handles closing the event channel implicitly.
> [ Snip {setup,free}_device_specific_crap, stop_device-requests ]
> +/* Write the information out to the store for the backend to read, and
> + * know we're ready. */
> +static int publish_info(struct skeleton_info *info)
> +{
> + struct xenbus_transaction *trans;
> + int err;
> +
> + printk("%s: state %s\n", __func__, state(info));
> + /* Transactions can fail spuriously, which means we loop. */
> +again:
> + trans = xenbus_transaction_start();
> + if (IS_ERR(trans))
> + return PTR_ERR(trans);
> +
> + err = xenbus_printf(trans, info->dev->nodename, "ring-reference", "%u",
> + info->ring_ref);
> + if (!err)
> + err = xenbus_printf(trans, info->dev->nodename,
> + "event-channel", "%u", info->evtchn);
> +
> + if (err) {
> + xenbus_transaction_end(trans, 1);
> + return err;
> + }
> + err = xenbus_transaction_end(trans, 0);
> + if (err == -EAGAIN)
> + goto again;
> + return err;
> +}
This is driver-specific publication of the connection info.
> +
> +/* Backend gone/going away. Clean up. */
> +static void skeleton_stop(struct skeleton_info *info)
> +{
> + printk("%s: state %s\n", __func__, state(info));
> + stop_device_requests(info->info);
> +
> + /* FIXME: can't use transaction, it requires alloc. */
> + xenbus_rm(NULL, info->dev->nodename, "ring-reference");
> + xenbus_rm(NULL, info->dev->nodename, "event-channel");
> +}
> +
> [Snip backend_changed, which has been split in a similar way to
> frontend_changed.]
> +static int skeleton_resume(struct xenbus_device *dev)
> +{
> + int err;
> + struct skeleton_info *info = dev->data;
> +
> + printk("%s: state %s\n", __func__, state(info));
> +
> + /* We need backend-id and path. */
> + err = xenbus_gather(NULL, dev->nodename,
> + "backend-id", "%i", &info->backend_id,
> + "backend", NULL, &info->backend,
> + NULL);
> + if (err) {
> + xenbus_dev_error(dev, err, "reading backend or backend-id");
> + goto out;
> + }
This has gone into xenbus_probe.c:read_from_otherend.
> + /* We need to allocate a page and event channel. */
> + info->page = (void *)__get_free_page(GFP_KERNEL);
> + if (!info->page) {
> + err = -ENOMEM;
> + xenbus_dev_error(dev, err, "allocating shared page");
> + goto free_backend;
> + }
> +
> + err = gnttab_grant_foreign_access(info->backend_id,
> + virt_to_mfn(info->page), 0);
> + if (err < 0) {
> + xenbus_dev_error(dev, err, "granting page");
> + goto free_page;
> + }
> + info->ring_ref = err;
> +
> + err = allocate_event_channel(info->backend_id);
> + if (err < 0) {
> + xenbus_dev_error(dev, err, "allocating event channel");
> + goto ungrant_page;
> + }
> + info->evtchn = err;
> +
> + info->watch.node = info->backend;
> + info->watch.callback = backend_changed;
> + err = register_xenbus_watch(&info->watch);
> + if (err) {
> + xenbus_dev_error(dev, err, "placing watch on %s", info->backend);
> + goto free_event_channel;
> + }
> + /* backend_changed called immediately: stuff might be there already. */
> + backend_changed(&info->watch, NULL, 0);
> + return 0;
> +
> +free_event_channel:
> + free_event_channel(info->evtchn);
> +ungrant_page:
> + /* FIXME: Need infrastructure to handle otherside holding onto page. */
> + gnttab_end_foreign_access(info->ring_ref, 0);
> +free_page:
> + free_page((unsigned long)info->page);
> +free_backend:
> + kfree(info->backend);
> +out:
> + return err;
> +}
> [Snip skeleton_probe; device-specific]
> +/* Clean up: will re-init and connect to backend on resume. */
> +static int skeleton_suspend(struct xenbus_device *dev)
> +{
> + struct skeleton_info *info = dev->data;
> +
> + printk("%s: state %s\n", __func__, state(info));
> + switch (info->state) {
> + case CONNECTED:
> + xenbus_rm(NULL, dev->nodename, "connected");
> + info->state = READY;
> + /* fall thru */
> + case READY:
> + skeleton_stop(info);
> + info->state = EXISTS;
> + case EXISTS:
> + ; /* Nothing to do */
> + }
> +
> + /* FIXME: Need infrastructure to handle otherside holding onto page. */
> + gnttab_end_foreign_access(info->ring_ref, 0);
> + free_page((unsigned long)info->page);
> + kfree(info->backend);
> + printk("%s:%i\n", __func__, __LINE__);
> + unregister_xenbus_watch(&info->watch);
> + printk("%s:%i\n", __func__, __LINE__);
> + return 0;
> +}
> [Snip skeleton_remove and the driver registration details, both pretty
> trivial]
> +static int init(void)
> +{
> + return xenbus_register_driver(&driver);
> +}
> +
> +static void fini(void)
> +{
> + xenbus_unregister_driver(&driver);
> +}
> +
> +module_init(init);
> +module_exit(fini);
All the drivers has init functions before, of course, but I have added exit
functions to the frontend drivers. The backend drivers I'm not sure about, as
the netback driver actually explicitly issues a BUG() if the exit function is
called.
> +MODULE_LICENSE("GPL");
I have added this to blkfront and netfront, except those files are both
BSD-licenced, so that's what I've added. The backend drivers don't have a
licence stated explicitly, so I've been forced to leave those alone for now.
That's it. As you can see, we've taken on board a lot of the abstractions
you've created here, and made them available to all drivers. Thanks a lot for
all your work!
Ewan.
^ permalink raw reply [flat|nested] 7+ messages in thread