Xen devel - Jul 2006 - [PATCH] Mini-OS to use evtchn_port_t for ports and other improvements

The enclosed patch modifies Mini-OS so it uses evtchn_port_t for
ports, instead of the current mixture of u32''s and int''s.  It
provides
a name for the type of an event channel handler in events.h.  It
modifies evtchn_alloc_unbound so that it can be used to set up an
event channel to a domain other than zero.  It adds
evtchn_bind_interdomain to support the other half of event channel set
up.  Finally, adds a routine that translates grant table operation
status values to their string equivalent.

John

Signed-off-by: John D. Ramsdell <ramsdell@mitre.org>
Only in xen-unstable/extras/mini-os/console: console.o
diff -ur oxen-unstable/extras/mini-os/console/xencons_ring.c
xen-unstable/extras/mini-os/console/xencons_ring.c
--- oxen-unstable/extras/mini-os/console/xencons_ring.c	2006-07-22
01:09:59.000000000 -0400
+++ xen-unstable/extras/mini-os/console/xencons_ring.c	2006-07-22
17:11:09.000000000 -0400
@@ -53,7 +53,7 @@
 
 
 
-static void handle_input(int port, struct pt_regs *regs, void *ign)
+static void handle_input(evtchn_port_t port, struct pt_regs *regs, void *ign)
 {
 	struct xencons_interface *intf = xencons_interface();
 	XENCONS_RING_IDX cons, prod;
Only in xen-unstable/extras/mini-os/console: xencons_ring.c~
Only in xen-unstable/extras/mini-os/console: xencons_ring.o
diff -ur oxen-unstable/extras/mini-os/events.c
xen-unstable/extras/mini-os/events.c
--- oxen-unstable/extras/mini-os/events.c	2006-07-22 01:09:59.000000000 -0400
+++ xen-unstable/extras/mini-os/events.c	2006-07-22 17:09:34.000000000 -0400
@@ -26,20 +26,20 @@
 
 /* this represents a event handler. Chaining or sharing is not allowed */
 typedef struct _ev_action_t {
-	void (*handler)(int, struct pt_regs *, void *);
+	evtchn_handler_t handler;
 	void *data;
     u32 count;
 } ev_action_t;
 
 
 static ev_action_t ev_actions[NR_EVS];
-void default_handler(int port, struct pt_regs *regs, void *data);
+void default_handler(evtchn_port_t port, struct pt_regs *regs, void *data);
 
 
 /*
  * Demux events to different handlers.
  */
-int do_event(u32 port, struct pt_regs *regs)
+int do_event(evtchn_port_t port, struct pt_regs *regs)
 {
     ev_action_t  *action;
     if (port >= NR_EVS) {
@@ -60,8 +60,8 @@
 
 }
 
-int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *, void *),
-				 void *data )
+evtchn_port_t bind_evtchn(evtchn_port_t port, evtchn_handler_t handler, 
+						  void *data)
 {
  	if(ev_actions[port].handler != default_handler)
         printk("WARN: Handler for port %d already registered,
replacing\n",
@@ -77,7 +77,7 @@
 	return port;
 }
 
-void unbind_evtchn( u32 port )
+void unbind_evtchn(evtchn_port_t port)
 {
 	if (ev_actions[port].handler == default_handler)
 		printk("WARN: No handler for port %d when unbinding\n", port);
@@ -86,8 +86,7 @@
 	ev_actions[port].data = NULL;
 }
 
-int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *, void *data),
-			   void *data)
+int bind_virq(evtchn_port_t virq, evtchn_handler_t handler, void *data)
 {
 	evtchn_op_t op;
 
@@ -105,7 +104,7 @@
 	return 0;
 }
 
-void unbind_virq( u32 port )
+void unbind_virq(evtchn_port_t port)
 {
 	unbind_evtchn(port);
 }
@@ -142,32 +141,57 @@
     }
 }
 
-void default_handler(int port, struct pt_regs *regs, void *ignore)
+void default_handler(evtchn_port_t port, struct pt_regs *regs, void *ignore)
 {
     printk("[Port %d] - event received\n", port);
 }
 
+/* Create a port available to the pal for exchanging notifications.
+   Returns the result of the hypervisor call. */
+
 /* Unfortunate confusion of terminology: the port is unbound as far
    as Xen is concerned, but we automatically bind a handler to it
    from inside mini-os. */
-int evtchn_alloc_unbound(void (*handler)(int, struct pt_regs *regs,
-										 void *data),
-						 void *data)
-{
-	u32 port;
-	evtchn_op_t op;
-	int err;
 
-	op.cmd = EVTCHNOP_alloc_unbound;
-	op.u.alloc_unbound.dom = DOMID_SELF;
-	op.u.alloc_unbound.remote_dom = 0;
-
-	err = HYPERVISOR_event_channel_op(&op);
-	if (err) {
-		printk("Failed to alloc unbound evtchn: %d.\n", err);
-		return -1;
-	}
-	port = op.u.alloc_unbound.port;
-	bind_evtchn(port, handler, data);
-	return port;
+static inline evtchn_port_t
+maybe_bind_evtchn(evtchn_port_t port, evtchn_handler_t handler, void *data)
+{
+    if (handler)
+		return bind_evtchn(port, handler, data);
+    else
+		return port;
+}
+
+int evtchn_alloc_unbound(domid_t pal, evtchn_handler_t handler,
+						 void *data, evtchn_port_t *port)
+{
+    evtchn_op_t op;
+    op.cmd = EVTCHNOP_alloc_unbound;
+    op.u.alloc_unbound.dom = DOMID_SELF;
+    op.u.alloc_unbound.remote_dom = pal;
+    int err = HYPERVISOR_event_channel_op(&op);
+    if (err)
+		return err;
+    *port = maybe_bind_evtchn(op.u.alloc_unbound.port, handler, data);
+    return err;
+}
+
+/* Connect to a port so as to allow the exchange of notifications with
+   the pal. Returns the result of the hypervisor call. */
+
+int evtchn_bind_interdomain(domid_t pal, evtchn_port_t remote_port,
+							evtchn_handler_t handler, void *data,
+							evtchn_port_t *local_port)
+{
+    evtchn_op_t op;
+    op.cmd = EVTCHNOP_bind_interdomain;
+    op.u.bind_interdomain.remote_dom = pal;
+    op.u.bind_interdomain.remote_port = remote_port;
+    int err = HYPERVISOR_event_channel_op(&op);
+    if (err)
+		return err;
+    evtchn_port_t port = op.u.bind_interdomain.local_port;
+    clear_evtchn(port);	      /* Without, handler gets invoked now! */
+    *local_port = maybe_bind_evtchn(port, handler, data);
+    return err;
 }
Only in xen-unstable/extras/mini-os: events.o
diff -ur oxen-unstable/extras/mini-os/gnttab.c
xen-unstable/extras/mini-os/gnttab.c
--- oxen-unstable/extras/mini-os/gnttab.c	2006-07-22 01:09:59.000000000 -0400
+++ xen-unstable/extras/mini-os/gnttab.c	2006-07-22 16:41:12.000000000 -0400
@@ -137,6 +137,24 @@
     return gref;
 }
 
+static char *gnttabop_error_msgs[] = GNTTABOP_error_msgs;
+
+static const size_t gnttabop_error_size +   
sizeof(gnttabop_error_msgs)/sizeof(gnttabop_error_msgs[0]);
+
+const char *
+gnttabop_error(int16_t status)
+{
+    status = -status;
+    if (status < 0 || status >= gnttabop_error_size)
+	return "bad status";
+    const char *msg = gnttabop_error_msgs[status];
+    if (msg)
+	return msg;
+    else
+	return "bad status";
+}
+
 void
 init_gnttab(void)
 {
Only in xen-unstable/extras/mini-os: gnttab.o
Only in xen-unstable/extras/mini-os: hypervisor.o
diff -ur oxen-unstable/extras/mini-os/include/events.h
xen-unstable/extras/mini-os/include/events.h
--- oxen-unstable/extras/mini-os/include/events.h	2006-07-22 01:09:59.000000000
-0400
+++ xen-unstable/extras/mini-os/include/events.h	2006-07-22 17:08:38.000000000
-0400
@@ -22,20 +22,24 @@
 #include<traps.h>
 #include <xen/event_channel.h>
 
+typedef void (*evtchn_handler_t)(evtchn_port_t, struct pt_regs *, void *);
+
 /* prototypes */
-int do_event(u32 port, struct pt_regs *regs);
-int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *, void *data),
-			   void *data);
-int bind_evtchn( u32 virq, void (*handler)(int, struct pt_regs *, void *data),
-				 void *data );
-void unbind_evtchn( u32 port );
+int do_event(evtchn_port_t port, struct pt_regs *regs);
+int bind_virq(evtchn_port_t virq, evtchn_handler_t handler, void *data);
+evtchn_port_t bind_evtchn(evtchn_port_t virq, evtchn_handler_t handler, 
+						  void *data);
+void unbind_evtchn(evtchn_port_t port);
 void init_events(void);
-void unbind_virq( u32 port );
-int evtchn_alloc_unbound(void (*handler)(int, struct pt_regs *regs,
-										 void *data),
-						 void *data);
+void unbind_virq(evtchn_port_t port);
+int evtchn_alloc_unbound(domid_t pal, evtchn_handler_t handler,
+						 void *data, evtchn_port_t *port);
+
+int evtchn_bind_interdomain(domid_t pal, evtchn_port_t remote_port,
+							evtchn_handler_t handler, void *data,
+							evtchn_port_t *local_port);
 
-static inline int notify_remote_via_evtchn(int port)
+static inline int notify_remote_via_evtchn(evtchn_port_t port)
 {
     evtchn_op_t op;
     op.cmd = EVTCHNOP_send;
Only in xen-unstable/extras/mini-os/include: events.h~
diff -ur oxen-unstable/extras/mini-os/include/gnttab.h
xen-unstable/extras/mini-os/include/gnttab.h
--- oxen-unstable/extras/mini-os/include/gnttab.h	2006-07-22 01:09:59.000000000
-0400
+++ xen-unstable/extras/mini-os/include/gnttab.h	2006-07-22 16:41:25.000000000
-0400
@@ -10,5 +10,6 @@
 grant_ref_t gnttab_grant_transfer(domid_t domid, unsigned long pfn);
 unsigned long gnttab_end_transfer(grant_ref_t gref);
 int gnttab_end_access(grant_ref_t ref);
+const char *gnttabop_error(int16_t status);
 
 #endif /* !__GNTTAB_H__ */
Only in xen-unstable/extras/mini-os/include: gnttab.h~
Only in xen-unstable/extras/mini-os/include: xen
Only in xen-unstable/extras/mini-os: kernel.o
Only in xen-unstable/extras/mini-os/lib: math.o
Only in xen-unstable/extras/mini-os/lib: printf.o
Only in xen-unstable/extras/mini-os/lib: string.o
Only in xen-unstable/extras/mini-os/lib: xmalloc.o
Only in xen-unstable/extras/mini-os: libminios.a
Only in xen-unstable/extras/mini-os: mini-os.elf
Only in xen-unstable/extras/mini-os: mini-os.gz
Only in xen-unstable/extras/mini-os: mm.o
Only in xen-unstable/extras/mini-os: sched.o
diff -ur oxen-unstable/extras/mini-os/time.c xen-unstable/extras/mini-os/time.c
--- oxen-unstable/extras/mini-os/time.c	2006-07-22 01:09:59.000000000 -0400
+++ xen-unstable/extras/mini-os/time.c	2006-07-22 17:10:15.000000000 -0400
@@ -215,7 +215,7 @@
 /*
  * Just a dummy 
  */
-static void timer_handler(int ev, struct pt_regs *regs, void *ign)
+static void timer_handler(evtchn_port_t ev, struct pt_regs *regs, void *ign)
 {
     static int i;
 
Only in xen-unstable/extras/mini-os: time.o
Only in xen-unstable/extras/mini-os: traps.o
Only in xen-unstable/extras/mini-os: x86_32.o
diff -ur oxen-unstable/extras/mini-os/xenbus/xenbus.c
xen-unstable/extras/mini-os/xenbus/xenbus.c
--- oxen-unstable/extras/mini-os/xenbus/xenbus.c	2006-07-22 01:09:59.000000000
-0400
+++ xen-unstable/extras/mini-os/xenbus/xenbus.c	2006-07-22 17:10:48.000000000
-0400
@@ -112,7 +112,8 @@
     }
 }
 
-static void xenbus_evtchn_handler(int port, struct pt_regs *regs, void *ign)
+static void xenbus_evtchn_handler(evtchn_port_t port, struct pt_regs *regs,
+				  void *ign)
 {
     wake_up(&xb_waitq);
 }
Only in xen-unstable/extras/mini-os/xenbus: xenbus.c~
Only in xen-unstable/extras/mini-os/xenbus: xenbus.o

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

Sorry about the delay responding, I''ve been busy with other things.

This mostly looks like a pretty reasonable bit of cleanup, with just a
few minor niggles.
> @@ -86,8 +86,7 @@
>  	ev_actions[port].data = NULL;
>  }
>  
> -int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *, void
*data),
> -			   void *data)
> +int bind_virq(evtchn_port_t virq, evtchn_handler_t handler, void *data)
Did you mean this?  A virq isn''t the same as an evtchn_port_t, and is
usually uint32_t in the Xen header files.
>  {
>  	evtchn_op_t op;
>  
> @@ -105,7 +104,7 @@
>  	return 0;
>  }
>  
> -void unbind_virq( u32 port )
> +void unbind_virq(evtchn_port_t port)
>  {
>  	unbind_evtchn(port);
>  }
Hmm... not your fault, but unbinding from virqs is broken.  You need
to know the port number, but there''s no way of getting it from
bind_virq.  Given that noone in the tree currently uses either
unbind_virq, I''d be inclined to just kill it.
> +static inline evtchn_port_t
> +maybe_bind_evtchn(evtchn_port_t port, evtchn_handler_t handler, void
*data)
> +{
> +    if (handler)
> +		return bind_evtchn(port, handler, data);
> +    else
> +		return port;
> +}
> +
> +int evtchn_alloc_unbound(domid_t pal, evtchn_handler_t handler,
> +						 void *data, evtchn_port_t *port)
> +{
> +    evtchn_op_t op;
> +    op.cmd = EVTCHNOP_alloc_unbound;
> +    op.u.alloc_unbound.dom = DOMID_SELF;
> +    op.u.alloc_unbound.remote_dom = pal;
> +    int err = HYPERVISOR_event_channel_op(&op);
> +    if (err)
> +		return err;
> +    *port = maybe_bind_evtchn(op.u.alloc_unbound.port, handler, data);
Why maybe_bind?  Do you ever expect to need to allocate an unbound
event channel before you know what handler to use for it?
> +    return err;
> +}
> +
> +/* Connect to a port so as to allow the exchange of notifications with
> +   the pal. Returns the result of the hypervisor call. */
> +
> +int evtchn_bind_interdomain(domid_t pal, evtchn_port_t remote_port,
> +							evtchn_handler_t handler, void *data,
> +							evtchn_port_t *local_port)
> +{
> +    evtchn_op_t op;
> +    op.cmd = EVTCHNOP_bind_interdomain;
> +    op.u.bind_interdomain.remote_dom = pal;
> +    op.u.bind_interdomain.remote_port = remote_port;
> +    int err = HYPERVISOR_event_channel_op(&op);
> +    if (err)
> +		return err;
> +    evtchn_port_t port = op.u.bind_interdomain.local_port;
> +    clear_evtchn(port);	      /* Without, handler gets invoked now! */
Invoking the handler as soon as you bind the interdomain channel is a
mostly-deliberate part of the interface.  If the other end makes
notifications before you get around to binding they can get lost, and
forcing the channel to fire as soon as you bind to it avoids some
potential lost wakeups.
> +    *local_port = maybe_bind_evtchn(port, handler, data);
Again, why maybe_bind?
> @@ -137,6 +137,24 @@
>      return gref;
>  }
>  
> +static char *gnttabop_error_msgs[] = GNTTABOP_error_msgs;
Maybe static const char *const gnttabop_error_msgs[] ?
> +
> +static const size_t gnttabop_error_size > +   
sizeof(gnttabop_error_msgs)/sizeof(gnttabop_error_msgs[0]);
Use ARRAY_SIZE here.


Other than that, this looks pretty good to me.

Steven.


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

Steven Smith <sos22@cam.ac.uk> writes:
> This mostly looks like a pretty reasonable bit of cleanup, with just
> a few minor niggles.
Steven,

Thank you for your considered opinions.  For most of your comments, I
plan to implement your suggestions precisely.  I''d like to discuss
just two of them.
> Why maybe_bind?  Do you ever expect to need to allocate an unbound
> event channel before you know what handler to use for it?
I wanted to capture the usual pattern of immediately binding a port
after it''s allocated, without forcing programmers to follow that
pattern.  In the case of evtchn_bind_interdomain, you wondered why I
added the call to clear_event.  Should it be decided it should be
eliminated, the use of maybe_bind allows a programmer to still use the
function, but delay the binding until after the programmer calls
clear_event.
> > +    evtchn_port_t port = op.u.bind_interdomain.local_port;
> > +    clear_evtchn(port);	      /* Without, handler gets invoked now!
*/
> Invoking the handler as soon as you bind the interdomain channel is
> a mostly-deliberate part of the interface.  If the other end makes
> notifications before you get around to binding they can get lost,
> and forcing the channel to fire as soon as you bind to it avoids
> some potential lost wakeups.
If the handler is invoke whenever the port is bound, there is no
information to be gained as a result of the first invocation of the
handler--as the programmer already knows when it will happen.  If it
is important to invoke the handler upon binding, why can''t the
programmer simply follow the call the to evtchn_bind_interdomain with
a call to the evt_handler of type evt_handler_t with:

    (*evt_handler)(port, NULL, data);

where both port and data are already known for the call to the
function evtchn_bind_interdomain?  It''s easy to simulate the case of a
handler call on binding with clear_evtchn included, but a pain to
handle the case in which one wants the handler to be invoked only when
a notification arrives, when it is omitted.

John

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

Steven,

Enclosed is a revision of the patch that responds to your
suggested improvements. 

John

The enclosed patch modifies Mini-OS so it uses evtchn_port_t for
ports, instead of the current mixture of u32''s and int''s.  It
provides
a name for the type of an event channel handler in events.h.  It
modifies evtchn_alloc_unbound so that it can be used to set up an
event channel to a domain other than zero.  It adds
evtchn_bind_interdomain to support the other half of event channel set
up.  Finally, adds a routine that translates grant table operation
status values to their string equivalent.



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

> > Why maybe_bind?  Do you ever expect to need to allocate an unbound
> > event channel before you know what handler to use for it?
> I wanted to capture the usual pattern of immediately binding a port
> after it''s allocated, without forcing programmers to follow that
> pattern.
That''s not a bad idea, but I''d rather leave this until we have
an
example of some actual code which needs it.
> > > +    evtchn_port_t port = op.u.bind_interdomain.local_port;
> > > +    clear_evtchn(port);	      /* Without, handler gets invoked
now! */
> > Invoking the handler as soon as you bind the interdomain channel is
> > a mostly-deliberate part of the interface.  If the other end makes
> > notifications before you get around to binding they can get lost,
> > and forcing the channel to fire as soon as you bind to it avoids
> > some potential lost wakeups.
> It''s easy to simulate the case of a handler call on binding with
> clear_evtchn included, but a pain to handle the case in which one
> wants the handler to be invoked only when a notification arrives,
> when it is omitted.
I think you have a point here.  Consider my objection withdrawn.

Steven.


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

Steven Smith <sos22-xen@srcf.ucam.org> writes:
> > > Why maybe_bind?  ...
> That''s not a bad idea, but I''d rather leave this until we
have an
> example of some actual code which needs it.
Maybe bind has been eliminated.
> 
> > > > +    evtchn_port_t port = op.u.bind_interdomain.local_port;
> > > > +    clear_evtchn(port);	      /* Without, handler gets
invoked now! */
> > > Invoking the handler as soon as you bind the interdomain channel
is
> > > a mostly-deliberate part of the interface.  If the other end
makes
> > > notifications before you get around to binding they can get lost,
> > > and forcing the channel to fire as soon as you bind to it avoids
> > > some potential lost wakeups.
> > It''s easy to simulate the case of a handler call on binding
with
> > clear_evtchn included, but a pain to handle the case in which one
> > wants the handler to be invoked only when a notification arrives,
> > when it is omitted.
> I think you have a point here.  Consider my objection withdrawn.
I added the clear_evtchn back in.  Here is the patch freshly minted
patch.

John

The enclosed patch modifies Mini-OS so it uses evtchn_port_t for
ports, instead of the current mixture of u32''s and int''s.  It
provides
a name for the type of an event channel handler in events.h.  It
modifies evtchn_alloc_unbound so that it can be used to set up an
event channel to a domain other than zero.  It adds
evtchn_bind_interdomain to support the other half of event channel set
up.  Finally, it adds a routine that translates grant table operation
status values to their string equivalent.



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