[Xen-devel] Re: [PATCH] Mini-OS events handling cleanup

[Horms <horms@xxxxxxxxxxxx>]

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