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 to use evtchn_port_t for ports and other

To: "John D. Ramsdell" <ramsdell@xxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] Mini-OS to use evtchn_port_t for ports and other improvements
From: Steven Smith <sos22@xxxxxxxxx>
Date: Tue, 25 Jul 2006 11:27:15 +0100
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, Steven Smith <sos22@xxxxxxxxx>, Grzegorz Milos <gm281@xxxxxxxxx>, sos22@xxxxxxxxxxxxx
Delivery-date: Tue, 25 Jul 2006 03:27:44 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <ogt3bcszejg.fsf@xxxxxxxxxxxxxxx>
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>
References: <ogt3bcszejg.fsf@xxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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.

Attachment: signature.asc
Description: Digital signature

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