Mini-OS to use evtchn_port_t for ports and other improvements
PATCH 1/1 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
Only in xen-unstable/extras/mini-os/console: console.o
extras/mini-os/console/xencons_ring.c +1 -1
@@ -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
extras/mini-os/events.c +53 -29
135 lines not on this page · raw message
Only in xen-unstable/extras/mini-os: events.o
extras/mini-os/gnttab.c +18 -0
@@ -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
extras/mini-os/include/events.h +15 -11
@@ -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~
extras/mini-os/include/gnttab.h +1 -0
@@ -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
extras/mini-os/time.c +1 -1
@@ -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
extras/mini-os/xenbus/xenbus.c +2 -1
@@ -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
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.
extras/mini-os/events.c line 89
Quoted 7 lines
extras/mini-os/events.c +1
-2@@ -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.
extras/mini-os/events.c line 110
Quoted 12 lines
extras/mini-os/events.c +1
-1{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.
Quoted 11 lines
extras/mini-os/events.c +10
-0+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,
extras/mini-os/events.c line 175
Quoted 10 lines
extras/mini-os/events.c +10
-0+ 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?
Quoted 9 lines
extras/mini-os/events.c +9
-0+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)
extras/mini-os/events.c line 194
Quoted 10 lines
extras/mini-os/events.c +10
-0+{ +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.
Quoted 1 line
+
*local_port=maybe_bind_evtchn(port, handler, data);
Again, why maybe_bind?
extras/mini-os/gnttab.c line 140
Quoted 5 lines
extras/mini-os/gnttab.c +1
-0@@ -137,6 +137,24 @@return gref; } +static char *gnttabop_error_msgs[] = GNTTABOP_error_msgs;
Maybe static const char *const gnttabop_error_msgs[] ?
extras/mini-os/gnttab.c line 143
Quoted 3 lines
extras/mini-os/gnttab.c +3
-0+ +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
[email protected]
http://lists.xensource.com/xen-devel
Quoted 2 lines · Steven Smith
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.
Quoted 2 lines
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.
Quoted 7 lines
extras/mini-os/events.c line 194
Quoted 2 lines
extras/mini-os/events.c +2
-0+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
Quoted 5 lines
Quoted 2 lines
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.
Quoted 11 lines
Quoted 7 lines
extras/mini-os/events.c line 194
Quoted 2 lines
extras/mini-os/events.c +2
-0+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_evtchnincluded, 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
[email protected]
http://lists.xensource.com/xen-devel
Quoted 3 lines · Steven Smith
Quoted 1 line
Quoted 1 line
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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ 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.
Quoted 5 lines
Quoted 4 lines
It's easy to simulate the case of a handler call on binding with
clear_evtchnincluded, 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.
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.
Only in xen-unstable/extras/mini-os/console: console.o
extras/mini-os/console/xencons_ring.c +1 -1
@@ -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.o
extras/mini-os/events.c +43 -33
129 lines not on this page · raw message
Only in xen-unstable/extras/mini-os: events.o
extras/mini-os/gnttab.c +19 -0
@@ -137,6 +137,18 @@return gref; } +static const char *gnttabop_error_msgs[] = GNTTABOP_error_msgs; + +const char * +gnttabop_error (int16_t status) +{ + status = -status; +if (status <0 || status >=ARRAY_SIZE (gnttabop_error_msgs)) +return "bad status" ; +else +return gnttabop_error_msgs[status]; +} +void init_gnttab (void ) {
@@ -156,3 +168,10 @@ gnttab_table =map_frames (frames,NR_GRANT_FRAMES );printk ("gnttab_table mapped at %p.\n" , gnttab_table); } + +/* +* Local variables: +* mode: C +* c-basic-offset: 4 +* End: +*/
Only in xen-unstable/extras/mini-os: gnttab.c~
Only in xen-unstable/extras/mini-os: gnttab.o
Only in xen-unstable/extras/mini-os: hypervisor.o
extras/mini-os/include/events.h +13 -11
@@ -22,20 +22,22 @@#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 (uint32_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); +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~
extras/mini-os/include/gnttab.h +1 -0
@@ -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: 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
extras/mini-os/time.c +1 -1
@@ -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
extras/mini-os/xenbus/xenbus.c +2 -1
@@ -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.o
_______________________________________________
Xen-devel mailing list
[email protected]
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.
Only in xen-unstable/extras/mini-os/console: console.o
extras/mini-os/console/xencons_ring.c +1 -1
@@ -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.o
extras/mini-os/events.c +40 -33
126 lines not on this page · raw message
Only in xen-unstable/extras/mini-os: events.o
extras/mini-os/gnttab.c +21 -0
@@ -137,6 +137,20 @@return gref; } +static const char *gnttabop_error_msgs[] = GNTTABOP_error_msgs; + +static const size_t gnttabop_error_size =ARRAY_SIZE (gnttabop_error_msgs); + +const char * +gnttabop_error (int16_t status) +{ + status = -status; +if (status <0 || status >= gnttabop_error_size) +return "bad status" ; +else +return gnttabop_error_msgs[status]; +} +void init_gnttab (void ) {
@@ -156,3 +170,10 @@ gnttab_table =map_frames (frames,NR_GRANT_FRAMES );printk ("gnttab_table mapped at %p.\n" , gnttab_table); } + +/* +* Local variables: +* mode: C +* c-basic-offset: 4 +* End: +*/
Only in xen-unstable/extras/mini-os: gnttab.o
Only in xen-unstable/extras/mini-os: hypervisor.o
extras/mini-os/include/events.h +13 -11
@@ -22,20 +22,22 @@#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 (uint32_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); +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_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~
extras/mini-os/include/gnttab.h +1 -0
@@ -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: 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
extras/mini-os/time.c +1 -1
@@ -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
extras/mini-os/xenbus/xenbus.c +2 -1
@@ -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.o
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel