[PATCH 0/6] V3, Move stuff out of xenfs

[PATCH 0/6] V3, Move stuff out of xenfs

[Bastian Blank]

Changes from v2:
- Rename xenbusd to xenbus_backend
- Register xenbus_backend device only if ring is local
- Use more correct error value: EPERM

Bastian

[PATCH 1/6] xen: Add privcmd device driver

[Bastian Blank]

Access to arbitrary hypercalls is currently provided via xenfs. This
adds a standard character device to handle this. The support in xenfs
remains for backward compatibility and uses the device driver code.

Signed-off-by: Bastian Blank <waldi@debian.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/Kconfig               |    7 ++++++
 drivers/xen/Makefile              |    2 +
 drivers/xen/{xenfs => }/privcmd.c |   39 ++++++++++++++++++++++++++++++++++++-
 drivers/xen/privcmd.h             |    3 ++
 drivers/xen/xenfs/Makefile        |    2 +-
 drivers/xen/xenfs/super.c         |    3 +-
 drivers/xen/xenfs/xenfs.h         |    1 -
 7 files changed, 53 insertions(+), 4 deletions(-)
 rename drivers/xen/{xenfs => }/privcmd.c (92%)
 create mode 100644 drivers/xen/privcmd.h

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 8795480..a1ced52 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -86,6 +86,7 @@ config XEN_BACKEND
 
 config XENFS
 	tristate "Xen filesystem"
+	select XEN_PRIVCMD
 	default y
 	help
 	  The xen filesystem provides a way for domains to share
@@ -171,4 +172,10 @@ config XEN_PCIDEV_BACKEND
 	  xen-pciback.hide=(03:00.0)(04:00.0)
 
 	  If in doubt, say m.
+
+config XEN_PRIVCMD
+	tristate
+	depends on XEN
+	default m
+
 endmenu
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 974fffd..aa31337 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -19,7 +19,9 @@ obj-$(CONFIG_XEN_TMEM)			+= tmem.o
 obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
 obj-$(CONFIG_XEN_DOM0)			+= pci.o
 obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
+obj-$(CONFIG_XEN_PRIVCMD)		+= xen-privcmd.o
 
 xen-evtchn-y				:= evtchn.o
 xen-gntdev-y				:= gntdev.o
 xen-gntalloc-y				:= gntalloc.o
+xen-privcmd-y				:= privcmd.o
diff --git a/drivers/xen/xenfs/privcmd.c b/drivers/xen/privcmd.c
similarity index 92%
rename from drivers/xen/xenfs/privcmd.c
rename to drivers/xen/privcmd.c
index dbd3b16..4e8d3da 100644
--- a/drivers/xen/xenfs/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -18,6 +19,7 @@
 #include <linux/highmem.h>
 #include <linux/pagemap.h>
 #include <linux/seq_file.h>
+#include <linux/miscdevice.h>
 
 #include <asm/pgalloc.h>
 #include <asm/pgtable.h>
@@ -32,6 +34,10 @@
 #include <xen/page.h>
 #include <xen/xen-ops.h>
 
+#include "privcmd.h"
+
+MODULE_LICENSE("GPL");
+
 #ifndef HAVE_ARCH_PRIVCMD_MMAP
 static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma);
 #endif
@@ -394,7 +400,38 @@ static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma)
 }
 #endif
 
-const struct file_operations privcmd_file_ops = {
+const struct file_operations xen_privcmd_fops = {
+	.owner = THIS_MODULE,
 	.unlocked_ioctl = privcmd_ioctl,
 	.mmap = privcmd_mmap,
 };
+EXPORT_SYMBOL_GPL(xen_privcmd_fops);
+
+static struct miscdevice privcmd_dev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "xen/privcmd",
+	.fops = &xen_privcmd_fops,
+};
+
+static int __init privcmd_init(void)
+{
+	int err;
+
+	if (!xen_domain())
+		return -ENODEV;
+
+	err = misc_register(&privcmd_dev);
+	if (err != 0) {
+		printk(KERN_ERR "Could not register Xen privcmd device\n");
+		return err;
+	}
+	return 0;
+}
+
+static void __exit privcmd_exit(void)
+{
+	misc_deregister(&privcmd_dev);
+}
+
+module_init(privcmd_init);
+module_exit(privcmd_exit);
diff --git a/drivers/xen/privcmd.h b/drivers/xen/privcmd.h
new file mode 100644
index 0000000..14facae
--- /dev/null
+++ b/drivers/xen/privcmd.h
@@ -0,0 +1,3 @@
+#include <linux/fs.h>
+
+extern const struct file_operations xen_privcmd_fops;
diff --git a/drivers/xen/xenfs/Makefile b/drivers/xen/xenfs/Makefile
index 4fde944..5d45ff1 100644
--- a/drivers/xen/xenfs/Makefile
+++ b/drivers/xen/xenfs/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_XENFS) += xenfs.o
 
-xenfs-y			  = super.o xenbus.o privcmd.o
+xenfs-y			  = super.o xenbus.o
 xenfs-$(CONFIG_XEN_DOM0) += xenstored.o
diff --git a/drivers/xen/xenfs/super.c b/drivers/xen/xenfs/super.c
index 1aa3897..a55fbf9 100644
--- a/drivers/xen/xenfs/super.c
+++ b/drivers/xen/xenfs/super.c
@@ -16,6 +16,7 @@
 #include <xen/xen.h>
 
 #include "xenfs.h"
+#include "../privcmd.h"
 
 #include <asm/xen/hypervisor.h>
 
@@ -84,7 +85,7 @@ static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
 		[1] = {},
 		{ "xenbus", &xenbus_file_ops, S_IRUSR|S_IWUSR },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &privcmd_file_ops, S_IRUSR|S_IWUSR },
+		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
 		{""},
 	};
 	int rc;
diff --git a/drivers/xen/xenfs/xenfs.h b/drivers/xen/xenfs/xenfs.h
index b68aa62..5056306 100644
--- a/drivers/xen/xenfs/xenfs.h
+++ b/drivers/xen/xenfs/xenfs.h
@@ -2,7 +2,6 @@
 #define _XENFS_XENBUS_H
 
 extern const struct file_operations xenbus_file_ops;
-extern const struct file_operations privcmd_file_ops;
 extern const struct file_operations xsd_kva_file_ops;
 extern const struct file_operations xsd_port_file_ops;
 
-- 
1.7.7.3

Re: [PATCH 1/6] xen: Add privcmd device driver

[Konrad Rzeszutek Wilk]

On Sat, Dec 10, 2011 at 07:29:46PM +0100, Bastian Blank wrote:
> Access to arbitrary hypercalls is currently provided via xenfs. This
> adds a standard character device to handle this. The support in xenfs
> remains for backward compatibility and uses the device driver code.

Bastian Blank (5):
      xen: Add privcmd device driver
      xen: Add xenbus device driver
      xen: Add xenbus_backend device
      xen/privcmd: Remove unused support for arch specific privcmp mmap
      xen/xenbus-frontend: Make error message more clear

Will wait for the re-worked sixth patch from you.

[PATCH 2/6] xen: Add xenbus device driver

[Bastian Blank]

Access to xenbus is currently handled via xenfs. This adds a device
driver for xenbus and makes xenfs use this code.

Signed-off-by: Bastian Blank <waldi@debian.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/xenbus/Makefile                        |    1 +
 drivers/xen/xenbus/xenbus_comms.h                  |    4 ++
 .../xenbus.c => xenbus/xenbus_dev_frontend.c}      |   37 ++++++++++++++++++--
 drivers/xen/xenfs/Makefile                         |    2 +-
 drivers/xen/xenfs/super.c                          |    3 +-
 drivers/xen/xenfs/xenfs.h                          |    1 -
 6 files changed, 42 insertions(+), 6 deletions(-)
 rename drivers/xen/{xenfs/xenbus.c => xenbus/xenbus_dev_frontend.c} (95%)

diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
index 8dca685..a2ea363 100644
--- a/drivers/xen/xenbus/Makefile
+++ b/drivers/xen/xenbus/Makefile
@@ -1,4 +1,5 @@
 obj-y	+= xenbus.o
+obj-y	+= xenbus_dev_frontend.o
 
 xenbus-objs =
 xenbus-objs += xenbus_client.o
diff --git a/drivers/xen/xenbus/xenbus_comms.h b/drivers/xen/xenbus/xenbus_comms.h
index c21db75..6e42800 100644
--- a/drivers/xen/xenbus/xenbus_comms.h
+++ b/drivers/xen/xenbus/xenbus_comms.h
@@ -31,6 +31,8 @@
 #ifndef _XENBUS_COMMS_H
 #define _XENBUS_COMMS_H
 
+#include <linux/fs.h>
+
 int xs_init(void);
 int xb_init_comms(void);
 
@@ -43,4 +45,6 @@ int xs_input_avail(void);
 extern struct xenstore_domain_interface *xen_store_interface;
 extern int xen_store_evtchn;
 
+extern const struct file_operations xen_xenbus_fops;
+
 #endif /* _XENBUS_COMMS_H */
diff --git a/drivers/xen/xenfs/xenbus.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
similarity index 95%
rename from drivers/xen/xenfs/xenbus.c
rename to drivers/xen/xenbus/xenbus_dev_frontend.c
index bbd000f..fb30cff 100644
--- a/drivers/xen/xenfs/xenbus.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -52,13 +52,16 @@
 #include <linux/namei.h>
 #include <linux/string.h>
 #include <linux/slab.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
 
-#include "xenfs.h"
-#include "../xenbus/xenbus_comms.h"
+#include "xenbus_comms.h"
 
 #include <xen/xenbus.h>
 #include <asm/xen/hypervisor.h>
 
+MODULE_LICENSE("GPL");
+
 /*
  * An element of a list of outstanding transactions, for which we're
  * still waiting a reply.
@@ -583,7 +586,7 @@ static unsigned int xenbus_file_poll(struct file *file, poll_table *wait)
 	return 0;
 }
 
-const struct file_operations xenbus_file_ops = {
+const struct file_operations xen_xenbus_fops = {
 	.read = xenbus_file_read,
 	.write = xenbus_file_write,
 	.open = xenbus_file_open,
@@ -591,3 +594,31 @@ const struct file_operations xenbus_file_ops = {
 	.poll = xenbus_file_poll,
 	.llseek = no_llseek,
 };
+EXPORT_SYMBOL_GPL(xen_xenbus_fops);
+
+static struct miscdevice xenbus_dev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "xen/xenbus",
+	.fops = &xen_xenbus_fops,
+};
+
+static int __init xenbus_init(void)
+{
+	int err;
+
+	if (!xen_domain())
+		return -ENODEV;
+
+	err = misc_register(&xenbus_dev);
+	if (err)
+		printk(KERN_ERR "Could not register xenbus device\n");
+	return err;
+}
+
+static void __exit xenbus_exit(void)
+{
+	misc_deregister(&xenbus_dev);
+}
+
+module_init(xenbus_init);
+module_exit(xenbus_exit);
diff --git a/drivers/xen/xenfs/Makefile b/drivers/xen/xenfs/Makefile
index 5d45ff1..b019865 100644
--- a/drivers/xen/xenfs/Makefile
+++ b/drivers/xen/xenfs/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_XENFS) += xenfs.o
 
-xenfs-y			  = super.o xenbus.o
+xenfs-y			  = super.o
 xenfs-$(CONFIG_XEN_DOM0) += xenstored.o
diff --git a/drivers/xen/xenfs/super.c b/drivers/xen/xenfs/super.c
index a55fbf9..a84b53c 100644
--- a/drivers/xen/xenfs/super.c
+++ b/drivers/xen/xenfs/super.c
@@ -17,6 +17,7 @@
 
 #include "xenfs.h"
 #include "../privcmd.h"
+#include "../xenbus/xenbus_comms.h"
 
 #include <asm/xen/hypervisor.h>
 
@@ -83,7 +84,7 @@ static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	static struct tree_descr xenfs_files[] = {
 		[1] = {},
-		{ "xenbus", &xenbus_file_ops, S_IRUSR|S_IWUSR },
+		{ "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
 		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
 		{""},
diff --git a/drivers/xen/xenfs/xenfs.h b/drivers/xen/xenfs/xenfs.h
index 5056306..6b80c77 100644
--- a/drivers/xen/xenfs/xenfs.h
+++ b/drivers/xen/xenfs/xenfs.h
@@ -1,7 +1,6 @@
 #ifndef _XENFS_XENBUS_H
 #define _XENFS_XENBUS_H
 
-extern const struct file_operations xenbus_file_ops;
 extern const struct file_operations xsd_kva_file_ops;
 extern const struct file_operations xsd_port_file_ops;
 
-- 
1.7.7.3

[PATCH 3/6] xen: Add xenbus_backend device

[Bastian Blank]

Access for xenstored to the event channel and pre-allocated ring is
managed via xenfs.  This adds its own character device featuring mmap
for the ring and an ioctl for the event channel.

Signed-off-by: Bastian Blank <waldi@debian.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/xenbus/Makefile             |    1 +
 drivers/xen/xenbus/xenbus_dev_backend.c |   89 +++++++++++++++++++++++++++++++
 include/xen/xenbus_dev.h                |   41 ++++++++++++++
 3 files changed, 131 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..31e2e90 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_BACKEND) += xenbus_dev_backend.o
 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..a2092bd
--- /dev/null
+++ b/drivers/xen/xenbus/xenbus_dev_backend.c
@@ -0,0 +1,89 @@
+#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 <linux/capability.h>
+
+#include <xen/page.h>
+#include <xen/xenbus_dev.h>
+
+#include "xenbus_comms.h"
+
+MODULE_LICENSE("GPL");
+
+static int xenbus_backend_open(struct inode *inode, struct file *filp)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	return nonseekable_open(inode, filp);
+}
+
+static long xenbus_backend_ioctl(struct file *file, unsigned int cmd, unsigned long data)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	switch (cmd) {
+		case IOCTL_XENBUS_BACKEND_EVTCHN:
+			if (xen_store_evtchn > 0)
+				return xen_store_evtchn;
+			return -ENODEV;
+
+		default:
+			return -ENOTTY;
+	}
+}
+
+static int xenbus_backend_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	size_t size = vma->vm_end - vma->vm_start;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	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 xenbus_backend_fops = {
+	.open = xenbus_backend_open,
+	.mmap = xenbus_backend_mmap,
+	.unlocked_ioctl = xenbus_backend_ioctl,
+};
+
+static struct miscdevice xenbus_backend_dev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "xen/xenbus_backend",
+	.fops = &xenbus_backend_fops,
+};
+
+static int __init xenbus_backend_init(void)
+{
+	int err;
+
+	if (!xen_initial_domain())
+		return -ENODEV;
+
+	err = misc_register(&xenbus_backend_dev);
+	if (err)
+		printk(KERN_ERR "Could not register xenbus backend device\n");
+	return err;
+}
+
+static void __exit xenbus_backend_exit(void)
+{
+	misc_deregister(&xenbus_backend_dev);
+}
+
+module_init(xenbus_backend_init);
+module_exit(xenbus_backend_exit);
diff --git a/include/xen/xenbus_dev.h b/include/xen/xenbus_dev.h
new file mode 100644
index 0000000..ac5f0fe
--- /dev/null
+++ b/include/xen/xenbus_dev.h
@@ -0,0 +1,41 @@
+/******************************************************************************
+ * evtchn.h
+ *
+ * Interface to /dev/xen/xenbus_backend.
+ *
+ * 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_XENBUS_BACKEND_EVTCHN			\
+	_IOC(_IOC_NONE, 'B', 0, 0)
+
+#endif /* __LINUX_XEN_XENBUS_DEV_H__ */
-- 
1.7.7.3

[PATCH 4/6] xen/privcmd: Remove unused support for arch specific privcmp mmap

[Bastian Blank]

This was used for ia64. But there is no working ia64 support in sight,
so remove it for now.

Signed-off-by: Bastian Blank <waldi@debian.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/privcmd.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 4e8d3da..ccee0f1 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -365,7 +365,6 @@ static long privcmd_ioctl(struct file *file,
 	return ret;
 }
 
-#ifndef HAVE_ARCH_PRIVCMD_MMAP
 static int privcmd_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	printk(KERN_DEBUG "privcmd_fault: vma=%p %lx-%lx, pgoff=%lx, uv=%p\n",
@@ -398,7 +397,6 @@ static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma)
 {
 	return (xchg(&vma->vm_private_data, (void *)1) == NULL);
 }
-#endif
 
 const struct file_operations xen_privcmd_fops = {
 	.owner = THIS_MODULE,
-- 
1.7.7.3

[PATCH 5/6] xen/xenbus-frontend: Make error message more clear

[Bastian Blank]

Add the work frontend to the error message because we now also have a
backend device.

Signed-off-by: Bastian Blank <waldi@debian.org>
---
 drivers/xen/xenbus/xenbus_dev_frontend.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index fb30cff..9f6be7d 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -611,7 +611,7 @@ static int __init xenbus_init(void)
 
 	err = misc_register(&xenbus_dev);
 	if (err)
-		printk(KERN_ERR "Could not register xenbus device\n");
+		printk(KERN_ERR "Could not register xenbus frontend device\n");
 	return err;
 }
 
-- 
1.7.7.3

Re: [Xen-devel] [PATCH 5/6] xen/xenbus-frontend: Make error message more clear

[Ian Campbell]

On Sat, 2011-12-10 at 18:29 +0000, Bastian Blank wrote:
> Add the work frontend to the error message because we now also have a
> backend device.
> 
> Signed-off-by: Bastian Blank <waldi@debian.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  drivers/xen/xenbus/xenbus_dev_frontend.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
> index fb30cff..9f6be7d 100644
> --- a/drivers/xen/xenbus/xenbus_dev_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
> @@ -611,7 +611,7 @@ static int __init xenbus_init(void)
>  
>  	err = misc_register(&xenbus_dev);
>  	if (err)
> -		printk(KERN_ERR "Could not register xenbus device\n");
> +		printk(KERN_ERR "Could not register xenbus frontend device\n");
>  	return err;
>  }
>  

[PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local

[Bastian Blank]

Signed-off-by: Bastian Blank <waldi@debian.org>
---
 drivers/xen/xenbus/Makefile             |    2 +-
 drivers/xen/xenbus/xenbus_dev_backend.c |   13 ++-----------
 drivers/xen/xenbus/xenbus_dev_backend.h |    2 ++
 drivers/xen/xenbus/xenbus_probe.c       |   18 ++++++++++++++++++
 4 files changed, 23 insertions(+), 12 deletions(-)
 create mode 100644 drivers/xen/xenbus/xenbus_dev_backend.h

diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
index 31e2e90..d751f45 100644
--- a/drivers/xen/xenbus/Makefile
+++ b/drivers/xen/xenbus/Makefile
@@ -7,8 +7,8 @@ xenbus-objs += xenbus_comms.o
 xenbus-objs += xenbus_xs.o
 xenbus-objs += xenbus_probe.o
 
+xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
 xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
 xenbus-objs += $(xenbus-be-objs-y)
 
-obj-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
 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
index a2092bd..fb87f1b 100644
--- a/drivers/xen/xenbus/xenbus_dev_backend.c
+++ b/drivers/xen/xenbus/xenbus_dev_backend.c
@@ -3,7 +3,6 @@
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
-#include <linux/module.h>
 #include <linux/capability.h>
 
 #include <xen/page.h>
@@ -11,8 +10,6 @@
 
 #include "xenbus_comms.h"
 
-MODULE_LICENSE("GPL");
-
 static int xenbus_backend_open(struct inode *inode, struct file *filp)
 {
 	if (!capable(CAP_SYS_ADMIN))
@@ -67,23 +64,17 @@ static struct miscdevice xenbus_backend_dev = {
 	.fops = &xenbus_backend_fops,
 };
 
-static int __init xenbus_backend_init(void)
+int __init xenbus_backend_init(void)
 {
 	int err;
 
-	if (!xen_initial_domain())
-		return -ENODEV;
-
 	err = misc_register(&xenbus_backend_dev);
 	if (err)
 		printk(KERN_ERR "Could not register xenbus backend device\n");
 	return err;
 }
 
-static void __exit xenbus_backend_exit(void)
+void __exit xenbus_backend_exit(void)
 {
 	misc_deregister(&xenbus_backend_dev);
 }
-
-module_init(xenbus_backend_init);
-module_exit(xenbus_backend_exit);
diff --git a/drivers/xen/xenbus/xenbus_dev_backend.h b/drivers/xen/xenbus/xenbus_dev_backend.h
new file mode 100644
index 0000000..d986853
--- /dev/null
+++ b/drivers/xen/xenbus/xenbus_dev_backend.h
@@ -0,0 +1,2 @@
+int xenbus_backend_init(void);
+void xenbus_backend_exit(void);
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 1b178c6..4eff095 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -60,6 +60,7 @@
 #include <xen/hvm.h>
 
 #include "xenbus_comms.h"
+#include "xenbus_dev_backend.h"
 #include "xenbus_probe.h"
 
 
@@ -641,6 +642,10 @@ EXPORT_SYMBOL_GPL(xenbus_dev_cancel);
 /* A flag to determine if xenstored is 'ready' (i.e. has started) */
 int xenstored_ready;
 
+/* A flag to determine if xenstored is 'local' */
+#ifdef CONFIG_XEN_BACKEND
+static int xenstored_local;
+#endif
 
 int register_xenstore_notifier(struct notifier_block *nb)
 {
@@ -675,6 +680,14 @@ static int __init xenbus_probe_initcall(void)
 	if (!xen_domain())
 		return -ENODEV;
 
+#ifdef CONFIG_XEN_BACKEND
+	if (xenstored_local) {
+		int ret = xenbus_backend_init();
+		if (ret)
+			return ret;
+	}
+#endif
+
 	if (xen_initial_domain() || xen_hvm_domain())
 		return 0;
 
@@ -747,9 +760,14 @@ static int __init xenbus_init(void)
 		if (xen_store_evtchn)
 			xenstored_ready = 1;
 		else {
+#ifdef CONFIG_XEN_BACKEND
+			xenstored_local = 1;
 			err = xenstored_local_init();
 			if (err)
 				goto out_error;
+#else
+			BUG();
+#endif
 		}
 		xen_store_interface = mfn_to_virt(xen_store_mfn);
 	}
-- 
1.7.7.3

Re: [Xen-devel] [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local

[Ian Campbell]

On Sat, 2011-12-10 at 18:29 +0000, Bastian Blank wrote:
> +/* A flag to determine if xenstored is 'local' */
> +#ifdef CONFIG_XEN_BACKEND
> +static int xenstored_local;
> +#endif

I think this can be __initdata since all the users are __init.

> +#ifdef CONFIG_XEN_BACKEND
> +			xenstored_local = 1;

I would push this down into xenstore_local_init() and only set it on
success.

>  			err = xenstored_local_init();

This is now only called if CONFIG_XEN_BACKEND. You should add a similar
#ifdef around the function definition.

Ian.

>  			if (err)
>  				goto out_error;
> +#else
> +			BUG();
> +#endif
>  		}
>  		xen_store_interface = mfn_to_virt(xen_store_mfn);
>  	}

Re: [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local

[Konrad Rzeszutek Wilk]

On Sat, Dec 10, 2011 at 07:29:51PM +0100, Bastian Blank wrote:
> Signed-off-by: Bastian Blank <waldi@debian.org>
> ---
>  drivers/xen/xenbus/Makefile             |    2 +-
>  drivers/xen/xenbus/xenbus_dev_backend.c |   13 ++-----------
>  drivers/xen/xenbus/xenbus_dev_backend.h |    2 ++
>  drivers/xen/xenbus/xenbus_probe.c       |   18 ++++++++++++++++++
>  4 files changed, 23 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/xen/xenbus/xenbus_dev_backend.h
> 
> diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
> index 31e2e90..d751f45 100644
> --- a/drivers/xen/xenbus/Makefile
> +++ b/drivers/xen/xenbus/Makefile
> @@ -7,8 +7,8 @@ xenbus-objs += xenbus_comms.o
>  xenbus-objs += xenbus_xs.o
>  xenbus-objs += xenbus_probe.o
>  
> +xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
>  xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
>  xenbus-objs += $(xenbus-be-objs-y)
>  
> -obj-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
>  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
> index a2092bd..fb87f1b 100644
> --- a/drivers/xen/xenbus/xenbus_dev_backend.c
> +++ b/drivers/xen/xenbus/xenbus_dev_backend.c
> @@ -3,7 +3,6 @@
>  #include <linux/mm.h>
>  #include <linux/fs.h>
>  #include <linux/miscdevice.h>
> -#include <linux/module.h>
>  #include <linux/capability.h>
>  
>  #include <xen/page.h>
> @@ -11,8 +10,6 @@
>  
>  #include "xenbus_comms.h"
>  
> -MODULE_LICENSE("GPL");
> -
>  static int xenbus_backend_open(struct inode *inode, struct file *filp)
>  {
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -67,23 +64,17 @@ static struct miscdevice xenbus_backend_dev = {
>  	.fops = &xenbus_backend_fops,
>  };
>  
> -static int __init xenbus_backend_init(void)
> +int __init xenbus_backend_init(void)
>  {
>  	int err;
>  
> -	if (!xen_initial_domain())
> -		return -ENODEV;
> -
>  	err = misc_register(&xenbus_backend_dev);
>  	if (err)
>  		printk(KERN_ERR "Could not register xenbus backend device\n");
>  	return err;
>  }
>  
> -static void __exit xenbus_backend_exit(void)
> +void __exit xenbus_backend_exit(void)
>  {
>  	misc_deregister(&xenbus_backend_dev);
>  }
> -
> -module_init(xenbus_backend_init);
> -module_exit(xenbus_backend_exit);

Why are we removing the module functionality?
Can't this [the purpose of this patch] be done while still maintaining the module functionality?

> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.h b/drivers/xen/xenbus/xenbus_dev_backend.h
> new file mode 100644
> index 0000000..d986853
> --- /dev/null
> +++ b/drivers/xen/xenbus/xenbus_dev_backend.h
> @@ -0,0 +1,2 @@
> +int xenbus_backend_init(void);
> +void xenbus_backend_exit(void);
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index 1b178c6..4eff095 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -60,6 +60,7 @@
>  #include <xen/hvm.h>
>  
>  #include "xenbus_comms.h"
> +#include "xenbus_dev_backend.h"
>  #include "xenbus_probe.h"
>  
>  
> @@ -641,6 +642,10 @@ EXPORT_SYMBOL_GPL(xenbus_dev_cancel);
>  /* A flag to determine if xenstored is 'ready' (i.e. has started) */
>  int xenstored_ready;
>  
> +/* A flag to determine if xenstored is 'local' */
> +#ifdef CONFIG_XEN_BACKEND
> +static int xenstored_local;

bool?

> +#endif
>  
>  int register_xenstore_notifier(struct notifier_block *nb)
>  {
> @@ -675,6 +680,14 @@ static int __init xenbus_probe_initcall(void)
>  	if (!xen_domain())
>  		return -ENODEV;
>  
> +#ifdef CONFIG_XEN_BACKEND
> +	if (xenstored_local) {
> +		int ret = xenbus_backend_init();
> +		if (ret)
> +			return ret;
> +	}
> +#endif
> +
>  	if (xen_initial_domain() || xen_hvm_domain())
>  		return 0;
>  
> @@ -747,9 +760,14 @@ static int __init xenbus_init(void)
>  		if (xen_store_evtchn)
>  			xenstored_ready = 1;
>  		else {
> +#ifdef CONFIG_XEN_BACKEND
> +			xenstored_local = 1;
>  			err = xenstored_local_init();
>  			if (err)
>  				goto out_error;
> +#else
> +			BUG();

No way. A WARN, sure - but BUG() is way too intense for this.

> +#endif
>  		}
>  		xen_store_interface = mfn_to_virt(xen_store_mfn);
>  	}
> -- 
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local

[Bastian Blank]

On Mon, Dec 12, 2011 at 10:42:27AM -0500, Konrad Rzeszutek Wilk wrote:
> On Sat, Dec 10, 2011 at 07:29:51PM +0100, Bastian Blank wrote:
> > -module_init(xenbus_backend_init);
> > -module_exit(xenbus_backend_exit);
> 
> Why are we removing the module functionality?
> Can't this [the purpose of this patch] be done while still maintaining the module functionality?

Not really. The purpose is to register the device only if the ring is
local. This information is not available to modules.

However we could just move the whole xenbus ring setup into a "module".
Or is there any reason for this to be in a postcore_initcall, even if it
is only operational after a userspace process works?

> >  int xenstored_ready;
> >  
> > +/* A flag to determine if xenstored is 'local' */
> > +#ifdef CONFIG_XEN_BACKEND
> > +static int xenstored_local;
> 
> bool?

Well. The other flags are int also.

> > +			BUG();
> No way. A WARN, sure - but BUG() is way too intense for this.

Okay.

Bastian

-- 
The sight of death frightens them [Earthers].
		-- Kras the Klingon, "Friday's Child", stardate 3497.2

Re: [Xen-devel] [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local

[Konrad Rzeszutek Wilk]

On Mon, Dec 12, 2011 at 07:28:07PM +0100, Bastian Blank wrote:
> On Mon, Dec 12, 2011 at 10:42:27AM -0500, Konrad Rzeszutek Wilk wrote:
> > On Sat, Dec 10, 2011 at 07:29:51PM +0100, Bastian Blank wrote:
> > > -module_init(xenbus_backend_init);
> > > -module_exit(xenbus_backend_exit);
> > 
> > Why are we removing the module functionality?
> > Can't this [the purpose of this patch] be done while still maintaining the module functionality?
> 
> Not really. The purpose is to register the device only if the ring is
> local. This information is not available to modules.
> 
> However we could just move the whole xenbus ring setup into a "module".
> Or is there any reason for this to be in a postcore_initcall, even if it
> is only operational after a userspace process works?

My thinking is that we do not want to use memory space if the kernel
is booted as baremetal (as there is no point of having Xen pieces around).
Modules work great for that.

But then part of this code is __init so some does get evicted, but not sure
how much. Could you measure this on baremetal please?

> 
> > >  int xenstored_ready;
> > >  
> > > +/* A flag to determine if xenstored is 'local' */
> > > +#ifdef CONFIG_XEN_BACKEND
> > > +static int xenstored_local;
> > 
> > bool?
> 
> Well. The other flags are int also.

Sure.. and they ought to be modified to bool too now that I think of it.

> 
> > > +			BUG();
> > No way. A WARN, sure - but BUG() is way too intense for this.
> 
> Okay.
> 
> Bastian
> 
> -- 
> The sight of death frightens them [Earthers].
> 		-- Kras the Klingon, "Friday's Child", stardate 3497.2