# HG changeset patch
# User [email protected]
# Node ID 10c66e0408d1b22db15b8943223f1b6d7713422d
# Parent f5f32dc60121c32fab158a814c914aae3b77ba06
Module that exports Xen Hypervisor attributes to /sys/hypervisor.
signed-off-by: Mike D. Day <[email protected]>
diff -r f5f32dc60121 -r 10c66e0408d1 linux-2.6-xen-sparse/drivers/xen/core/Makefile
--- a/linux-2.6-xen-sparse/drivers/xen/core/Makefile Tue Feb 21 08:12:11 2006 -0500
+++ b/linux-2.6-xen-sparse/drivers/xen/core/Makefile Tue Feb 21 08:13:00 2006 -0500
@@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += xen_proc.o
obj-$(CONFIG_PROC_FS) += xen_proc.o
obj-$(CONFIG_NET) += skbuff.o
obj-$(CONFIG_SMP) += smpboot.o
+obj-$(CONFIG_XEN_SYSFS) += xen_sysfs.o
diff -r f5f32dc60121 -r 10c66e0408d1 linux-2.6-xen-sparse/drivers/xen/core/xen_sysfs.c
--- /dev/null Thu Jan 1 00:00:00 1970 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/core/xen_sysfs.c Tue Feb 21 08:13:00 2006 -0500
@@ -0,0 +1,411 @@
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+
+#include <xen/xen_sysfs.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mike D. Day <[email protected]>");
+
+decl_subsys(hypervisor, NULL, NULL);
+
+struct kset xen_kset = {
+ .subsys = &hypervisor_subsys,
+ .kobj = {
+ .name = "xen",
+ },
+};
+
+static ssize_t xen_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ struct xen_attr * xattr;
+ xattr = container_of(attr, struct xen_attr, attr);
+ if (xattr->show)
+ return xattr->show(kobj, attr, page);
+ return 0;
+}
+
+static void kobj_release(struct kobject * kobj)
+{
+ kfree(kobj);
+}
+
+static struct sysfs_ops xen_ops = {
+
+ .show = xen_show,
+};
+
+/* xen version attributes */
+
+static ssize_t major_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int version = HYPERVISOR_xen_version(XENVER_version, NULL);
+ if (version)
+ return sprintf(page, "%d\n", version >> 16);
+ return 0;
+}
+
+static struct xen_attr major = __ATTR_RO(major);
+
+static ssize_t minor_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int version = HYPERVISOR_xen_version(XENVER_version, NULL);
+ if (version)
+ return sprintf(page, "%d\n", version & 0xff);
+ return 0;
+}
+
+static struct xen_attr minor = __ATTR_RO(minor);
+
+static ssize_t extra_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ char extra_ver[XENVER_EXTRAVERSION_LEN];
+ int err = HYPERVISOR_xen_version(XENVER_extraversion, extra_ver);
+ if (0 == err)
+ return sprintf(page, "%s\n", extra_ver);
+ return 0;
+}
+
+static struct xen_attr extra = __ATTR_RO(extra);
+
+static struct attribute * version_attrs[] = {
+ &major.attr,
+ &minor.attr,
+ &extra.attr,
+ NULL
+};
+
+static struct attribute_group version_group = {
+ .name = "version",
+ .attrs = version_attrs,
+};
+
+static struct kobj_type xen_version_type = {
+ .release = kobj_release,
+ .sysfs_ops = &xen_ops,
+ .default_attrs = version_attrs,
+};
+
+static struct kobject * v_kobj;
+
+int __init
+xen_version_init(void)
+{
+ int err = -ENOMEM;
+ v_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
+ if (v_kobj) {
+ kobject_set_name(v_kobj, "version");
+ v_kobj->ktype = &xen_version_type;
+ kobject_init(v_kobj);
+ v_kobj->dentry = xen_kset.kobj.dentry;
+ err = sysfs_create_group(v_kobj, &version_group);
+ }
+ return err;
+}
+
+static void xen_version_destroy(void)
+{
+ sysfs_remove_group(v_kobj, &version_group);
+ kobject_put(v_kobj);
+ return;
+}
+
+
+
+/* xen compilation attributes */
+
+static ssize_t compiler_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_compile_info * info =
+ kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
+ if (info ) {
+ if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
+ err = sprintf(page, "%s\n", info->compiler);
+ kfree(info);
+ }
+ return err;
+}
+
+static struct xen_attr compiler = __ATTR_RO(compiler);
+
+static ssize_t by_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_compile_info * info =
+ kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
+ if (info ) {
+ if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
+ return sprintf(page, "%s\n", info->compile_by);
+ kfree(info);
+ }
+ return err;
+}
+
+static struct xen_attr compiled_by = __ATTR_RO(by);
+
+static ssize_t date_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_compile_info * info =
+ kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
+ if (info ) {
+ if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
+ return sprintf(page, "%s\n", info->compile_date);
+ kfree(info);
+ }
+ return err;
+}
+
+static struct xen_attr compiled_date = __ATTR_RO(date);
+
+static struct attribute * xen_compile_attrs[] = {
+ &compiler.attr,
+ &compiled_by.attr,
+ &compiled_date.attr,
+ NULL
+};
+
+static struct attribute_group xen_compilation_group = {
+ .name = "compilation",
+ .attrs = xen_compile_attrs,
+};
+
+static struct kobj_type xen_compilation_type = {
+ .release = kobj_release,
+ .sysfs_ops = &xen_ops,
+ .default_attrs = xen_compile_attrs,
+};
+
+static struct kobject * c_kobj;
+
+
+int __init
+static xen_compilation_init(void)
+{
+ int err = -ENOMEM;
+ c_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
+ if (c_kobj) {
+ kobject_set_name(c_kobj, "compilation");
+ c_kobj->ktype = &xen_compilation_type;
+ kobject_init(c_kobj);
+ c_kobj->dentry = xen_kset.kobj.dentry;
+ err = sysfs_create_group(c_kobj, &xen_compilation_group);
+ }
+ return err;
+}
+
+static void xen_compilation_destroy(void)
+{
+ sysfs_remove_group(c_kobj, &xen_compilation_group);
+ kobject_put(c_kobj);
+ return;
+}
+
+
+/* xen properties info */
+
+static ssize_t capabilities_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ char * caps = kmalloc(XENVER_CAPABILITIES_INFO_LEN, GFP_KERNEL);
+ if (caps) {
+ if (0 == HYPERVISOR_xen_version(XENVER_capabilities, caps))
+ err = sprintf(page, "%s\n", caps);
+ kfree(caps);
+ }
+ return err;
+}
+
+static struct xen_attr xen_capable = __ATTR_RO(capabilities);
+
+static ssize_t changeset_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ char * cset = kmalloc(XENVER_CSET_INFO_LEN, GFP_KERNEL);
+ if (cset) {
+ if (0 == HYPERVISOR_xen_version(XENVER_changeset, cset))
+ err = sprintf(page, "%s\n", cset);
+ kfree(cset);
+ }
+ return err;
+}
+
+static struct xen_attr xen_cset = __ATTR_RO(changeset);
+
+static ssize_t virt_start_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_platform_parameters * parms =
+ kmalloc(sizeof(struct xen_platform_parameters), GFP_KERNEL);
+ if (parms) {
+ if (0 == HYPERVISOR_xen_version(XENVER_platform_parameters,
+ parms))
+ err = sprintf(page, "%lx\n", parms->virt_start);
+ kfree(parms);
+ }
+ return err;
+}
+
+static struct xen_attr xen_virt_start = __ATTR_RO(virt_start);
+
+
+static ssize_t writable_pt_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_feature_info * info =
+ kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
+ if (info) {
+ info->submap_idx = XENFEAT_writable_page_tables;
+ if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
+ err = sprintf(page, "%d\n", info->submap);
+ kfree(info);
+ }
+ return err;
+}
+
+static struct xen_attr xen_wpt = __ATTR_RO(writable_pt);
+
+static ssize_t writable_dt_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_feature_info * info =
+ kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
+ if (info) {
+ info->submap_idx = XENFEAT_writable_descriptor_tables;
+ if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
+ err = sprintf(page, "%d\n", info->submap);
+ kfree(info);
+ }
+ return err;
+}
+
+static struct xen_attr xen_wdt = __ATTR_RO(writable_dt);
+
+static ssize_t translated_pm_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ int err = 0;
+ struct xen_feature_info * info =
+ kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
+ if (info) {
+ info->submap_idx = XENFEAT_auto_translated_physmap;
+ if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
+ err = sprintf(page, "%d\n", info->submap);
+ kfree(info);
+ }
+ return err;
+}
+
+static struct xen_attr xen_atp = __ATTR_RO(translated_pm);
+
+static struct attribute * xen_properties_attrs[] = {
+ &xen_capable.attr,
+ &xen_cset.attr,
+ &xen_virt_start.attr,
+ &xen_wpt.attr,
+ &xen_wdt.attr,
+ &xen_atp.attr,
+ NULL
+};
+
+static struct attribute_group xen_properties_group = {
+ .name = "properties",
+ .attrs = xen_properties_attrs,
+};
+
+static struct kobj_type xen_properties_type = {
+ .release = kobj_release,
+ .sysfs_ops = &xen_ops,
+ .default_attrs = xen_properties_attrs,
+};
+
+static struct kobject * p_kobj;
+
+static int __init
+xen_properties_init(void)
+{
+ int err = -ENOMEM;
+ p_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
+ if (p_kobj) {
+ kobject_set_name(p_kobj, "properties");
+ p_kobj->ktype = &xen_properties_type;
+ kobject_init(p_kobj);
+ p_kobj->dentry = xen_kset.kobj.dentry;
+ err = sysfs_create_group(p_kobj, &xen_properties_group);
+ }
+ return err;
+}
+
+static void xen_properties_destroy(void)
+{
+ sysfs_remove_group(p_kobj, &xen_properties_group);
+ kobject_put(p_kobj);
+}
+
+
+static int __init
+hyper_sysfs_init(void)
+{
+ __label__ out;
+
+ int err = subsystem_register(&hypervisor_subsys);
+ if (err)
+ goto out;
+ err = kset_register(&xen_kset);
+ if (err)
+ goto out;
+ err = xen_version_init();
+ if (err)
+ goto out;
+ err = xen_compilation_init();
+ if (err)
+ goto out;
+ err = xen_properties_init();
+
+out:
+ return err;
+}
+
+
+static void hyper_sysfs_exit(void)
+{
+ xen_properties_destroy();
+ xen_compilation_destroy();
+ xen_version_destroy();
+ kset_unregister(&xen_kset);
+ subsystem_unregister(&hypervisor_subsys);
+}
+
+
+module_init(hyper_sysfs_init);
+module_exit(hyper_sysfs_exit);
+
diff -r f5f32dc60121 -r 10c66e0408d1 linux-2.6-xen-sparse/include/xen/xen_sysfs.h
--- /dev/null Thu Jan 1 00:00:00 1970 +0000
+++ b/linux-2.6-xen-sparse/include/xen/xen_sysfs.h Tue Feb 21 08:13:00 2006 -0500
@@ -0,0 +1,30 @@
+/*
+ copyright (c) 2006 IBM Corporation
+ Authored by: Mike D. Day <[email protected]>
+
+ 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.
+*/
+
+
+
+#ifndef _XEN_SYSFS_H_
+#define _XEN_SYSFS_H_
+
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <asm/hypercall.h>
+#include <xen/interface/version.h>
+
+
+struct xen_attr {
+ struct attribute attr;
+ ssize_t (*show)(struct kobject *, struct attribute *, char *);
+ ssize_t (*store)(struct kobject *, struct attribute *, char *);
+};
+
+extern int HYPERVISOR_xen_version(int, void*);
+
+#endif /* _XEN_SYSFS_H_ */
--
On Tue, 2006-02-21 at 09:40 -0500, Mike D. Day wrote:
> Module that exports Xen Hypervisor attributes to /sys/hypervisor.
This set is looking much better. The functions are all quite small.
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Mike D. Day <[email protected]>");
> +
> +decl_subsys(hypervisor, NULL, NULL);
> +
> +struct kset xen_kset = {
> + .subsys = &hypervisor_subsys,
> + .kobj = {
> + .name = "xen",
> + },
> +};
I'm wondering if this information couldn't be laid out in a slightly
more generic way so that other hypervisors could use the same layout.
Instead of:
+---sys
+---hypervisor
+---xen
+---version
+---major
+---minor
+---extra
It could be:
+---sys
+---hypervisor
+---type
+---version
+---major
+---minor
+---extra
Where cat /sys/hypervisor/type is 'xen'. That way, if there are generic
things about hypervisors to export here, any generic tools can find them
without having to know exactly which hypervisor is running.
You can also set the standard that any other hypervisor has to
follow! :)
> +static ssize_t xen_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + struct xen_attr * xattr;
The usual kernel coding style is to put the '*' next to the variable
name:
struct xen_attr *xattr;
I'd probably take a good look through the driver and get rid of those.
Another minor nit: your "page" variable could become a very bad name if
we ever decide to give sysfs more or less than a page of buffer space
for its attributes.
> +static void kobj_release(struct kobject * kobj)
> +{
> + kfree(kobj);
> +}
This is a pretty generic function name. I know it is static, but it
might be useful to give it a slightly more descriptive name. It is
possible that there may be a global function named kobj_release() at
some point in the future, and you don't want a conflict.
> +static struct sysfs_ops xen_ops = {
> +
> + .show = xen_show,
> +};
> +
> +/* xen version attributes */
> +
> +static ssize_t major_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int version = HYPERVISOR_xen_version(XENVER_version, NULL);
> + if (version)
> + return sprintf(page, "%d\n", version >> 16);
> + return 0;
> +}
> +
> +static struct xen_attr major = __ATTR_RO(major);
> +
> +static ssize_t minor_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int version = HYPERVISOR_xen_version(XENVER_version, NULL);
> + if (version)
> + return sprintf(page, "%d\n", version & 0xff);
> + return 0;
> +}
> +
> +static struct xen_attr minor = __ATTR_RO(minor);
> +
> +static ssize_t extra_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + char extra_ver[XENVER_EXTRAVERSION_LEN];
At 1k, this is a wee bit too big to be on the stack. I'd just kmalloc
it instead.
> + int err = HYPERVISOR_xen_version(XENVER_extraversion, extra_ver);
> + if (0 == err)
> + return sprintf(page, "%s\n", extra_ver);
> + return 0;
> +}
Again, these names are a bit generic, but it probably isn't too harmful.
I'd mostly be worried that they would be hard to find if you were
cscope'ing.
> +/* xen compilation attributes */
> +
> +static ssize_t compiler_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_compile_info * info =
> + kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
> + if (info ) {
> + if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
> + err = sprintf(page, "%s\n", info->compiler);
> + kfree(info);
> + }
> + return err;
> +}
> +
> +static struct xen_attr compiler = __ATTR_RO(compiler);
> +
> +static ssize_t by_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
OK, one last thing about the function names:
xen_compiled_by_show() is a _lot_ more informative than by_show(). I
had to think about what this did, I didn't quite know just from the
name.
> + int err = 0;
> + struct xen_compile_info * info =
> + kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
> + if (info ) {
> + if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
> + return sprintf(page, "%s\n", info->compile_by);
I think this was mentioned last time, but the '0 ==' stuff is a little
non-conventional. I'd probably kill it just for readability-sake.
> +static struct attribute * xen_compile_attrs[] = {
> + &compiler.attr,
> + &compiled_by.attr,
> + &compiled_date.attr,
> + NULL
> +};
I like this variable name. A bit more complete than the function names.
> +static struct kobject * c_kobj;
> +
> +
Little bit of extra whitespace here.
> +int __init
> +static xen_compilation_init(void)
> +{
> + int err = -ENOMEM;
> + c_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
> + if (c_kobj) {
> + kobject_set_name(c_kobj, "compilation");
> + c_kobj->ktype = &xen_compilation_type;
> + kobject_init(c_kobj);
> + c_kobj->dentry = xen_kset.kobj.dentry;
> + err = sysfs_create_group(c_kobj, &xen_compilation_group);
> + }
> + return err;
> +}
> +
> +static void xen_compilation_destroy(void)
> +{
> + sysfs_remove_group(c_kobj, &xen_compilation_group);
> + kobject_put(c_kobj);
> + return;
> +}
Yup, this is another excellent function name.
-- Dave
Dave Hansen wrote:
> It could be:
>
> +---sys
> +---hypervisor
> +---type
> +---version
> +---major
> +---minor
> +---extra
>
Yeah, the type file could be useful.
>> +static ssize_t extra_show(struct kobject * kobj,
>> + struct attribute * attr,
>> + char * page)
>> +{
>> + char extra_ver[XENVER_EXTRAVERSION_LEN];
>
> At 1k, this is a wee bit too big to be on the stack. I'd just kmalloc
> it instead.
This particular constant is 16 bytes so I'll leave it as is. The other
one (CAPABILTIES_INFO), which is 1k, I did kmalloc as you suggested.
>> +static void xen_compilation_destroy(void)
>> +{
>> + sysfs_remove_group(c_kobj, &xen_compilation_group);
>> + kobject_put(c_kobj);
>> + return;
>> +}
>
> Yup, this is another excellent function name.
I'll change the others to be as descriptive. Thanks,
Mike Day
Last time I checked sizeof(char) was 1 so the kcalloc sizeof(char) is
excessive and we als have kzalloc() which would be slightly cleaner.
Otherwise looks sane to me
Alan
No full description explaining why you need this?
No "Signed-off-by:"?
On Tue, Feb 21, 2006 at 09:40:02AM -0500, Mike D. Day wrote:
> # HG changeset patch
> # User [email protected]
> # Node ID 10c66e0408d1b22db15b8943223f1b6d7713422d
> # Parent f5f32dc60121c32fab158a814c914aae3b77ba06
> Module that exports Xen Hypervisor attributes to /sys/hypervisor.
>
> signed-off-by: Mike D. Day <[email protected]>
>
> diff -r f5f32dc60121 -r 10c66e0408d1
> linux-2.6-xen-sparse/drivers/xen/core/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/core/Makefile Tue Feb 21 08:12:11
> 2006 -0500
Linewrapped :(
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/Makefile Tue Feb 21 08:13:00
> 2006 -0500
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += xen_proc.o
> obj-$(CONFIG_PROC_FS) += xen_proc.o
> obj-$(CONFIG_NET) += skbuff.o
> obj-$(CONFIG_SMP) += smpboot.o
> +obj-$(CONFIG_XEN_SYSFS) += xen_sysfs.o
leading spaces eaten by your email client. It looks like it was hungry
this morning...
> diff -r f5f32dc60121 -r 10c66e0408d1
> linux-2.6-xen-sparse/drivers/xen/core/xen_sysfs.c
> --- /dev/null Thu Jan 1 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/xen_sysfs.c Tue Feb 21
> 08:13:00 2006 -0500
> @@ -0,0 +1,411 @@
> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +
> +#include <xen/xen_sysfs.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Mike D. Day <[email protected]>");
> +
> +decl_subsys(hypervisor, NULL, NULL);
This should go into a separate file, along with it's registration, so
that other hypervisors can have access to it, right?
> +struct kset xen_kset = {
> + .subsys = &hypervisor_subsys,
> + .kobj = {
> + .name = "xen",
> + },
> +};
I thought I asked you last time to not create a "bare" kset. Any reason
why you didn't like that suggestion?
> +static struct sysfs_ops xen_ops = {
> +
> + .show = xen_show,
> +};
You will _never_ have a store attribute for xen? And why are you
passing the attribute field to your show function if you never use it?
> +/* xen version attributes */
> +
> +static ssize_t major_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int version = HYPERVISOR_xen_version(XENVER_version, NULL);
> + if (version)
> + return sprintf(page, "%d\n", version >> 16);
> + return 0;
> +}
Shouldn't you return an error if you could not get the version?
Otherwise your userspace tools will not know something is wrong.
You do this for all of your show attributes...
> +static ssize_t extra_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + char extra_ver[XENVER_EXTRAVERSION_LEN];
> + int err = HYPERVISOR_xen_version(XENVER_extraversion, extra_ver);
> + if (0 == err)
> + return sprintf(page, "%s\n", extra_ver);
> + return 0;
> +}
What's with the (0 == err)? You don't do that above in the other
functions, so please be consistant. Putting a value on the left side is
not normal kernel coding style.
> +static struct kobject * v_kobj;
> +
> +int __init
> +xen_version_init(void)
> +{
> + int err = -ENOMEM;
> + v_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
kzalloc please.
> + if (v_kobj) {
> + kobject_set_name(v_kobj, "version");
> + v_kobj->ktype = &xen_version_type;
> + kobject_init(v_kobj);
> + v_kobj->dentry = xen_kset.kobj.dentry;
> + err = sysfs_create_group(v_kobj, &version_group);
> + }
> + return err;
> +}
> +
> +
> +static ssize_t compiler_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_compile_info * info =
> + kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
> + if (info ) {
> + if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
> + err = sprintf(page, "%s\n", info->compiler);
Hey, look a different way to check the return value... Are you sure only
1 person wrote this file, and not 3 different authors? :)
> + kfree(info);
> + }
> + return err;
> +}
> +
> +static struct attribute_group xen_compilation_group = {
> + .name = "compilation",
> + .attrs = xen_compile_attrs,
> +};
> +
Trailing whitespace. This is also in other parts of the patch, please
do not do that.
> +static struct kobj_type xen_compilation_type = {
> + .release = kobj_release,
> + .sysfs_ops = &xen_ops,
> + .default_attrs = xen_compile_attrs,
> +};
> +
> +static struct kobject * c_kobj;
I'm all for descriptive variable names, but unfortunatly, this does not
qualify...
> +int __init
> +static xen_compilation_init(void)
> +{
> + int err = -ENOMEM;
> + c_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
> + if (c_kobj) {
> + kobject_set_name(c_kobj, "compilation");
> + c_kobj->ktype = &xen_compilation_type;
> + kobject_init(c_kobj);
> + c_kobj->dentry = xen_kset.kobj.dentry;
> + err = sysfs_create_group(c_kobj, &xen_compilation_group);
> + }
> + return err;
> +}
No, just name the attribute group. Then you don't have to create a
kobject at all. Same goes for all of the different attribute groups...
> +
> +static void xen_compilation_destroy(void)
> +{
> + sysfs_remove_group(c_kobj, &xen_compilation_group);
> + kobject_put(c_kobj);
> + return;
> +}
> +
> +
> +/* xen properties info */
> +
> +static ssize_t capabilities_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + char * caps = kmalloc(XENVER_CAPABILITIES_INFO_LEN, GFP_KERNEL);
> + if (caps) {
> + if (0 == HYPERVISOR_xen_version(XENVER_capabilities, caps))
> + err = sprintf(page, "%s\n", caps);
> + kfree(caps);
> + }
> + return err;
> +}
> +
> +static struct xen_attr xen_capable = __ATTR_RO(capabilities);
> +
> +static ssize_t changeset_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + char * cset = kmalloc(XENVER_CSET_INFO_LEN, GFP_KERNEL);
> + if (cset) {
> + if (0 == HYPERVISOR_xen_version(XENVER_changeset, cset))
> + err = sprintf(page, "%s\n", cset);
> + kfree(cset);
> + }
> + return err;
> +}
> +
> +static struct xen_attr xen_cset = __ATTR_RO(changeset);
> +
> +static ssize_t virt_start_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_platform_parameters * parms =
> + kmalloc(sizeof(struct xen_platform_parameters), GFP_KERNEL);
> + if (parms) {
> + if (0 == HYPERVISOR_xen_version(XENVER_platform_parameters,
> + parms))
> + err = sprintf(page, "%lx\n", parms->virt_start);
> + kfree(parms);
> + }
> + return err;
> +}
> +
> +static struct xen_attr xen_virt_start = __ATTR_RO(virt_start);
> +
> +
> +static ssize_t writable_pt_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_feature_info * info =
> + kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
> + if (info) {
> + info->submap_idx = XENFEAT_writable_page_tables;
> + if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
> + err = sprintf(page, "%d\n", info->submap);
> + kfree(info);
> + }
> + return err;
> +}
> +
> +static struct xen_attr xen_wpt = __ATTR_RO(writable_pt);
> +
> +static ssize_t writable_dt_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_feature_info * info =
> + kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
> + if (info) {
> + info->submap_idx = XENFEAT_writable_descriptor_tables;
> + if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
> + err = sprintf(page, "%d\n", info->submap);
> + kfree(info);
> + }
> + return err;
> +}
> +
> +static struct xen_attr xen_wdt = __ATTR_RO(writable_dt);
> +
> +static ssize_t translated_pm_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_feature_info * info =
> + kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
> + if (info) {
> + info->submap_idx = XENFEAT_auto_translated_physmap;
> + if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
> + err = sprintf(page, "%d\n", info->submap);
> + kfree(info);
> + }
> + return err;
> +}
> +
> +static struct xen_attr xen_atp = __ATTR_RO(translated_pm);
> +
> +static struct attribute * xen_properties_attrs[] = {
> + &xen_capable.attr,
> + &xen_cset.attr,
> + &xen_virt_start.attr,
> + &xen_wpt.attr,
> + &xen_wdt.attr,
> + &xen_atp.attr,
> + NULL
> +};
> +
> +static struct attribute_group xen_properties_group = {
> + .name = "properties",
> + .attrs = xen_properties_attrs,
> +};
> +
> +static struct kobj_type xen_properties_type = {
> + .release = kobj_release,
> + .sysfs_ops = &xen_ops,
> + .default_attrs = xen_properties_attrs,
> +};
> +
> +static struct kobject * p_kobj;
> +
> +static int __init
> +xen_properties_init(void)
> +{
> + int err = -ENOMEM;
> + p_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
> + if (p_kobj) {
> + kobject_set_name(p_kobj, "properties");
> + p_kobj->ktype = &xen_properties_type;
> + kobject_init(p_kobj);
> + p_kobj->dentry = xen_kset.kobj.dentry;
> + err = sysfs_create_group(p_kobj, &xen_properties_group);
> + }
> + return err;
> +}
> +
> +static void xen_properties_destroy(void)
> +{
> + sysfs_remove_group(p_kobj, &xen_properties_group);
> + kobject_put(p_kobj);
> +}
> +
> +
> +static int __init
> +hyper_sysfs_init(void)
> +{
> + __label__ out;
What is "__label__"?
> +
> + int err = subsystem_register(&hypervisor_subsys);
> + if (err)
> + goto out;
> + err = kset_register(&xen_kset);
> + if (err)
> + goto out;
> + err = xen_version_init();
> + if (err)
> + goto out;
> + err = xen_compilation_init();
> + if (err)
> + goto out;
> + err = xen_properties_init();
> +
> +out:
> + return err;
If you have an error, you do not back out the registration of any of the
other attributes. So, why have any error handling at all?
> +static void hyper_sysfs_exit(void)
> +{
> + xen_properties_destroy();
> + xen_compilation_destroy();
> + xen_version_destroy();
> + kset_unregister(&xen_kset);
> + subsystem_unregister(&hypervisor_subsys);
> +}
> +
> +
> +module_init(hyper_sysfs_init);
> +module_exit(hyper_sysfs_exit);
> +
> diff -r f5f32dc60121 -r 10c66e0408d1
> linux-2.6-xen-sparse/include/xen/xen_sysfs.h
> --- /dev/null Thu Jan 1 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/include/xen/xen_sysfs.h Tue Feb 21 08:13:00
> 2006 -0500
> @@ -0,0 +1,30 @@
> +/*
> + copyright (c) 2006 IBM Corporation
> + Authored by: Mike D. Day <[email protected]>
> +
> + 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.
> +*/
> +
> +
> +
> +#ifndef _XEN_SYSFS_H_
> +#define _XEN_SYSFS_H_
> +
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <asm/hypercall.h>
> +#include <xen/interface/version.h>
Why does this header file depend on the module, hypercall and version
header files?
> +
> +
> +struct xen_attr {
> + struct attribute attr;
> + ssize_t (*show)(struct kobject *, struct attribute *, char *);
> + ssize_t (*store)(struct kobject *, struct attribute *, char *);
> +};
Why export this structure?
Why even have this header file at all?
> +
> +extern int HYPERVISOR_xen_version(int, void*);
You don't have this in some xen header file? Shouldn't it go next to
all of the other HYPERVISOR calls?
Care to try again?
thanks,
greg k-h
> +static ssize_t by_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + int err = 0;
> + struct xen_compile_info * info =
> + kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
> + if (info ) {
> + if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
> + return sprintf(page, "%s\n", info->compile_by);
> + kfree(info);
> + }
> + return err;
> +}
Looks like you have a memory leak here. There's at least one more of the
same kind in your code.
Heiko
> I'm wondering if this information couldn't be laid out in a slightly
> more generic way so that other hypervisors could use the same layout.
> Instead of:
>
> +---sys
> +---hypervisor
> +---xen
> +---version
> +---major
> +---minor
> +---extra
>
> It could be:
>
> +---sys
> +---hypervisor
> +---type
> +---version
> +---major
> +---minor
> +---extra
>
> Where cat /sys/hypervisor/type is 'xen'. That way, if there are generic
> things about hypervisors to export here, any generic tools can find them
> without having to know exactly which hypervisor is running.
>
> You can also set the standard that any other hypervisor has to
> follow! :)
I doubt that there is much that different hypervisors can share.
Why should all this be visible for user space anyway? What's the purpose?
Heiko
Heiko Carstens wrote:
>> You can also set the standard that any other hypervisor has to
>> follow! :)
>
> I doubt that there is much that different hypervisors can share.
> Why should all this be visible for user space anyway? What's the purpose?
In Xen at least, hypervisor management and control programs run in user
space in a "privileged" domain (or virtual machine). Systems management
agents in user space on the privileged domain need to know this
information, and sysfs is a good place to expose it.
Mike
--
On Wed, 2006-02-22 at 07:37 -0500, Mike D. Day wrote:
> Heiko Carstens wrote:
> >> You can also set the standard that any other hypervisor has to
> >> follow! :)
> >
> > I doubt that there is much that different hypervisors can share.
> > Why should all this be visible for user space anyway? What's the purpose?
>
> In Xen at least, hypervisor management and control programs run in user
> space in a "privileged" domain (or virtual machine). Systems management
> agents in user space on the privileged domain need to know this
> information, and sysfs is a good place to expose it.
surely those tools already talk to the hypervisor.. so they might as
well ask for this information themselves... no need to bloat the kernel
for this
Arjan van de Ven wrote:
> surely those tools already talk to the hypervisor.. so they might as
> well ask for this information themselves... no need to bloat the kernel
> for this
Yes, it is an optional function. The current implementation is a module
that can be omitted from the configuration, built-in, or loadable.
Mike
On Wed, Feb 22, 2006 at 08:06:12AM -0500, Mike D. Day wrote:
> Arjan van de Ven wrote:
> >surely those tools already talk to the hypervisor.. so they might as
> >well ask for this information themselves... no need to bloat the kernel
> >for this
>
> Yes, it is an optional function. The current implementation is a module
> that can be omitted from the configuration, built-in, or loadable.
If it's not needed, why include it at all?
Heiko
Heiko Carstens wrote:
> On Wed, Feb 22, 2006 at 08:06:12AM -0500, Mike D. Day wrote:
>
> If it's not needed, why include it at all?
Sorry for not being clear. It *is* needed for control tools and agents
running in the privileged domain. Kernels running in unprivileged
domains will not use the module.
Mike
On Wed, 2006-02-22 at 08:43 -0500, Mike D. Day wrote:
> Heiko Carstens wrote:
> > On Wed, Feb 22, 2006 at 08:06:12AM -0500, Mike D. Day wrote:
> >
> > If it's not needed, why include it at all?
>
> Sorry for not being clear. It *is* needed for control tools and agents
> running in the privileged domain.
but again those tools and agents *already* have a way of talking to the
hypervisor themselves. Why can't they just first ask this info? Why does
that need to be in the kernel, in unswappable memory?
Arjan van de Ven wrote:
> but again those tools and agents *already* have a way of talking to the
> hypervisor themselves. Why can't they just first ask this info? Why does
> that need to be in the kernel, in unswappable memory?
Currently the two ways to get this data from user space are python via
xend, the xen control daemon, and through a C library call.
The two arguments for making some data available via sysfs are (1) to
support scripts and to (2) support efforts to slim down the required
user space tool stack.
There are alternatives for both arguments. To support scripting one
could add bindings (perl etc.) to the c library. Another alternative is
to write a succinct set of utility programs that call the c library and
invoke those utilities from scripts.
Neither of the above alternatives really help to slim down existing user
space tools, but on the other hand they don't materially add to the
problem either.
Sysfs is the simplest way to expose this info to user space. As an 8k
module it is pretty small. It fits well with convention because Xen
support is driver-like in the current linux patches. I think a xen sysfs
module is a reasonable solution. However I understand and agree with the
desire to keep unnecessary code out of the kernel.
Mike
Arjan van de Ven wrote:
> On Wed, 2006-02-22 at 08:43 -0500, Mike D. Day wrote:
>
>> Heiko Carstens wrote:
>>
>>> On Wed, Feb 22, 2006 at 08:06:12AM -0500, Mike D. Day wrote:
>>>
>>> If it's not needed, why include it at all?
>>>
>> Sorry for not being clear. It *is* needed for control tools and agents
>> running in the privileged domain.
>>
>
> but again those tools and agents *already* have a way of talking to the
> hypervisor themselves. Why can't they just first ask this info? Why does
> that need to be in the kernel, in unswappable memory?
>
Hypercalls have to be done in ring 0 for security reasons) There has to
be some kernel interface for making hypercalls.
The current interface is a ioctl() on a /proc file (which is awful).
The ioctl just pretty much passes 5 word arguments to the hypervisor.
It was suggested previously here that a hypercall pass-through interface
isn't the right approach. One suggestion that came up was a syscall
interface.
Also, there are some kernel-level drivers, like the memory ballooning
driver, that only exist in the kernel. Controlling the balloon driver
requires some sort of interface. That was the original point of this
effort (since it's currently exposed as a /proc file). I think it's
quite clear that the balloon driver should expose itself through sysfs
but I'm not personally convinced that this information (hypervisor
version information) ought to be exposed in sysfs.
Regards,
Anthony Liguori
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
On Wed, 2006-02-22 at 22:26 -0600, Anthony Liguori wrote:
> Arjan van de Ven wrote:
> > On Wed, 2006-02-22 at 08:43 -0500, Mike D. Day wrote:
> >
> >> Heiko Carstens wrote:
> >>
> >>> On Wed, Feb 22, 2006 at 08:06:12AM -0500, Mike D. Day wrote:
> >>>
> >>> If it's not needed, why include it at all?
> >>>
> >> Sorry for not being clear. It *is* needed for control tools and agents
> >> running in the privileged domain.
> >>
> >
> > but again those tools and agents *already* have a way of talking to the
> > hypervisor themselves. Why can't they just first ask this info? Why does
> > that need to be in the kernel, in unswappable memory?
> >
> Hypercalls have to be done in ring 0 for security reasons) There has to
> be some kernel interface for making hypercalls.
sure but you need that ANYWAY; the management tools will want a generic
"THIS hypercall" thing
>
> The current interface is a ioctl() on a /proc file (which is awful).
> The ioctl just pretty much passes 5 word arguments to the hypervisor.
> It was suggested previously here that a hypercall pass-through interface
> isn't the right approach. One suggestion that came up was a syscall
> interface.
yeah a syscall sounds right for this
> Also, there are some kernel-level drivers, like the memory ballooning
> driver, that only exist in the kernel. Controlling the balloon driver
> requires some sort of interface. That was the original point of this
> effort (since it's currently exposed as a /proc file). I think it's
> quite clear that the balloon driver should expose itself through sysfs
> but I'm not personally convinced that this information (hypervisor
> version information) ought to be exposed in sysfs.
that's a different thing; the ballooning driver obviously has to be in
kernel due to how it interacts with the VM, and having it's 1 or 2
controls in sysfs (hint: make it writable module parameters, so that the
defaults can be set at module load time, and you'll also get the rest
for free ;)
pure hypervisor info... no. "only" 8Kb of code + all data structures..
for something just as easily done in the management layer. In fact I
suspect the management layer will do the hypercalls anyway just because
it's easier to do than parsing sysfs !