Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Anthony Liguori <anthony@codemonkey.ws>]

Gerd Hoffmann wrote:
> This adds a xen event channel implementation to qemu, intented to be
> used by xenner (aka xen emulation).
>
> The patch also adds a XenEvtOps struct with function pointers for the
> xc_evtchn_* family, which is used to switch between libxenctrl and the
> qemu implementation at runtime.  By default libxenctrl is used.
>   

I suppose the QEMU implementation is to eventually eliminate the need 
for libxc?  Do you also plan on doing a XenStore implementation within QEMU?

> ---
>  Makefile.target          |    1 +
>  hw/xen_interfaces.h      |   27 +++
>  hw/xen_machine_pv.c      |    2 +
>  hw/xenner_libxc_evtchn.c |  396 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 426 insertions(+), 0 deletions(-)
>  create mode 100644 hw/xen_interfaces.h
>  create mode 100644 hw/xenner_libxc_evtchn.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 5c97874..b88fd8f 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -521,6 +521,7 @@ endif
>  # xen backend driver support
>  XEN_OBJS := xen_machine_pv.o xen_backend.o xen_devconfig.o xen_domainbuild.o
>  XEN_OBJS += xen_console.o xen_framebuffer.o xen_disk.o xen_nic.o
> +XEN_OBJS += xenner_libxc_evtchn.o
>   

Historically, we didn't have directories in QEMU because we used CVS and 
directories are a nightmare.  With the shear number of files being added 
for Xen, it probably makes sense to stick them all in a directory.

>  ifeq ($(CONFIG_XEN), yes)
>    OBJS += $(XEN_OBJS)
>    LIBS += $(XEN_LIBS)
> diff --git a/hw/xen_interfaces.h b/hw/xen_interfaces.h
> new file mode 100644
> index 0000000..869b382
> --- /dev/null
> +++ b/hw/xen_interfaces.h
> @@ -0,0 +1,27 @@
> +#ifndef QEMU_XEN_INTERFACES_H
> +#define QEMU_XEN_INTERFACES_H 1
>   

Minor nit, make sure to have copyrights in all of your files.

> +static void bind_port_peer(struct port *p, int domid, int port)
> +{
> +    struct domain *domain;
> +    struct port *o;
> +    char *msg = "ok";
>   

Should be const char *.

> +    domain = get_domain(domid);
> +    o = domain->p+port;
> +    if (!o->priv) {
> +	msg = "peer not allocated";
> +    } else if (o->peer) {
> +	msg = "peer already bound";
> +    } else if (p->peer) {
> +	msg = "port already bound";
> +    } else {
> +	o->peer = p;
> +	p->peer = o;
> +    }
>   

Watch the whitespace (you've got tabs all over).

> +    if (debug)
> +	fprintf(stderr, "xen ev:%3d: bind port %d domain %d  <->  port %d domain %d : %s\n",
> +		p->priv->fd_read,
> +		p->port, p->priv->domain->domid,
> +		port, domid, msg);
> +    put_domain(domain);
> +}
> +
> +static void unbind_port(struct port *p)
> +{
> +    struct port *o;
> +
> +    o = p->peer;
> +    if (o) {
> +	if (debug)
> +	    fprintf(stderr,"xen ev:%3d: unbind port %d domain %d  <->  port %d domain %d\n",
> +		    p->priv->fd_read,
> +		    p->port, p->priv->domain->domid,
> +		    o->port, o->priv->domain->domid);
> +	o->peer = NULL;
> +	p->peer = NULL;
> +    }
> +}
> +
> +static void notify_send_peer(struct port *peer)
> +{
> +    uint32_t evtchn = peer->port;
> +    write(peer->priv->fd_write, &evtchn, sizeof(evtchn));
>   

Should be unix_write and should deal with errors.

Regards,

Anthony Liguori