Hi Grzegorz,
I've made a few coding-style comments inline.
--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/
On Wed, 5 Jul 2006 11:22:20 +0100, Grzegorz Milos wrote:
>
> --Apple-Mail-3--158434814
> Content-Transfer-Encoding: 7bit
> Content-Type: text/plain;
> charset=US-ASCII;
> format=flowed
>
> Hello Keir, could you apply the following cleanup patch?
>
> Cheers
> Gregor
>
>
> --Apple-Mail-3--158434814
> Content-Transfer-Encoding: 7bit
> Content-Type: application/octet-stream; x-unix-mode=0644;
> name="minios.events.patch"
> Content-Disposition: attachment;
> filename=minios.events.patch
>
> # HG changeset patch
> # User gm281@xxxxxxxxxxxxxxxxxx
> # Node ID ef01229d6344d7ff1c8970512fdb812445ec3430
> # Parent c3d6610fc0f0510a09bd3d6c63a4e8a3f78312cc
> Mini-OS: Events handling cleaned up. The interface extended to provide void*
> pointer to handlers.
>
> Signed-off-by: Steven Smith <sos22@xxxxxxxxx>
> Signed-off-by: Grzegorz Milos <gm281@xxxxxxxxx>
>
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/console/xencons_ring.c
> --- a/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:20:15 2006
> @@ -53,7 +53,7 @@
>
>
>
> -static void handle_input(int port, struct pt_regs *regs)
> +static void handle_input(int port, struct pt_regs *regs, void *ign)
> {
> struct xencons_interface *intf = xencons_interface();
> XENCONS_RING_IDX cons, prod;
> @@ -83,7 +83,8 @@
> if (!start_info.console_evtchn)
> return 0;
>
> - err = bind_evtchn(start_info.console_evtchn, handle_input);
> + err = bind_evtchn(start_info.console_evtchn, handle_input,
> + NULL);
Could you move the NULL ont the same line as bind_evtchn( ?
> if (err <= 0) {
> printk("XEN console request chn bind failed %i\n", err);
> return err;
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/events.c
> --- a/extras/mini-os/events.c Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/events.c Wed Jul 5 10:20:15 2006
> @@ -22,9 +22,18 @@
> #include <events.h>
> #include <lib.h>
>
> +#define NR_EVS 1024
> +
> +/* this represents a event handler. Chaining or sharing is not allowed */
> +typedef struct _ev_action_t {
> + void (*handler)(int, struct pt_regs *, void *);
> + void *data;
> + u32 count;
> +} ev_action_t;
> +
Tabs and spaces are used inconsistently in the above structure definition.
> static ev_action_t ev_actions[NR_EVS];
> -void default_handler(int port, struct pt_regs *regs);
> +void default_handler(int port, struct pt_regs *regs, void *data);
>
>
> /*
> @@ -35,42 +44,33 @@
> ev_action_t *action;
> if (port >= NR_EVS) {
> printk("Port number too large: %d\n", port);
> - goto out;
> + goto out;
> }
>
> action = &ev_actions[port];
> action->count++;
>
> - if (!action->handler)
> - {
> - printk("Spurious event on port %d\n", port);
> - goto out;
> - }
> -
> - if (action->status & EVS_DISABLED)
> - {
> - printk("Event on port %d disabled\n", port);
> - goto out;
> - }
> -
> /* call the handler */
> - action->handler(port, regs);
> -
> + action->handler(port, regs, action->data);
> +
> out:
> clear_evtchn(port);
> +
> return 1;
>
> }
>
> -int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *) )
> +int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *, void *),
> + void *data )
> {
> if(ev_actions[port].handler != default_handler)
> printk("WARN: Handler for port %d already registered, replacing\n",
> port);
>
> + ev_actions[port].data = data;
> + wmb();
> ev_actions[port].handler = handler;
> - ev_actions[port].status &= ~EVS_DISABLED;
> -
> +
> /* Finally unmask the port */
> unmask_evtchn(port);
>
> @@ -82,13 +82,14 @@
> if (ev_actions[port].handler == default_handler)
> printk("WARN: No handler for port %d when unbinding\n", port);
> ev_actions[port].handler = default_handler;
> - ev_actions[port].status |= EVS_DISABLED;
> + wmb();
> + ev_actions[port].data = NULL;
> }
I'm not expert here, but the wmb() additions seem a bit odd.
Are the neccessary?
>
> -int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *) )
> +int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *, void *data),
> + void *data)
> {
> evtchn_op_t op;
> - int ret = 0;
>
> /* Try to bind the virq to a port */
> op.cmd = EVTCHNOP_bind_virq;
> @@ -97,13 +98,11 @@
>
> if ( HYPERVISOR_event_channel_op(&op) != 0 )
> {
> - ret = 1;
> printk("Failed to bind virtual IRQ %d\n", virq);
> - goto out;
> + return 1;
> }
> - bind_evtchn(op.u.bind_virq.port, handler);
> -out:
> - return ret;
> + bind_evtchn(op.u.bind_virq.port, handler, data);
> + return 0;
> }
>
> void unbind_virq( u32 port )
> @@ -137,13 +136,38 @@
> #endif
> /* inintialise event handler */
> for ( i = 0; i < NR_EVS; i++ )
> - {
> - ev_actions[i].status = EVS_DISABLED;
> + {
> ev_actions[i].handler = default_handler;
> mask_evtchn(i);
> }
> }
>
> -void default_handler(int port, struct pt_regs *regs) {
> +void default_handler(int port, struct pt_regs *regs, void *ignore)
> +{
> printk("[Port %d] - event received\n", port);
> }
> +
> +/* 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;
> +}
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/include/events.h
> --- a/extras/mini-os/include/events.h Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/include/events.h Wed Jul 5 10:20:15 2006
> @@ -22,28 +22,18 @@
> #include<traps.h>
> #include <xen/event_channel.h>
>
> -#define NR_EVS 1024
> -
> -/* ev handler status */
> -#define EVS_INPROGRESS 1 /* Event handler active - do not enter!
> */
> -#define EVS_DISABLED 2 /* Event disabled - do not enter! */
> -#define EVS_PENDING 4 /* Event pending - replay on enable */
> -#define EVS_REPLAY 8 /* Event has been replayed but not acked yet */
> -
> -/* this represents a event handler. Chaining or sharing is not allowed */
> -typedef struct _ev_action_t {
> - void (*handler)(int, struct pt_regs *);
> - unsigned int status; /* IRQ status */
> - u32 count;
> -} ev_action_t;
> -
> /* prototypes */
> int do_event(u32 port, struct pt_regs *regs);
> -int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *) );
> -int bind_evtchn( u32 virq, void (*handler)(int, struct pt_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 );
> void init_events(void);
> void unbind_virq( u32 port );
> +int evtchn_alloc_unbound(void (*handler)(int, struct pt_regs *regs,
> +
> void *data),
> + void *data);
>
> static inline int notify_remote_via_evtchn(int port)
> {
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/include/lib.h
> --- a/extras/mini-os/include/lib.h Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/include/lib.h Wed Jul 5 10:20:15 2006
> @@ -89,6 +89,7 @@
> char *strchr(const char *s, int c);
> char *strstr(const char *s1, const char *s2);
> char * strcat(char * dest, const char * src);
> +char *strdup(const char *s);
Is this strdup addition related to the rest of the patch?
> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> @@ -98,6 +99,18 @@
> size_t iov_len;
> };
>
> +#define ASSERT(x) \
> +do { \
> + if (!(x)) { \
> + printk("ASSERTION FAILED: %s at %s:%d.\n", \
> + # x , \
> + __FILE__, \
> + __LINE__); \
Could the above 4 lines be merged into one or two?
Also, is the do {} while(0) blocl neccessary if evertthing is
inside an if() { } block ?
> + BUG(); \
> + } \
> +} while(0)
>
> +/* Consistency check as much as possible. */
> +void sanity_check(void);
>
> #endif /* _LIB_H_ */
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/lib/string.c
> --- a/extras/mini-os/lib/string.c Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/lib/string.c Wed Jul 5 10:20:15 2006
> @@ -23,6 +23,7 @@
> #include <os.h>
> #include <types.h>
> #include <lib.h>
> +#include <xmalloc.h>
>
> int memcmp(const void * cs,const void * ct,size_t count)
> {
> @@ -156,4 +157,13 @@
> return NULL;
> }
>
> +char *strdup(const char *x)
> +{
> + int l = strlen(x);
> + char *res = malloc(l + 1);
> + if (!res) return NULL;
> + memcpy(res, x, l + 1);
> + return res;
> +}
> +
> #endif
Again, strdup.
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/time.c
> --- a/extras/mini-os/time.c Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/time.c Wed Jul 5 10:20:15 2006
> @@ -215,7 +215,7 @@
> /*
> * Just a dummy
> */
> -static void timer_handler(int ev, struct pt_regs *regs)
> +static void timer_handler(int ev, struct pt_regs *regs, void *ign)
> {
> static int i;
>
> @@ -233,5 +233,5 @@
> void init_time(void)
> {
> printk("Initialising timer interface\n");
> - bind_virq(VIRQ_TIMER, &timer_handler);
> -}
> + bind_virq(VIRQ_TIMER, &timer_handler, NULL);
> +}
> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/xenbus/xenbus.c
> --- a/extras/mini-os/xenbus/xenbus.c Wed Jul 5 10:06:14 2006
> +++ b/extras/mini-os/xenbus/xenbus.c Wed Jul 5 10:20:15 2006
> @@ -112,7 +112,7 @@
> }
> }
>
> -static void xenbus_evtchn_handler(int port, struct pt_regs *regs)
> +static void xenbus_evtchn_handler(int port, struct pt_regs *regs, void *ign)
> {
> wake_up(&xb_waitq);
> }
> @@ -174,7 +174,8 @@
> create_thread("xenstore", xenbus_thread_func, NULL);
> DEBUG("buf at %p.\n", xenstore_buf);
> err = bind_evtchn(start_info.store_evtchn,
> - xenbus_evtchn_handler);
> + xenbus_evtchn_handler,
> + NULL);
Again, could the NULL be appended to the previous line?
> DEBUG("xenbus on irq %d\n", err);
> }
>
>
> --Apple-Mail-3--158434814
> Content-Type: text/plain; charset="us-ascii"
> MIME-Version: 1.0
> Content-Transfer-Encoding: 7bit
> Content-Disposition: inline
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>
> --Apple-Mail-3--158434814--
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home