Re: [PATCH 5/5] xen: Add xenbusd device driver

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Sun, Nov 27, 2011 at 11:07:08PM +0100, Bastian Blank wrote:
> Access for xenstored to the event channel and pre-allocated ring is
> managed via xenfs.  This adds its own device driver featuring mmap for
> the ring and an ioctl for the event channel.
> 
> Signed-off-by: Bastian Blank <waldi@debian.org>
> ---
>  drivers/xen/xenbus/Makefile             |    1 +
>  drivers/xen/xenbus/xenbus_dev_backend.c |   79 +++++++++++++++++++++++++++++++
>  include/xen/xenbus_dev.h                |   41 ++++++++++++++++
>  3 files changed, 121 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/xen/xenbus/xenbus_dev_backend.c
>  create mode 100644 include/xen/xenbus_dev.h
> 
> diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
> index a2ea363..7e1aa85 100644
> --- a/drivers/xen/xenbus/Makefile
> +++ b/drivers/xen/xenbus/Makefile
> @@ -10,4 +10,5 @@ xenbus-objs += xenbus_probe.o
>  xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
>  xenbus-objs += $(xenbus-be-objs-y)
>  
> +obj-$(CONFIG_XEN_DOM0) += xenbus_dev_backend.o

I think this needs to depend on XEN_BACKEND ?

You could have a dom0 without any backends .. (Which is one of the goals
of disegragated device driver domains).

>  obj-$(CONFIG_XEN_XENBUS_FRONTEND) += xenbus_probe_frontend.o
> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c b/drivers/xen/xenbus/xenbus_dev_backend.c
> new file mode 100644
> index 0000000..5d77cee
> --- /dev/null
> +++ b/drivers/xen/xenbus/xenbus_dev_backend.c
> @@ -0,0 +1,79 @@
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/mm.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +
> +#include <xen/page.h>
> +#include <xen/xenbus_dev.h>
> +
> +#include "xenbus_comms.h"
> +
> +MODULE_LICENSE("GPL");
> +
> +static long xenbusd_ioctl(struct file *file, unsigned int cmd, unsigned long data)
> +{
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EACCES;
> +
> +	switch (cmd) {
> +		case IOCTL_XENBUSD_EVTCHN:
> +			if (xen_store_evtchn > 0)
> +				return xen_store_evtchn;
> +			return -EINVAL;

Not -ENODEV? After all, the command arguments were OK, it is just that
the eventchannel has not been set.

> +
> +		default:
> +			return -ENOTTY;
> +	}
> +}
> +
> +static int xenbusd_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	size_t size = vma->vm_end - vma->vm_start;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EACCES;
> +
> +	if ((size > PAGE_SIZE) || (vma->vm_pgoff != 0))
> +		return -EINVAL;
> +
> +	if (remap_pfn_range(vma, vma->vm_start,
> +			    virt_to_pfn(xen_store_interface),
> +			    size, vma->vm_page_prot))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +const struct file_operations xenbusd_fops = {
> +	.mmap = xenbusd_mmap,
> +	.unlocked_ioctl = xenbusd_ioctl,
> +};
> +
> +static struct miscdevice xenbusd_dev = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "xen/xenbusd",
> +	.fops = &xenbusd_fops,
> +};
> +
> +static int __init xenbusd_init(void)
> +{
> +	int err;
> +
> +	if (!xen_initial_domain())

With the disgregated domains (and the patches that Daniel posted), I
think this needs to relax a bit. Perhaps just make it 'xen_domain'?

Lets CC him here.
> +		return -ENODEV;
> +
> +	err = misc_register(&xenbusd_dev);
> +	if (err)
> +		printk(KERN_ERR "Could not register xenbus device\n");
> +	return err;
> +}
> +
> +static void __exit xenbusd_exit(void)
> +{
> +	misc_deregister(&xenbusd_dev);
> +}
> +
> +module_init(xenbusd_init);
> +module_exit(xenbusd_exit);
> diff --git a/include/xen/xenbus_dev.h b/include/xen/xenbus_dev.h
> new file mode 100644
> index 0000000..f551404
> --- /dev/null
> +++ b/include/xen/xenbus_dev.h
> @@ -0,0 +1,41 @@
> +/******************************************************************************
> + * evtchn.h
> + *
> + * Interface to /dev/xen/xenbusd.
> + *
> + * Copyright (c) 2011 Bastian Blank <waldi@debian.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the following license:
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this source file (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use, copy, modify,
> + * merge, publish, distribute, sublicense, and/or sell copies of the Software,
> + * and to permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef __LINUX_XEN_XENBUS_DEV_H__
> +#define __LINUX_XEN_XENBUS_DEV_H__
> +
> +#include <linux/ioctl.h>
> +
> +#define IOCTL_XENBUSD_EVTCHN				\
> +	_IOC(_IOC_NONE, 'X', 0, 0)

_IOC_READ ?

So why 'X', not 'B' for bus?

> +
> +#endif /* __LINUX_XEN_XENBUS_DEV_H__ */
> -- 
> 1.7.7.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel