WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Grzegorz Milos <gm281@xxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] Mini-OS events handling cleanup
From: Horms <horms@xxxxxxxxxxxx>
Date: Tue, 11 Jul 2006 21:30:17 +0900 (JST)
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, Steven Smith <sos22@xxxxxxxxx>
Delivery-date: Wed, 12 Jul 2006 01:33:45 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <B61CAEDF-574C-42F9-86D6-160DBFC81FB6@xxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: tin/1.8.2-20060425 ("Shillay") (UNIX) (Linux/2.6.17-1-686 (i686))
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