* [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support
@ 2019-10-02 11:19 Juergen Gross
2019-10-02 11:19 ` [Xen-devel] [PATCH v2 1/6] docs: add feature document for Xen " Juergen Gross
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:19 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
On the 2019 Xen developer summit there was agreement that the Xen
hypervisor should gain support for a hierarchical name-value store
similar to the Linux kernel's sysfs.
This is a first implementation of that idea adding the basic
functionality to hypervisor and tools side. The interface to any
user program making use of that "xen-hypfs" is a new library
"libxenhypfs" with a stable interface.
Changes in V2:
- all comments to V1 addressed
- added man-page for xenhypfs tool
- added runtime parameter read access for string parameters
Changes in V1:
- renamed xenfs ->xenhypfs
- added writable entries support at the interface level and in the
xenhypfs tool
- added runtime parameter read access (integer type only for now)
- added docs/misc/hypfs-paths.pandoc for path descriptions
Juergen Gross (6):
docs: add feature document for Xen hypervisor sysfs-like support
xen: add basic hypervisor filesystem support
libs: add libxenhypfs
tools: add xenfs tool
xen: add /buildinfo/config entry to hypervisor filesystem
xen: add runtime parameter reading support to hypfs
.gitignore | 3 +
docs/features/hypervisorfs.pandoc | 87 ++++++++++
docs/man/xenhypfs.1.pod | 61 +++++++
docs/misc/hypfs-paths.pandoc | 113 ++++++++++++
tools/Rules.mk | 6 +
tools/libs/Makefile | 1 +
tools/libs/hypfs/Makefile | 14 ++
tools/libs/hypfs/core.c | 252 +++++++++++++++++++++++++++
tools/libs/hypfs/include/xenhypfs.h | 60 +++++++
tools/libs/hypfs/libxenhypfs.map | 9 +
tools/libs/hypfs/xenhypfs.pc.in | 10 ++
tools/misc/Makefile | 6 +
tools/misc/xenhypfs.c | 160 +++++++++++++++++
xen/arch/arm/traps.c | 1 +
xen/arch/x86/hvm/hypercall.c | 1 +
xen/arch/x86/hypercall.c | 1 +
xen/arch/x86/pv/hypercall.c | 1 +
xen/common/Makefile | 10 ++
xen/common/hypfs.c | 335 ++++++++++++++++++++++++++++++++++++
xen/common/kernel.c | 39 +++++
xen/include/public/errno.h | 1 +
xen/include/public/hypfs.h | 123 +++++++++++++
xen/include/public/xen.h | 1 +
xen/include/xen/hypercall.h | 8 +
xen/include/xen/hypfs.h | 40 +++++
xen/include/xen/kernel.h | 2 +
xen/tools/Makefile | 9 +-
xen/tools/bin2c.c | 28 +++
28 files changed, 1380 insertions(+), 2 deletions(-)
create mode 100644 docs/features/hypervisorfs.pandoc
create mode 100644 docs/man/xenhypfs.1.pod
create mode 100644 docs/misc/hypfs-paths.pandoc
create mode 100644 tools/libs/hypfs/Makefile
create mode 100644 tools/libs/hypfs/core.c
create mode 100644 tools/libs/hypfs/include/xenhypfs.h
create mode 100644 tools/libs/hypfs/libxenhypfs.map
create mode 100644 tools/libs/hypfs/xenhypfs.pc.in
create mode 100644 tools/misc/xenhypfs.c
create mode 100644 xen/common/hypfs.c
create mode 100644 xen/include/public/hypfs.h
create mode 100644 xen/include/xen/hypfs.h
create mode 100644 xen/tools/bin2c.c
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2 1/6] docs: add feature document for Xen hypervisor sysfs-like support
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
@ 2019-10-02 11:19 ` Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support Juergen Gross
` (4 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:19 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich
On the 2019 Xen developer summit there was agreement that the Xen
hypervisor should gain support for a hierarchical name-value store
similar to the Linux kernel's sysfs.
In the beginning there should only be basic support: entries can be
added from the hypervisor itself only, there is a simple hypercall
interface to read the data.
Add a feature document for setting the base of a discussion regarding
the desired functionality and the entries to add.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- remove the "--" prefixes of the sub-commands of the user tool
(Jan Beulich)
- rename xenfs to xenhypfs (Jan Beulich)
- add "tree" and "write" options to user tool
V2:
- move example tree to the paths description (Ian Jackson)
- specify allowed characters for keys and values (Ian Jackson)
---
docs/features/hypervisorfs.pandoc | 87 +++++++++++++++++++++++++++++++++++
docs/misc/hypfs-paths.pandoc | 95 +++++++++++++++++++++++++++++++++++++++
2 files changed, 182 insertions(+)
create mode 100644 docs/features/hypervisorfs.pandoc
create mode 100644 docs/misc/hypfs-paths.pandoc
diff --git a/docs/features/hypervisorfs.pandoc b/docs/features/hypervisorfs.pandoc
new file mode 100644
index 0000000000..64def6494a
--- /dev/null
+++ b/docs/features/hypervisorfs.pandoc
@@ -0,0 +1,87 @@
+% Hypervisor FS
+% Revision 1
+
+\clearpage
+
+# Basics
+---------------- ---------------------
+ Status: **Supported**
+
+ Architectures: all
+
+ Components: Hypervisor, toolstack
+---------------- ---------------------
+
+# Overview
+
+The Hypervisor FS is a hierarchical name-value store for reporting
+information to guests, especially dom0. It is similar to the Linux
+kernel's sysfs, but without the functionality to directly alter
+entries values. Entries and directories are created by the hypervisor,
+while the toolstack is able to use a hypercall to query the entry
+values.
+
+# User details
+
+With:
+
+ xenhypfs ls <path>
+
+the user can list the entries of a specific path of the FS. Using:
+
+ xenhypfs cat <path>
+
+the content of an entry can be retrieved. Using:
+
+ xenhypfs write <path> <string>
+
+a writable entry can be modified. With:
+
+ xenhypfs tree
+
+the complete Hypervisor FS entry tree can be printed.
+
+The FS paths are documented in `docs/misc/hypfs-paths.pandoc`.
+
+# Technical details
+
+Access to the hypervisor filesystem is done via the stable new hypercall
+__HYPERVISOR_filesystem_op.
+
+* hypercall interface specification
+ * `xen/include/public/filesystem.h`
+* hypervisor internal files
+ * `xen/include/xen/filesystem.h`
+ * `xen/common/filesystem.c`
+* `libxenhypfs`
+ * `tools/libs/libxenhypfs/*`
+* `xenhypfs`
+ * `tools/misc/xenhypfs.c`
+* path documentation
+ * `docs/misc/hypfs-paths.pandoc`
+
+# Testing
+
+Any new parameters or hardware mitigations should be verified to show up
+correctly in the filesystem.
+
+# Areas for improvement
+
+* More detailed access rights
+* Entries per domain and/or per cpupool
+
+# Known issues
+
+* None
+
+# References
+
+* None
+
+# History
+
+------------------------------------------------------------------------
+Date Revision Version Notes
+---------- -------- -------- -------------------------------------------
+2019-10-02 1 Xen 4.13 Document written
+---------- -------- -------- -------------------------------------------
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
new file mode 100644
index 0000000000..67de8d2cf8
--- /dev/null
+++ b/docs/misc/hypfs-paths.pandoc
@@ -0,0 +1,95 @@
+# Xenhypfs Paths
+
+This document attempts to define all the paths which are available
+in the Xen hypervisor file system (hypfs).
+
+The hypervisor file system can be accessed via the xenhypfs tool.
+
+## Notation
+
+The hypervisor file system is similar to the Linux kernel's sysfs.
+In this document directories are always specified with a trailing "/".
+
+The following notation conventions apply:
+
+ DIRECTORY/
+
+ PATH = VALUES [TAGS]
+
+The first syntax defines a directory. It normally contains related
+entries and the general scope of the directory is described.
+
+The second syntax defines a file entry containing values which are
+either set by the hypervisor or, if the file is writable, can be set
+by the user.
+
+PATH can contain simple regex constructs following the Perl compatible
+regexp syntax described in pcre(3) or perlre(1).
+
+A hypervisor file system entry name can be any 0-delimited byte string
+not containing any '/' character. The names "." and ".." are reserved
+for file system internal use.
+
+VALUES are strings and can take the following forms:
+
+* STRING -- an arbitrary 0-delimited byte string.
+* INTEGER -- An integer, in decimal representation unless otherwise
+ noted.
+* "a literal string" -- literal strings are contained within quotes.
+* (VALUE | VALUE | ... ) -- a set of alternatives. Alternatives are
+ separated by a "|" and all the alternatives are enclosed in "(" and
+ ")".
+
+Additional TAGS may follow as a comma separated set of the following
+tags enclosed in square brackets.
+
+* w -- Path is writable by the user. This capability is usually
+ limited to the control domain (e.g. dom0).
+* ARM | ARM32 | X86: the path is available for the respective architecture
+ only.
+* PV -- Path is valid for PV capable hypervisors only.
+* HVM -- Path is valid for HVM capable hypervisors only.
+* CONFIG_* -- Path is valid only in case the hypervisor was built with
+ the respective config option.
+
+## Example
+
+A populated Xen hypervisor file system might look like the following example:
+
+ /
+ buildinfo/ directory containing build-time data
+ config contents of .config file used to build Xen
+ cpu-bugs/ x86: directory of cpu bug information
+ l1tf "Vulnerable" or "Not vulnerable"
+ mds "Vulnerable" or "Not vulnerable"
+ meltdown "Vulnerable" or "Not vulnerable"
+ spec-store-bypass "Vulnerable" or "Not vulnerable"
+ spectre-v1 "Vulnerable" or "Not vulnerable"
+ spectre-v2 "Vulnerable" or "Not vulnerable"
+ mitigations/ directory of mitigation settings
+ bti-thunk "N/A", "RETPOLINE", "LFENCE" or "JMP"
+ spec-ctrl "No", "IBRS+" or IBRS-"
+ ibpb "No" or "Yes"
+ l1d-flush "No" or "Yes"
+ md-clear "No" or "VERW"
+ l1tf-barrier "No" or "Yes"
+ active-hvm/ directory for mitigations active in hvm doamins
+ msr-spec-ctrl "No" or "Yes"
+ rsb "No" or "Yes"
+ eager-fpu "No" or "Yes"
+ md-clear "No" or "Yes"
+ active-pv/ directory for mitigations active in pv doamins
+ msr-spec-ctrl "No" or "Yes"
+ rsb "No" or "Yes"
+ eager-fpu "No" or "Yes"
+ md-clear "No" or "Yes"
+ xpti "No" or list of "dom0", "domU", "PCID on"
+ l1tf-shadow "No" or list of "dom0", "domU"
+ params/ directory with hypervisor parameter values
+ (boot/runtime parameters)
+
+## General Paths
+
+#### /
+
+The root of the hypervisor file system.
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
2019-10-02 11:19 ` [Xen-devel] [PATCH v2 1/6] docs: add feature document for Xen " Juergen Gross
@ 2019-10-02 11:20 ` Juergen Gross
2019-11-12 13:48 ` Jan Beulich
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 3/6] libs: add libxenhypfs Juergen Gross
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:20 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
Add the infrastructure for the hypervisor filesystem.
This includes the hypercall interface and the base functions for
entry creation, deletion and modification.
Initially we support string and unsigned integer entry types. The saved
entry size is an upper bound, so for unsigned integer entries we always
set the value "11".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- rename files from filesystem.* to hypfs.*
- add dummy write entry support
- rename hypercall filesystem_op to hypfs_op
- add support for unsigned integer entries
V2:
- test new entry name to be valid
---
xen/arch/arm/traps.c | 1 +
xen/arch/x86/hvm/hypercall.c | 1 +
xen/arch/x86/hypercall.c | 1 +
xen/arch/x86/pv/hypercall.c | 1 +
xen/common/Makefile | 1 +
xen/common/hypfs.c | 318 +++++++++++++++++++++++++++++++++++++++++++
xen/include/public/errno.h | 1 +
xen/include/public/hypfs.h | 123 +++++++++++++++++
xen/include/public/xen.h | 1 +
xen/include/xen/hypercall.h | 8 ++
xen/include/xen/hypfs.h | 40 ++++++
11 files changed, 496 insertions(+)
create mode 100644 xen/common/hypfs.c
create mode 100644 xen/include/public/hypfs.h
create mode 100644 xen/include/xen/hypfs.h
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a3b961bd06..4b1acd67a2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1401,6 +1401,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
#ifdef CONFIG_ARGO
HYPERCALL(argo_op, 5),
#endif
+ HYPERCALL(hypfs_op, 5),
};
#ifndef NDEBUG
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 33dd2d99d2..210dda4f38 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -144,6 +144,7 @@ static const hypercall_table_t hvm_hypercall_table[] = {
#endif
HYPERCALL(xenpmu_op),
COMPAT_CALL(dm_op),
+ HYPERCALL(hypfs_op),
HYPERCALL(arch_1)
};
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index d483dbaa6b..8a8f223481 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -73,6 +73,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
ARGS(hvm_op, 2),
ARGS(dm_op, 3),
#endif
+ ARGS(hypfs_op, 5),
ARGS(mca, 1),
ARGS(arch_1, 1),
};
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 0c84c0b3a0..e6860247cc 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -84,6 +84,7 @@ const hypercall_table_t pv_hypercall_table[] = {
HYPERCALL(hvm_op),
COMPAT_CALL(dm_op),
#endif
+ HYPERCALL(hypfs_op),
HYPERCALL(mca),
HYPERCALL(arch_1),
};
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 62b34e69e9..a3f66aa0c0 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-y += domain.o
obj-y += event_2l.o
obj-y += event_channel.o
obj-y += event_fifo.o
+obj-y += hypfs.o
obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
obj-$(CONFIG_GRANT_TABLE) += grant_table.o
obj-y += guestcopy.o
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
new file mode 100644
index 0000000000..c8c5c504cf
--- /dev/null
+++ b/xen/common/hypfs.c
@@ -0,0 +1,318 @@
+/******************************************************************************
+ *
+ * hypfs.c
+ *
+ * Simple sysfs-like file system for the hypervisor.
+ */
+
+#include <xen/lib.h>
+#include <xen/hypfs.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
+#include <public/hypfs.h>
+
+static DEFINE_SPINLOCK(hypfs_lock);
+
+struct hypfs_dir hypfs_root = {
+ .list = LIST_HEAD_INIT(hypfs_root.list),
+};
+
+static struct hypfs_entry hypfs_root_entry = {
+ .type = hypfs_type_dir,
+ .name = "",
+ .list = LIST_HEAD_INIT(hypfs_root_entry.list),
+ .parent = &hypfs_root,
+ .dir = &hypfs_root,
+};
+
+static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
+{
+ int ret = -ENOENT;
+ struct list_head *l;
+
+ if ( !new->content )
+ return -EINVAL;
+
+ spin_lock(&hypfs_lock);
+
+ list_for_each ( l, &parent->list )
+ {
+ struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
+ int cmp = strcmp(e->name, new->name);
+
+ if ( cmp > 0 )
+ {
+ ret = 0;
+ list_add_tail(&new->list, l);
+ break;
+ }
+ if ( cmp == 0 )
+ {
+ ret = -EEXIST;
+ break;
+ }
+ }
+
+ if ( ret == -ENOENT )
+ {
+ ret = 0;
+ list_add_tail(&new->list, &parent->list);
+ }
+
+ if ( !ret )
+ {
+ unsigned int sz = strlen(new->name) + 1;
+
+ parent->content_size += sizeof(struct xen_hypfs_direntry) +
+ ROUNDUP(sz, 4);
+ new->parent = parent;
+ }
+
+ spin_unlock(&hypfs_lock);
+
+ return ret;
+}
+
+int hypfs_new_entry_any(struct hypfs_dir *parent, const char *name,
+ enum hypfs_entry_type type, void *content)
+{
+ int ret;
+ struct hypfs_entry *new;
+
+ if ( strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") )
+ return -EINVAL;
+
+ new = xzalloc(struct hypfs_entry);
+ if ( !new )
+ return -ENOMEM;
+
+ new->name = name;
+ new->type = type;
+ new->content = content;
+
+ ret = hypfs_add_entry(parent, new);
+
+ if ( ret )
+ xfree(new);
+
+ return ret;
+}
+
+int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
+ char *val)
+{
+ return hypfs_new_entry_any(parent, name, hypfs_type_string, val);
+}
+
+int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
+ unsigned int *val)
+{
+ return hypfs_new_entry_any(parent, name, hypfs_type_uint, val);
+}
+
+int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
+ struct hypfs_dir *dir)
+{
+ if ( !dir )
+ dir = xzalloc(struct hypfs_dir);
+
+ return hypfs_new_entry_any(parent, name, hypfs_type_dir, dir);
+}
+
+static int hypfs_get_path_user(char *buf, XEN_GUEST_HANDLE_PARAM(void) uaddr,
+ unsigned long len)
+{
+ if ( len > XEN_HYPFS_MAX_PATHLEN )
+ return -EINVAL;
+
+ if ( copy_from_guest(buf, uaddr, len) )
+ return -EFAULT;
+
+ buf[len - 1] = 0;
+
+ return 0;
+}
+
+static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry *dir,
+ char *path)
+{
+ char *slash;
+ struct hypfs_entry *entry;
+ struct list_head *l;
+ unsigned int name_len;
+
+ if ( *path == 0 )
+ return dir;
+
+ if ( dir->type != hypfs_type_dir )
+ return NULL;
+
+ slash = strchr(path, '/');
+ if ( !slash )
+ slash = strchr(path, '\0');
+ name_len = slash - path;
+
+ list_for_each ( l, &dir->dir->list )
+ {
+ int cmp;
+
+ entry = list_entry(l, struct hypfs_entry, list);
+ cmp = strncmp(path, entry->name, name_len);
+ if ( cmp < 0 )
+ return NULL;
+ if ( cmp > 0 )
+ continue;
+ if ( strlen(entry->name) == name_len )
+ return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
+ }
+
+ return NULL;
+}
+
+struct hypfs_entry *hypfs_get_entry(char *path)
+{
+ if ( path[0] != '/' )
+ return NULL;
+
+ return hypfs_get_entry_rel(&hypfs_root_entry, path + 1);
+}
+
+static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
+{
+ unsigned int len = 0;
+
+ switch ( entry->type )
+ {
+ case hypfs_type_dir:
+ len = entry->dir->content_size;
+ break;
+ case hypfs_type_string:
+ len = strlen(entry->str_val) + 1;
+ break;
+ case hypfs_type_uint:
+ len = 11; /* longest possible printed value + 1 */
+ break;
+ }
+
+ return len;
+}
+
+long do_hypfs_op(unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
+ XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
+{
+ int ret;
+ struct hypfs_entry *entry;
+ unsigned int len;
+ static char path[XEN_HYPFS_MAX_PATHLEN];
+
+ if ( !is_control_domain(current->domain) &&
+ !is_hardware_domain(current->domain) )
+ return -EPERM;
+
+ spin_lock(&hypfs_lock);
+
+ ret = hypfs_get_path_user(path, arg1, arg2);
+ if ( ret )
+ goto out;
+
+ entry = hypfs_get_entry(path);
+ if ( !entry )
+ {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ switch ( cmd )
+ {
+ case XEN_HYPFS_OP_read_contents:
+ {
+ char buf[12];
+ char *val = buf;
+
+ len = hypfs_get_entry_len(entry);
+ if ( len > arg4 )
+ {
+ ret = len;
+ break;
+ }
+
+ switch ( entry->type )
+ {
+ case hypfs_type_dir:
+ ret = -EISDIR;
+ break;
+ case hypfs_type_string:
+ val = entry->str_val;
+ break;
+ case hypfs_type_uint:
+ len = snprintf(buf, sizeof(buf), "%u", *entry->uint_val) + 1;
+ break;
+ }
+
+ if ( !ret && copy_to_guest(arg3, val, len) )
+ ret = -EFAULT;
+
+ break;
+ }
+
+ case XEN_HYPFS_OP_read_dir:
+ {
+ struct list_head *l;
+
+ if ( entry->type != hypfs_type_dir )
+ {
+ ret = -ENOTDIR;
+ break;
+ }
+
+ len = entry->dir->content_size;
+ if ( len > arg4 )
+ {
+ ret = len;
+ break;
+ }
+
+ list_for_each ( l, &entry->dir->list )
+ {
+ struct xen_hypfs_direntry direntry;
+ struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
+ unsigned int e_len = strlen(e->name) + 1;
+
+ e_len = sizeof(direntry) + ROUNDUP(e_len, 4);
+ direntry.flags = (e->type == hypfs_type_dir) ? XEN_HYPFS_ISDIR : 0;
+ direntry.off_next = list_is_last(l, &entry->dir->list) ? 0 : e_len;
+ direntry.content_len = hypfs_get_entry_len(e);
+ if ( copy_to_guest(arg3, &direntry, 1) )
+ {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if ( copy_to_guest_offset(arg3, sizeof(direntry), e->name,
+ strlen(e->name) + 1) )
+ {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ guest_handle_add_offset(arg3, e_len);
+ }
+
+ break;
+ }
+
+ case XEN_HYPFS_OP_write_contents:
+ ret = -EACCES;
+ break;
+
+ default:
+ ret = -ENOSYS;
+ break;
+ }
+
+ out:
+ spin_unlock(&hypfs_lock);
+
+ return ret;
+}
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index e1d02fcddf..5c53af6af9 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -78,6 +78,7 @@ XEN_ERRNO(EBUSY, 16) /* Device or resource busy */
XEN_ERRNO(EEXIST, 17) /* File exists */
XEN_ERRNO(EXDEV, 18) /* Cross-device link */
XEN_ERRNO(ENODEV, 19) /* No such device */
+XEN_ERRNO(ENOTDIR, 20) /* Not a directory */
XEN_ERRNO(EISDIR, 21) /* Is a directory */
XEN_ERRNO(EINVAL, 22) /* Invalid argument */
XEN_ERRNO(ENFILE, 23) /* File table overflow */
diff --git a/xen/include/public/hypfs.h b/xen/include/public/hypfs.h
new file mode 100644
index 0000000000..8822e2fb15
--- /dev/null
+++ b/xen/include/public/hypfs.h
@@ -0,0 +1,123 @@
+/******************************************************************************
+ * Xen Hypervisor Filesystem
+ *
+ * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (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 __XEN_PUBLIC_HYPFS_H__
+#define __XEN_PUBLIC_HYPFS_H__
+
+#include "xen.h"
+
+/*
+ * Definitions for the __HYPERVISOR_hypfs_op hypercall.
+ */
+
+/* Maximum length of a path in the filesystem. */
+#define XEN_HYPFS_MAX_PATHLEN 1024
+
+struct xen_hypfs_direntry {
+ uint16_t flags;
+#define XEN_HYPFS_ISDIR 0x0001
+#define XEN_HYPFS_WRITEABLE 0x0002
+ /* Offset in bytes to next entry (0 == this is the last entry). */
+ uint16_t off_next;
+ uint32_t content_len;
+ char name[XEN_FLEX_ARRAY_DIM];
+};
+
+/*
+ * Hypercall operations.
+ */
+
+/*
+ * XEN_HYPFS_OP_read_contents
+ *
+ * Read contents of a filesystem entry.
+ *
+ * Returns the contents of an entry in the buffer supplied by the caller.
+ * Only text data with a trailing zero byte is returned.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * -EPERM: operation not permitted
+ * -ENOENT: entry not found
+ * -EACCESS: access to entry not permitted
+ * -EISDIR: entry is a directory
+ * -EINVAL: invalid parameter
+ * positive value: content buffer was too small, returned value is needed size
+ */
+#define XEN_HYPFS_OP_read_contents 1
+
+/*
+ * XEN_HYPFS_OP_read_dir
+ *
+ * Read directory entries of a directory.
+ *
+ * Returns a struct xen_fs_direntry for each entry in a directory.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * -EPERM: operation not permitted
+ * -ENOENT: entry not found
+ * -EACCESS: access to entry not permitted
+ * -ENOTDIR: entry is not a directory
+ * -EINVAL: invalid parameter
+ * positive value: content buffer was too small, returned value is needed size
+ */
+#define XEN_HYPFS_OP_read_dir 2
+
+/*
+ * XEN_HYPFS_OP_read_contents
+ *
+ * Write contents of a filesystem entry.
+ *
+ * Writes an entry with the contents of a buffer supplied by the caller.
+ * Only text data with a trailing zero byte can be written.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * -EPERM: operation not permitted
+ * -ENOENT: entry not found
+ * -EACCESS: access to entry not permitted
+ * -EISDIR: entry is a directory
+ * -EINVAL: invalid parameter
+ * -ENOMEM: memory shortage in the hypervisor
+ */
+#define XEN_HYPFS_OP_write_contents 3
+
+#endif /* __XEN_PUBLIC_HYPFS_H__ */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d2198dffad..bf80f1da8c 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -130,6 +130,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
#define __HYPERVISOR_argo_op 39
#define __HYPERVISOR_xenpmu_op 40
#define __HYPERVISOR_dm_op 41
+#define __HYPERVISOR_hypfs_op 42
/* Architecture-specific hypercall definitions. */
#define __HYPERVISOR_arch_0 48
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index ad8ad27b23..349a0f6487 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -150,6 +150,14 @@ do_dm_op(
unsigned int nr_bufs,
XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
+extern long
+do_hypfs_op(
+ unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(void) arg1,
+ unsigned long arg2,
+ XEN_GUEST_HANDLE_PARAM(void) arg3,
+ unsigned long arg4);
+
#ifdef CONFIG_COMPAT
extern int
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
new file mode 100644
index 0000000000..d2537211f6
--- /dev/null
+++ b/xen/include/xen/hypfs.h
@@ -0,0 +1,40 @@
+#ifndef __XEN_HYPFS_H__
+#define __XEN_HYPFS_H__
+
+#include <xen/list.h>
+
+struct hypfs_dir {
+ unsigned int content_size;
+ struct list_head list;
+};
+
+enum hypfs_entry_type {
+ hypfs_type_dir,
+ hypfs_type_string,
+ hypfs_type_uint
+};
+
+struct hypfs_entry {
+ enum hypfs_entry_type type;
+ const char *name;
+ struct list_head list;
+ struct hypfs_dir *parent;
+ union {
+ void *content;
+ struct hypfs_dir *dir;
+ char *str_val;
+ unsigned int *uint_val;
+ };
+};
+
+extern struct hypfs_dir hypfs_root;
+
+int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
+ struct hypfs_dir *dir);
+int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
+ char *val);
+int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
+ unsigned int *val);
+struct hypfs_entry *hypfs_get_entry(char *path);
+
+#endif /* __XEN_HYPFS_H__ */
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2 3/6] libs: add libxenhypfs
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
2019-10-02 11:19 ` [Xen-devel] [PATCH v2 1/6] docs: add feature document for Xen " Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support Juergen Gross
@ 2019-10-02 11:20 ` Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 4/6] tools: add xenfs tool Juergen Gross
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:20 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Ian Jackson, Wei Liu
Add the new library libxenhypfs for access to the hypervisor filesystem.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
V1:
- rename to libxenhypfs
- add xenhypfs_write()
---
tools/Rules.mk | 6 +
tools/libs/Makefile | 1 +
tools/libs/hypfs/Makefile | 14 ++
tools/libs/hypfs/core.c | 252 ++++++++++++++++++++++++++++++++++++
tools/libs/hypfs/include/xenhypfs.h | 60 +++++++++
tools/libs/hypfs/libxenhypfs.map | 9 ++
tools/libs/hypfs/xenhypfs.pc.in | 10 ++
7 files changed, 352 insertions(+)
create mode 100644 tools/libs/hypfs/Makefile
create mode 100644 tools/libs/hypfs/core.c
create mode 100644 tools/libs/hypfs/include/xenhypfs.h
create mode 100644 tools/libs/hypfs/libxenhypfs.map
create mode 100644 tools/libs/hypfs/xenhypfs.pc.in
diff --git a/tools/Rules.mk b/tools/Rules.mk
index cf8935d6a3..b1ebb4c96c 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -19,6 +19,7 @@ XEN_LIBXENGNTTAB = $(XEN_ROOT)/tools/libs/gnttab
XEN_LIBXENCALL = $(XEN_ROOT)/tools/libs/call
XEN_LIBXENFOREIGNMEMORY = $(XEN_ROOT)/tools/libs/foreignmemory
XEN_LIBXENDEVICEMODEL = $(XEN_ROOT)/tools/libs/devicemodel
+XEN_LIBXENHYPFS = $(XEN_ROOT)/tools/libs/hypfs
XEN_LIBXC = $(XEN_ROOT)/tools/libxc
XEN_XENLIGHT = $(XEN_ROOT)/tools/libxl
# Currently libxlutil lives in the same directory as libxenlight
@@ -134,6 +135,11 @@ SHDEPS_libxendevicemodel = $(SHLIB_libxentoollog) $(SHLIB_libxentoolcore) $(SHLI
LDLIBS_libxendevicemodel = $(SHDEPS_libxendevicemodel) $(XEN_LIBXENDEVICEMODEL)/libxendevicemodel$(libextension)
SHLIB_libxendevicemodel = $(SHDEPS_libxendevicemodel) -Wl,-rpath-link=$(XEN_LIBXENDEVICEMODEL)
+CFLAGS_libxenhypfs = -I$(XEN_LIBXENHYPFS)/include $(CFLAGS_xeninclude)
+SHDEPS_libxenhypfs = $(SHLIB_libxentoollog) $(SHLIB_libxentoolcore) $(SHLIB_xencall)
+LDLIBS_libxenhypfs = $(SHDEPS_libxenhypfs) $(XEN_LIBXENHYPFS)/libxenhypfs$(libextension)
+SHLIB_libxenhypfs = $(SHDEPS_libxenhypfs) -Wl,-rpath-link=$(XEN_LIBXENHYPFS)
+
# code which compiles against libxenctrl get __XEN_TOOLS__ and
# therefore sees the unstable hypercall interfaces.
CFLAGS_libxenctrl = -I$(XEN_LIBXC)/include $(CFLAGS_libxentoollog) $(CFLAGS_libxenforeignmemory) $(CFLAGS_libxendevicemodel) $(CFLAGS_xeninclude) -D__XEN_TOOLS__
diff --git a/tools/libs/Makefile b/tools/libs/Makefile
index 88901e7341..69cdfb5975 100644
--- a/tools/libs/Makefile
+++ b/tools/libs/Makefile
@@ -9,6 +9,7 @@ SUBDIRS-y += gnttab
SUBDIRS-y += call
SUBDIRS-y += foreignmemory
SUBDIRS-y += devicemodel
+SUBDIRS-y += hypfs
ifeq ($(CONFIG_RUMP),y)
SUBDIRS-y := toolcore
diff --git a/tools/libs/hypfs/Makefile b/tools/libs/hypfs/Makefile
new file mode 100644
index 0000000000..c571597686
--- /dev/null
+++ b/tools/libs/hypfs/Makefile
@@ -0,0 +1,14 @@
+XEN_ROOT = $(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+MAJOR = 1
+MINOR = 0
+LIBNAME := hypfs
+USELIBS := toollog toolcore call
+
+SRCS-y += core.c
+
+include ../libs.mk
+
+$(PKG_CONFIG_LOCAL): PKG_CONFIG_INCDIR = $(XEN_LIBXENHYPFS)/include
+$(PKG_CONFIG_LOCAL): PKG_CONFIG_CFLAGS_LOCAL = $(CFLAGS_xeninclude)
diff --git a/tools/libs/hypfs/core.c b/tools/libs/hypfs/core.c
new file mode 100644
index 0000000000..edbf37f2c1
--- /dev/null
+++ b/tools/libs/hypfs/core.c
@@ -0,0 +1,252 @@
+/*
+ * Copyright (c) 2019 SUSE Software Solutions Germany GmbH
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define __XEN_TOOLS__ 1
+
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <xentoollog.h>
+#include <xenhypfs.h>
+#include <xencall.h>
+
+#include <xentoolcore_internal.h>
+
+struct xenhypfs_handle {
+ xentoollog_logger *logger, *logger_tofree;
+ unsigned int flags;
+ xencall_handle *xcall;
+};
+
+xenhypfs_handle *xenhypfs_open(xentoollog_logger *logger,
+ unsigned open_flags)
+{
+ xenhypfs_handle *fshdl = calloc(1, sizeof(*fshdl));
+
+ if (!fshdl)
+ return NULL;
+
+ fshdl->flags = open_flags;
+ fshdl->logger = logger;
+ fshdl->logger_tofree = NULL;
+
+ if (!fshdl->logger) {
+ fshdl->logger = fshdl->logger_tofree =
+ (xentoollog_logger*)
+ xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
+ if (!fshdl->logger)
+ goto err;
+ }
+
+ fshdl->xcall = xencall_open(fshdl->logger, 0);
+ if (!fshdl->xcall)
+ goto err;
+
+
+ return fshdl;
+
+err:
+ xtl_logger_destroy(fshdl->logger_tofree);
+ xencall_close(fshdl->xcall);
+ free(fshdl);
+ return NULL;
+}
+
+int xenhypfs_close(xenhypfs_handle *fshdl)
+{
+ if (!fshdl)
+ return 0;
+
+ xencall_close(fshdl->xcall);
+ xtl_logger_destroy(fshdl->logger_tofree);
+ free(fshdl);
+ return 0;
+}
+
+static int xenhypfs_get_pathbuf(xenhypfs_handle *fshdl, const char *path,
+ char **path_buf)
+{
+ int ret = -1;
+ int path_sz;
+
+ if (!fshdl) {
+ errno = EBADF;
+ goto out;
+ }
+
+ path_sz = strlen(path) + 1;
+ if (path_sz > XEN_HYPFS_MAX_PATHLEN)
+ {
+ errno = ENAMETOOLONG;
+ goto out;
+ }
+
+ *path_buf = xencall_alloc_buffer(fshdl->xcall, path_sz);
+ if (!*path_buf) {
+ errno = ENOMEM;
+ goto out;
+ }
+ strcpy(*path_buf, path);
+
+ ret = path_sz;
+
+ out:
+ return ret;
+}
+
+static void *xenhypfs_read_any(xenhypfs_handle *fshdl, const char *path,
+ unsigned int cmd)
+{
+ char *buf = NULL, *path_buf = NULL;
+ int ret;
+ int sz, path_sz;
+
+ ret = xenhypfs_get_pathbuf(fshdl, path, &path_buf);
+ if (ret < 0)
+ goto out;
+
+ path_sz = ret;
+
+ for (sz = 4096; sz > 0; sz = ret) {
+ if (buf)
+ xencall_free_buffer(fshdl->xcall, buf);
+
+ buf = xencall_alloc_buffer(fshdl->xcall, sz);
+ if (!buf) {
+ errno = ENOMEM;
+ goto out;
+ }
+
+ ret = xencall5(fshdl->xcall, __HYPERVISOR_hypfs_op, cmd,
+ (unsigned long)path_buf, path_sz,
+ (unsigned long)buf, sz);
+ }
+
+ if (ret < 0) {
+ errno = -ret;
+ xencall_free_buffer(fshdl->xcall, buf);
+ buf = NULL;
+ goto out;
+ }
+
+ out:
+ ret = errno;
+ xencall_free_buffer(fshdl->xcall, path_buf);
+ errno = ret;
+
+ return buf;
+}
+
+char *xenhypfs_read(xenhypfs_handle *fshdl, const char *path)
+{
+ char *buf, *ret_buf = NULL;
+ int ret;
+
+ buf = xenhypfs_read_any(fshdl, path, XEN_HYPFS_OP_read_contents);
+ if (buf)
+ ret_buf = strdup(buf);
+
+ ret = errno;
+ xencall_free_buffer(fshdl->xcall, buf);
+ errno = ret;
+
+ return ret_buf;
+}
+
+struct xenhypfs_dirent *xenhypfs_readdir(xenhypfs_handle *fshdl,
+ const char *path,
+ unsigned int *num_entries)
+{
+ void *buf, *curr;
+ int ret;
+ char *names;
+ struct xenhypfs_dirent *ret_buf = NULL;
+ unsigned int n, name_sz = 0;
+ struct xen_hypfs_direntry *entry;
+
+ buf = xenhypfs_read_any(fshdl, path, XEN_HYPFS_OP_read_dir);
+ if (!buf)
+ goto out;
+
+ curr = buf;
+ for (n = 1;; n++) {
+ entry = curr;
+ name_sz += strlen(entry->name) + 1;
+ if (!entry->off_next)
+ break;
+
+ curr += entry->off_next;
+ }
+
+ ret_buf = malloc(n * sizeof(*ret_buf) + name_sz);
+ if (!ret_buf)
+ goto out;
+
+ *num_entries = n;
+ names = (char *)(ret_buf + n);
+ curr = buf;
+ for (n = 0; n < *num_entries; n++) {
+ entry = curr;
+ ret_buf[n].name = names;
+ ret_buf[n].is_dir = entry->flags & XEN_HYPFS_ISDIR;
+ strcpy(names, entry->name);
+ names += strlen(entry->name) + 1;
+ curr += entry->off_next;
+ }
+
+ out:
+ ret = errno;
+ xencall_free_buffer(fshdl->xcall, buf);
+ errno = ret;
+
+ return ret_buf;
+}
+
+int xenhypfs_write(xenhypfs_handle *fshdl, const char *path, const char *val)
+{
+ char *buf = NULL, *path_buf = NULL;
+ int ret, saved_errno;
+ int sz, path_sz;
+
+ ret = xenhypfs_get_pathbuf(fshdl, path, &path_buf);
+ if (ret < 0)
+ goto out;
+
+ path_sz = ret;
+
+ sz = strlen(val) + 1;
+ buf = xencall_alloc_buffer(fshdl->xcall, sz);
+ if (!buf) {
+ ret = -1;
+ errno = ENOMEM;
+ goto out;
+ }
+ strcpy(buf, val);
+
+ ret = xencall5(fshdl->xcall, __HYPERVISOR_hypfs_op,
+ XEN_HYPFS_OP_write_contents,
+ (unsigned long)path_buf, path_sz,
+ (unsigned long)buf, sz);
+
+ out:
+ saved_errno = errno;
+ xencall_free_buffer(fshdl->xcall, path_buf);
+ xencall_free_buffer(fshdl->xcall, buf);
+ errno = saved_errno;
+ return ret;
+}
diff --git a/tools/libs/hypfs/include/xenhypfs.h b/tools/libs/hypfs/include/xenhypfs.h
new file mode 100644
index 0000000000..443221510a
--- /dev/null
+++ b/tools/libs/hypfs/include/xenhypfs.h
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2019 SUSE Software Solutions Germany GmbH
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef XENHYPFS_H
+#define XENHYPFS_H
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include <xen/xen.h>
+#include <xen/hypfs.h>
+
+/* Callers who don't care don't need to #include <xentoollog.h> */
+struct xentoollog_logger;
+
+typedef struct xenhypfs_handle xenhypfs_handle;
+
+struct xenhypfs_dirent {
+ char *name;
+ bool is_dir;
+};
+
+xenhypfs_handle *xenhypfs_open(struct xentoollog_logger *logger,
+ unsigned int open_flags);
+int xenhypfs_close(xenhypfs_handle *fshdl);
+
+/* Returned buffer should be freed via free(). */
+char *xenhypfs_read(xenhypfs_handle *fshdl, const char *path);
+
+/* Returned buffer should be freed via free(). */
+struct xenhypfs_dirent *xenhypfs_readdir(xenhypfs_handle *fshdl,
+ const char *path,
+ unsigned int *num_entries);
+
+int xenhypfs_write(xenhypfs_handle *fshdl, const char *path, const char *val);
+
+#endif /* XENHYPFS_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libs/hypfs/libxenhypfs.map b/tools/libs/hypfs/libxenhypfs.map
new file mode 100644
index 0000000000..39c63f4367
--- /dev/null
+++ b/tools/libs/hypfs/libxenhypfs.map
@@ -0,0 +1,9 @@
+VERS_1.0 {
+ global:
+ xenhypfs_open;
+ xenhypfs_close;
+ xenhypfs_read;
+ xenhypfs_readdir;
+ xenhypfs_write;
+ local: *; /* Do not expose anything by default */
+};
diff --git a/tools/libs/hypfs/xenhypfs.pc.in b/tools/libs/hypfs/xenhypfs.pc.in
new file mode 100644
index 0000000000..9cb968f0db
--- /dev/null
+++ b/tools/libs/hypfs/xenhypfs.pc.in
@@ -0,0 +1,10 @@
+prefix=@@prefix@@
+includedir=@@incdir@@
+libdir=@@libdir@@
+
+Name: Xenhypfs
+Description: The Xenhypfs library for Xen hypervisor
+Version: @@version@@
+Cflags: -I${includedir} @@cflagslocal@@
+Libs: @@libsflag@@${libdir} -lxenhypfs
+Requires.private: xentoolcore,xentoollog,xencall
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2 4/6] tools: add xenfs tool
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
` (2 preceding siblings ...)
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 3/6] libs: add libxenhypfs Juergen Gross
@ 2019-10-02 11:20 ` Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs Juergen Gross
5 siblings, 0 replies; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:20 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich
Add the xenfs tool for accessing the hypervisor filesystem.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- rename to xenhypfs
- don't use "--" for subcommands
- add write support
V2:
- escape non-printable characters per default with cat szbcommand
(Ian Jackson)
- add -b option to cat subcommand (Ian Jackson)
- add man page
---
.gitignore | 1 +
docs/man/xenhypfs.1.pod | 61 ++++++++++++++++++
tools/misc/Makefile | 6 ++
tools/misc/xenhypfs.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 228 insertions(+)
create mode 100644 docs/man/xenhypfs.1.pod
create mode 100644 tools/misc/xenhypfs.c
diff --git a/.gitignore b/.gitignore
index 3ada0c4f0b..954c1da2cb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -370,6 +370,7 @@ tools/libxl/test_timedereg
tools/libxl/test_fdderegrace
tools/firmware/etherboot/eb-roms.h
tools/firmware/etherboot/gpxe-git-snapshot.tar.gz
+tools/misc/xenhypfs
tools/misc/xenwatchdogd
tools/misc/xen-hvmcrash
tools/misc/xen-lowmemd
diff --git a/docs/man/xenhypfs.1.pod b/docs/man/xenhypfs.1.pod
new file mode 100644
index 0000000000..37aa488fcc
--- /dev/null
+++ b/docs/man/xenhypfs.1.pod
@@ -0,0 +1,61 @@
+=head1 NAME
+
+xenhypfs - Xen tool to access Xen hypervisor file system
+
+=head1 SYNOPSIS
+
+B<xenhypfs> I<subcommand> [I<options>] [I<args>]
+
+=head1 DESCRIPTION
+
+The B<xenhypfs> program is used to access the Xen hypervisor file system.
+It can be used to show the available entries, to show their contents and
+(if allowed) to modify their contents.
+
+=head1 SUBCOMMANDS
+
+=over 4
+
+=item B<ls> I<path>
+
+List the available entries below I<path>.
+
+=item B<cat> [I<-b>] I<path>
+
+Show the contents of the entry specified by I<path>. Non-printable characters
+other than white space characters (like tab, new line) will be shown as
+B<\xnn> (B<nn> being a two digit hex number) unless the option B<-b> is
+specified.
+
+=item B<write> I<path> I<value>
+
+Set the contents of the entry specified by I<path> to I<value>.
+
+=item B<tree>
+
+Show all the entries of the file system as a tree.
+
+=back
+
+=head1 RETURN CODES
+
+=over 4
+
+=item B<0>
+
+Success
+
+=item B<1>
+
+Invalid usage (e.g. unknown subcommand, unknown option, missing parameter).
+
+=item B<2>
+
+Entry not found while traversing the tree.
+
+=item B<3>
+
+Access right violation.
+
+=back
+
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 63947bfadc..9fdb13597f 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -24,6 +24,7 @@ INSTALL_SBIN-$(CONFIG_X86) += xen-lowmemd
INSTALL_SBIN-$(CONFIG_X86) += xen-mfndump
INSTALL_SBIN-$(CONFIG_X86) += xen-ucode
INSTALL_SBIN += xencov
+INSTALL_SBIN += xenhypfs
INSTALL_SBIN += xenlockprof
INSTALL_SBIN += xenperf
INSTALL_SBIN += xenpm
@@ -86,6 +87,9 @@ xenperf: xenperf.o
xenpm: xenpm.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+xenhypfs: xenhypfs.o
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenhypfs) $(APPEND_LDFLAGS)
+
xenlockprof: xenlockprof.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
@@ -94,6 +98,8 @@ xen-hptool.o: CFLAGS += -I$(XEN_ROOT)/tools/libxc $(CFLAGS_libxencall)
xen-hptool: xen-hptool.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(APPEND_LDFLAGS)
+xenhypfs.o: CFLAGS += $(CFLAGS_libxenhypfs)
+
# xen-mfndump incorrectly uses libxc internals
xen-mfndump.o: CFLAGS += -I$(XEN_ROOT)/tools/libxc $(CFLAGS_libxencall)
xen-mfndump: xen-mfndump.o
diff --git a/tools/misc/xenhypfs.c b/tools/misc/xenhypfs.c
new file mode 100644
index 0000000000..12cf989d53
--- /dev/null
+++ b/tools/misc/xenhypfs.c
@@ -0,0 +1,160 @@
+#define _GNU_SOURCE
+#include <ctype.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <xenhypfs.h>
+
+static struct xenhypfs_handle *hdl;
+
+static int usage(void)
+{
+ fprintf(stderr, "usage: xenhypfs ls <path>\n");
+ fprintf(stderr, " xenhypfs cat [-b] <path>\n");
+ fprintf(stderr, " xenhypfs write <path> <val>\n");
+ fprintf(stderr, " xenhypfs tree\n");
+
+ return 1;
+}
+
+static void xenhypfs_print_escaped(char *string)
+{
+ char *c;
+
+ for (c = string; *c; c++) {
+ if (isgraph(*c) || isspace(*c))
+ printf("%c", *c);
+ else
+ printf("\\x%02x", *c);
+ }
+ printf("\n");
+}
+
+static int xenhypfs_cat(int argc, char *argv[])
+{
+ int ret = 0;
+ char *result;
+ char *path;
+ bool bin = false;
+
+ switch (argc) {
+ case 1:
+ path = argv[0];
+ break;
+
+ case 2:
+ if (strcmp(argv[0], "-b"))
+ return usage();
+ bin = true;
+ path = argv[1];
+ break;
+
+ default:
+ return usage();
+ }
+
+ result = xenhypfs_read(hdl, path);
+ if (!result) {
+ perror("could not read");
+ ret = 3;
+ } else {
+ if (bin)
+ printf("%s\n", result);
+ else
+ xenhypfs_print_escaped(result);
+ free(result);
+ }
+
+ return ret;
+}
+
+static int xenhypfs_wr(char *path, char *val)
+{
+ int ret;
+
+ ret = xenhypfs_write(hdl, path, val);
+ if (ret) {
+ perror("could not write");
+ ret = 3;
+ }
+
+ return ret;
+}
+
+static int xenhypfs_ls(char *path)
+{
+ struct xenhypfs_dirent *ent;
+ unsigned int n, i;
+ int ret = 0;
+
+ ent = xenhypfs_readdir(hdl, path, &n);
+ if (!ent) {
+ perror("could not read dir");
+ ret = 3;
+ } else {
+ for (i = 0; i < n; i++)
+ printf("%c %s\n", ent[i].is_dir ? 'd' : '-', ent[i].name);
+
+ free(ent);
+ }
+
+ return ret;
+}
+
+static int xenhypfs_tree_sub(char *path, unsigned int depth)
+{
+ struct xenhypfs_dirent *ent;
+ unsigned int n, i;
+ int ret = 0;
+ char *p;
+
+ ent = xenhypfs_readdir(hdl, path, &n);
+ if (!ent)
+ return 2;
+
+ for (i = 0; i < n; i++) {
+ printf("%*s%s%s\n", depth * 2, "", ent[i].name,
+ ent[i].is_dir ? "/" : "");
+ if (ent[i].is_dir) {
+ asprintf(&p, "%s%s%s", path, (depth == 1) ? "" : "/", ent[i].name);
+ if (xenhypfs_tree_sub(p, depth + 1))
+ ret = 2;
+ }
+ }
+
+ free(ent);
+
+ return ret;
+}
+
+static int xenhypfs_tree(void)
+{
+ printf("/\n");
+
+ return xenhypfs_tree_sub("/", 1);
+}
+
+int main(int argc, char *argv[])
+{
+ int ret;
+
+ hdl = xenhypfs_open(NULL, 0);
+
+ if (!hdl) {
+ fprintf(stderr, "Could not open libxenhypfs\n");
+ ret = 2;
+ } else if (argc >= 3 && !strcmp(argv[1], "cat"))
+ ret = xenhypfs_cat(argc - 2, argv + 2);
+ else if (argc == 3 && !strcmp(argv[1], "ls"))
+ ret = xenhypfs_ls(argv[2]);
+ else if (argc == 4 && !strcmp(argv[1], "write"))
+ ret = xenhypfs_wr(argv[2], argv[3]);
+ else if (argc == 2 && !strcmp(argv[1], "tree"))
+ ret = xenhypfs_tree();
+ else
+ ret = usage();
+
+ xenhypfs_close(hdl);
+
+ return ret;
+}
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
` (3 preceding siblings ...)
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 4/6] tools: add xenfs tool Juergen Gross
@ 2019-10-02 11:20 ` Juergen Gross
2019-11-12 14:22 ` Jan Beulich
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs Juergen Gross
5 siblings, 1 reply; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:20 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich
Add the /buildinfo/config entry to the hypervisor filesystem. This
entry contains the .config file used to build the hypervisor.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 2 ++
docs/misc/hypfs-paths.pandoc | 9 +++++++++
xen/common/Makefile | 9 +++++++++
xen/common/hypfs.c | 17 +++++++++++++++++
xen/include/xen/kernel.h | 2 ++
xen/tools/Makefile | 9 +++++++--
xen/tools/bin2c.c | 28 ++++++++++++++++++++++++++++
7 files changed, 74 insertions(+), 2 deletions(-)
create mode 100644 xen/tools/bin2c.c
diff --git a/.gitignore b/.gitignore
index 954c1da2cb..16dfbb8302 100644
--- a/.gitignore
+++ b/.gitignore
@@ -295,6 +295,7 @@ xen/arch/*/efi/boot.c
xen/arch/*/efi/compat.c
xen/arch/*/efi/efi.h
xen/arch/*/efi/runtime.c
+xen/common/config_data.c
xen/include/headers*.chk
xen/include/asm
xen/include/asm-*/asm-offsets.h
@@ -312,6 +313,7 @@ xen/test/livepatch/xen_bye_world.livepatch
xen/test/livepatch/xen_hello_world.livepatch
xen/test/livepatch/xen_nop.livepatch
xen/test/livepatch/xen_replace_world.livepatch
+xen/tools/bin2c
xen/tools/kconfig/.tmp_gtkcheck
xen/tools/kconfig/.tmp_qtcheck
xen/tools/symbols
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index 67de8d2cf8..81353546ef 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -93,3 +93,12 @@ A populated Xen hypervisor file system might look like the following example:
#### /
The root of the hypervisor file system.
+
+#### /buildinfo/
+
+A directory containing static information generated while building the
+hypervisor.
+
+#### /buildinfo/config = STRING
+
+The contents of the `xen/.config` file at the time of the hypervisor build.
diff --git a/xen/common/Makefile b/xen/common/Makefile
index a3f66aa0c0..de7e0fa645 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -1,6 +1,7 @@
obj-$(CONFIG_ARGO) += argo.o
obj-y += bitmap.o
obj-y += bsearch.o
+obj-y += config_data.o
obj-$(CONFIG_CORE_PARKING) += core_parking.o
obj-y += cpu.o
obj-y += cpupool.o
@@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
subdir-$(CONFIG_NEEDS_LIBELF) += libelf
subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
+
+config_data.c: ../.config
+ ( echo "char xen_config_data[] ="; \
+ ../tools/bin2c <$<; \
+ echo ";" ) > $@
+
+clean::
+ rm config_data.c 2>/dev/null || true
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index c8c5c504cf..ab17d0de49 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
.dir = &hypfs_root,
};
+static struct hypfs_dir hypfs_buildinfo = {
+ .list = LIST_HEAD_INIT(hypfs_buildinfo.list),
+};
+
static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
{
int ret = -ENOENT;
@@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
return ret;
}
+
+static int __init hypfs_init(void)
+{
+ int ret;
+
+ ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
+ BUG_ON(ret);
+ ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
+ BUG_ON(ret);
+
+ return 0;
+}
+__initcall(hypfs_init);
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64da9f..5ff2280b0f 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -100,5 +100,7 @@ extern enum system_state {
bool_t is_active_kernel_text(unsigned long addr);
+extern char xen_config_data[];
+
#endif /* _LINUX_KERNEL_H */
diff --git a/xen/tools/Makefile b/xen/tools/Makefile
index e940939d61..cd2bbbf647 100644
--- a/xen/tools/Makefile
+++ b/xen/tools/Makefile
@@ -1,13 +1,18 @@
include $(XEN_ROOT)/Config.mk
+PROGS = symbols bin2c
+
.PHONY: default
default:
- $(MAKE) symbols
+ $(MAKE) $(PROGS)
.PHONY: clean
clean:
- rm -f *.o symbols
+ rm -f *.o $(PROGS)
symbols: symbols.c
$(HOSTCC) $(HOSTCFLAGS) -o $@ $<
+
+bin2c: bin2c.c
+ $(HOSTCC) $(HOSTCFLAGS) -o $@ $<
diff --git a/xen/tools/bin2c.c b/xen/tools/bin2c.c
new file mode 100644
index 0000000000..c332399b70
--- /dev/null
+++ b/xen/tools/bin2c.c
@@ -0,0 +1,28 @@
+/*
+ * Unloved program to convert a binary on stdin to a C include on stdout
+ *
+ * Jan 1999 Matt Mackall <mpm@selenic.com>
+ *
+ * This software may be used and distributed according to the terms
+ * of the GNU General Public License, incorporated herein by reference.
+ */
+
+#include <stdio.h>
+
+int main(int argc, char *argv[])
+{
+ int ch, total = 0;
+
+ do {
+ printf("\t\"");
+ while ((ch = getchar()) != EOF) {
+ total++;
+ printf("\\x%02x", ch);
+ if (total % 16 == 0)
+ break;
+ }
+ printf("\"\n");
+ } while (ch != EOF);
+
+ return 0;
+}
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
` (4 preceding siblings ...)
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
@ 2019-10-02 11:20 ` Juergen Gross
2019-11-12 14:28 ` Jan Beulich
5 siblings, 1 reply; 18+ messages in thread
From: Juergen Gross @ 2019-10-02 11:20 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich
Add support to read values of hypervisor runtime parameters via the
hypervisor file system for all unsigned integer type runtime parameters.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
docs/misc/hypfs-paths.pandoc | 9 +++++++++
xen/common/kernel.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index 81353546ef..c12014505e 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -102,3 +102,12 @@ hypervisor.
#### /buildinfo/config = STRING
The contents of the `xen/.config` file at the time of the hypervisor build.
+
+#### /params/
+
+A directory of runtime parameters (those can be set via xl set-parameters).
+
+#### /params/*
+
+The single parameters. The description of the different parameters can be
+found in `docs/misc/xen-command-line.pandoc`.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 760917dab5..e2e6d171a7 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -7,6 +7,7 @@
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/errno.h>
+#include <xen/hypfs.h>
#include <xen/version.h>
#include <xen/sched.h>
#include <xen/paging.h>
@@ -320,6 +321,44 @@ int cmdline_strcmp(const char *frag, const char *name)
}
}
+static struct hypfs_dir hypfs_params = {
+ .list = LIST_HEAD_INIT(hypfs_params.list),
+};
+
+static int __init runtime_param_hypfs_add(void)
+{
+ const struct kernel_param *param;
+ int ret;
+
+ ret = hypfs_new_dir(&hypfs_root, "params", &hypfs_params);
+ BUG_ON(ret);
+
+ for ( param = __param_start; param < __param_end; param++ )
+ {
+ switch ( param->type )
+ {
+ case OPT_UINT:
+ if ( param->len == sizeof(unsigned int) )
+ ret = hypfs_new_entry_uint(&hypfs_params, param->name,
+ (unsigned int *)(param->par.var));
+ break;
+
+ case OPT_STR:
+ ret = hypfs_new_entry_uint(&hypfs_params, param->name,
+ param->par.var);
+ break;
+
+ default:
+ break;
+ }
+
+ BUG_ON(ret);
+ }
+
+ return 0;
+}
+__initcall(runtime_param_hypfs_add);
+
unsigned int tainted;
/**
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support Juergen Gross
@ 2019-11-12 13:48 ` Jan Beulich
2019-11-12 16:06 ` Jürgen Groß
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-11-12 13:48 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 02.10.2019 13:20, Juergen Gross wrote:
> --- /dev/null
> +++ b/xen/common/hypfs.c
> @@ -0,0 +1,318 @@
> +/******************************************************************************
> + *
> + * hypfs.c
> + *
> + * Simple sysfs-like file system for the hypervisor.
> + */
> +
> +#include <xen/lib.h>
> +#include <xen/hypfs.h>
> +#include <xen/guest_access.h>
> +#include <xen/hypercall.h>
> +#include <public/hypfs.h>
> +
> +static DEFINE_SPINLOCK(hypfs_lock);
> +
> +struct hypfs_dir hypfs_root = {
> + .list = LIST_HEAD_INIT(hypfs_root.list),
> +};
> +
> +static struct hypfs_entry hypfs_root_entry = {
> + .type = hypfs_type_dir,
> + .name = "",
> + .list = LIST_HEAD_INIT(hypfs_root_entry.list),
> + .parent = &hypfs_root,
> + .dir = &hypfs_root,
> +};
This looks to be used only in hypfs_get_entry(). Unless there are
plans to have further uses, it should be moved there.
I'm also somewhat puzzled by "name" being an empty string; this
too would look less suspicious if this wasn't a file scope variable.
> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
> +{
> + int ret = -ENOENT;
> + struct list_head *l;
> +
> + if ( !new->content )
> + return -EINVAL;
> +
> + spin_lock(&hypfs_lock);
> +
> + list_for_each ( l, &parent->list )
> + {
> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
const?
> + int cmp = strcmp(e->name, new->name);
> +
> + if ( cmp > 0 )
> + {
> + ret = 0;
> + list_add_tail(&new->list, l);
> + break;
> + }
> + if ( cmp == 0 )
> + {
> + ret = -EEXIST;
> + break;
> + }
> + }
> +
> + if ( ret == -ENOENT )
> + {
> + ret = 0;
> + list_add_tail(&new->list, &parent->list);
> + }
> +
> + if ( !ret )
> + {
> + unsigned int sz = strlen(new->name) + 1;
> +
> + parent->content_size += sizeof(struct xen_hypfs_direntry) +
> + ROUNDUP(sz, 4);
What is this literal 4 coming from? DYM alignof(struct xen_hypfs_direntry)?
> + new->parent = parent;
> + }
> +
> + spin_unlock(&hypfs_lock);
> +
> + return ret;
> +}
> +
> +int hypfs_new_entry_any(struct hypfs_dir *parent, const char *name,
> + enum hypfs_entry_type type, void *content)
Perhaps drop the _any suffix?
> +{
> + int ret;
> + struct hypfs_entry *new;
> +
> + if ( strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") )
> + return -EINVAL;
> +
> + new = xzalloc(struct hypfs_entry);
> + if ( !new )
> + return -ENOMEM;
> +
> + new->name = name;
> + new->type = type;
> + new->content = content;
> +
> + ret = hypfs_add_entry(parent, new);
> +
> + if ( ret )
> + xfree(new);
> +
> + return ret;
> +}
> +
> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
> + char *val)
The last parameter here and below being non-const is because of the
intended write support?
> +{
> + return hypfs_new_entry_any(parent, name, hypfs_type_string, val);
> +}
> +
> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
> + unsigned int *val)
> +{
> + return hypfs_new_entry_any(parent, name, hypfs_type_uint, val);
> +}
> +
> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
> + struct hypfs_dir *dir)
> +{
> + if ( !dir )
> + dir = xzalloc(struct hypfs_dir);
> +
> + return hypfs_new_entry_any(parent, name, hypfs_type_dir, dir);
> +}
> +
> +static int hypfs_get_path_user(char *buf, XEN_GUEST_HANDLE_PARAM(void) uaddr,
> + unsigned long len)
> +{
> + if ( len > XEN_HYPFS_MAX_PATHLEN )
> + return -EINVAL;
> +
> + if ( copy_from_guest(buf, uaddr, len) )
> + return -EFAULT;
> +
> + buf[len - 1] = 0;
In the public interface description you have "including trailing zero
byte". I think instead of putting one there you should check there's
one.
> + return 0;
> +}
> +
> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry *dir,
> + char *path)
const?
> +{
> + char *slash;
const?
> + struct hypfs_entry *entry;
> + struct list_head *l;
> + unsigned int name_len;
> +
> + if ( *path == 0 )
Please either use !*path or be consistent with code a few lines
down and use '\0'.
> + return dir;
> +
> + if ( dir->type != hypfs_type_dir )
> + return NULL;
> +
> + slash = strchr(path, '/');
> + if ( !slash )
> + slash = strchr(path, '\0');
With this better name the variable "end" or some such?
> + name_len = slash - path;
> +
> + list_for_each ( l, &dir->dir->list )
> + {
> + int cmp;
> +
> + entry = list_entry(l, struct hypfs_entry, list);
Why not list_for_each_entry(), eliminating the need for the "l"
helper variable?
> + cmp = strncmp(path, entry->name, name_len);
> + if ( cmp < 0 )
> + return NULL;
> + if ( cmp > 0 )
> + continue;
> + if ( strlen(entry->name) == name_len )
> + return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
Perhaps slightly shorter
if ( cmp == 0 && strlen(entry->name) == name_len )
return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
?
> + }
> +
> + return NULL;
> +}
> +
> +struct hypfs_entry *hypfs_get_entry(char *path)
const?
> +{
> + if ( path[0] != '/' )
> + return NULL;
> +
> + return hypfs_get_entry_rel(&hypfs_root_entry, path + 1);
> +}
> +
> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
> +{
> + unsigned int len = 0;
> +
> + switch ( entry->type )
> + {
> + case hypfs_type_dir:
> + len = entry->dir->content_size;
> + break;
> + case hypfs_type_string:
> + len = strlen(entry->str_val) + 1;
> + break;
> + case hypfs_type_uint:
> + len = 11; /* longest possible printed value + 1 */
Why would uint values be restricted to 32 bits? There are plenty of
64-bit numbers that might be of interest exposing through this
interface (and even more so if down the road we were to re-use this
for something debugfs-like). And even without this I think it would
be better to not have a literal number here - it'll be close to
unnoticeable (without someone remembering) when porting to an arch
with unsigned int wider than 32 bits.
> + break;
> + }
> +
> + return len;
> +}
> +
> +long do_hypfs_op(unsigned int cmd,
> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
> +{
> + int ret;
> + struct hypfs_entry *entry;
> + unsigned int len;
> + static char path[XEN_HYPFS_MAX_PATHLEN];
> +
> + if ( !is_control_domain(current->domain) &&
> + !is_hardware_domain(current->domain) )
> + return -EPERM;
Replace by an XSM check?
> + spin_lock(&hypfs_lock);
Wouldn't this better be an r/w lock from the beginning, requiring
only read access here?
> + ret = hypfs_get_path_user(path, arg1, arg2);
> + if ( ret )
> + goto out;
> +
> + entry = hypfs_get_entry(path);
> + if ( !entry )
> + {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + switch ( cmd )
> + {
> + case XEN_HYPFS_OP_read_contents:
> + {
> + char buf[12];
> + char *val = buf;
const void *?
> + len = hypfs_get_entry_len(entry);
> + if ( len > arg4 )
> + {
> + ret = len;
> + break;
> + }
> +
> + switch ( entry->type )
> + {
> + case hypfs_type_dir:
> + ret = -EISDIR;
> + break;
> + case hypfs_type_string:
> + val = entry->str_val;
> + break;
> + case hypfs_type_uint:
> + len = snprintf(buf, sizeof(buf), "%u", *entry->uint_val) + 1;
> + break;
> + }
> +
> + if ( !ret && copy_to_guest(arg3, val, len) )
> + ret = -EFAULT;
> +
> + break;
> + }
> +
> + case XEN_HYPFS_OP_read_dir:
> + {
> + struct list_head *l;
> +
> + if ( entry->type != hypfs_type_dir )
> + {
> + ret = -ENOTDIR;
> + break;
> + }
> +
> + len = entry->dir->content_size;
> + if ( len > arg4 )
> + {
> + ret = len;
> + break;
> + }
> +
> + list_for_each ( l, &entry->dir->list )
list_for_each_entry() again?
> + {
> + struct xen_hypfs_direntry direntry;
> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
> + unsigned int e_len = strlen(e->name) + 1;
> +
> + e_len = sizeof(direntry) + ROUNDUP(e_len, 4);
Literal 4 again. Perhaps you want to put the entire ROUNDUP(, 4)
construct in a macro or function?
> + direntry.flags = (e->type == hypfs_type_dir) ? XEN_HYPFS_ISDIR : 0;
> + direntry.off_next = list_is_last(l, &entry->dir->list) ? 0 : e_len;
> + direntry.content_len = hypfs_get_entry_len(e);
> + if ( copy_to_guest(arg3, &direntry, 1) )
> + {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + if ( copy_to_guest_offset(arg3, sizeof(direntry), e->name,
> + strlen(e->name) + 1) )
You calculate the length once already a few lines up. Please store into
another local variable and reuse here.
> + {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + guest_handle_add_offset(arg3, e_len);
I think it would be good to assert somewhere here that you don't go
beyond the dir's stored content_size.
> --- /dev/null
> +++ b/xen/include/public/hypfs.h
> @@ -0,0 +1,123 @@
> +/******************************************************************************
> + * Xen Hypervisor Filesystem
> + *
> + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (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 __XEN_PUBLIC_HYPFS_H__
> +#define __XEN_PUBLIC_HYPFS_H__
> +
> +#include "xen.h"
> +
> +/*
> + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
> + */
> +
> +/* Maximum length of a path in the filesystem. */
> +#define XEN_HYPFS_MAX_PATHLEN 1024
> +
> +struct xen_hypfs_direntry {
> + uint16_t flags;
> +#define XEN_HYPFS_ISDIR 0x0001
> +#define XEN_HYPFS_WRITEABLE 0x0002
> + /* Offset in bytes to next entry (0 == this is the last entry). */
> + uint16_t off_next;
> + uint32_t content_len;
> + char name[XEN_FLEX_ARRAY_DIM];
> +};
Are you certain we won't soon need further fields here? Expressing
symlinks can perhaps be done via a new XEN_HYPFS_* flag, but there
may be other things that would be better to provide for even if
there's no implementation from the beginning. It's just the case
that changing this structure after the fact is going to be
impossible, and it'll take new sub-ops then instead.
> +/*
> + * Hypercall operations.
> + */
> +
> +/*
> + * XEN_HYPFS_OP_read_contents
> + *
> + * Read contents of a filesystem entry.
> + *
> + * Returns the contents of an entry in the buffer supplied by the caller.
> + * Only text data with a trailing zero byte is returned.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(content buffer)
> + * arg4: content buffer size
> + *
> + * Possible return values:
> + * 0: success
> + * -EPERM: operation not permitted
> + * -ENOENT: entry not found
> + * -EACCESS: access to entry not permitted
> + * -EISDIR: entry is a directory
> + * -EINVAL: invalid parameter
I'm not convinced enumerating possible return value is a good idea.
Down the road we're certain to forget extending this list. Plus
extension would, strictly speaking, not even be allowed if these
enumerations are considered part of the interface.
> + * positive value: content buffer was too small, returned value is needed size
Positive return values are problematic when reaching INT_MAX. Are you
convinced we want to have yet another instance?
> + */
> +#define XEN_HYPFS_OP_read_contents 1
> +
> +/*
> + * XEN_HYPFS_OP_read_dir
> + *
> + * Read directory entries of a directory.
> + *
> + * Returns a struct xen_fs_direntry for each entry in a directory.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(content buffer)
> + * arg4: content buffer size
> + *
> + * Possible return values:
> + * 0: success
> + * -EPERM: operation not permitted
> + * -ENOENT: entry not found
> + * -EACCESS: access to entry not permitted
> + * -ENOTDIR: entry is not a directory
> + * -EINVAL: invalid parameter
> + * positive value: content buffer was too small, returned value is needed size
> + */
> +#define XEN_HYPFS_OP_read_dir 2
> +
> +/*
> + * XEN_HYPFS_OP_read_contents
XEN_HYPFS_OP_write_contents
> + * Write contents of a filesystem entry.
> + *
> + * Writes an entry with the contents of a buffer supplied by the caller.
> + * Only text data with a trailing zero byte can be written.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(content buffer)
> + * arg4: content buffer size
The latest here (in contrast to the read counterpart) I think it becomes
desirable to identify what's IN and what's OUT.
> --- /dev/null
> +++ b/xen/include/xen/hypfs.h
> @@ -0,0 +1,40 @@
> +#ifndef __XEN_HYPFS_H__
> +#define __XEN_HYPFS_H__
> +
> +#include <xen/list.h>
> +
> +struct hypfs_dir {
> + unsigned int content_size;
> + struct list_head list;
> +};
> +
> +enum hypfs_entry_type {
> + hypfs_type_dir,
> + hypfs_type_string,
> + hypfs_type_uint
> +};
> +
> +struct hypfs_entry {
> + enum hypfs_entry_type type;
> + const char *name;
> + struct list_head list;
> + struct hypfs_dir *parent;
Afaict you set this field, but you never use it anywhere. Why do you
add it in the first place? (Initially I meant to ask whether this
can be pointer-to-const.)
> + union {
> + void *content;
const?
> + struct hypfs_dir *dir;
const?
> + char *str_val;
> + unsigned int *uint_val;
> + };
> +};
> +
> +extern struct hypfs_dir hypfs_root;
> +
> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
> + struct hypfs_dir *dir);
> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
> + char *val);
> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
> + unsigned int *val);
Thinking about the lack of const on the last parameters here again -
if these are for the pointed to values to be modifiable through
this interface, then how would the "owning" component learn of the
value having changed? Not everyone may need this, but I think there
would want to be a callback. Until then perhaps better to add const
here, promising that the values won't change behind the backs of
the owners.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
@ 2019-11-12 14:22 ` Jan Beulich
2019-11-12 16:45 ` Jürgen Groß
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-11-12 14:22 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
On 02.10.2019 13:20, Juergen Gross wrote:
> Add the /buildinfo/config entry to the hypervisor filesystem. This
> entry contains the .config file used to build the hypervisor.
I think this is the 2nd step ahead of the 1st: Much of the stuff
exposed as XENVER_* sub-ops should manifest itself here ahead of
exposing xen/.config.
> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
>
> subdir-$(CONFIG_NEEDS_LIBELF) += libelf
> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
> +
> +config_data.c: ../.config
> + ( echo "char xen_config_data[] ="; \
> + ../tools/bin2c <$<; \
> + echo ";" ) > $@
This is the typical kind of construct that may break (a subsequent
build attempt) when interrupted in the middle. This pretty clearly
is a move-if-changed candidate, at the same time also avoiding a
(cheap, but anyway) pointless re-build in case .config was touched
without actually changing.
Furthermore is there a reason to expose this as plain text, when
Linux exposes a gzip-ed version in /proc? The file isn't very
large now, but this was also the case for Linux many years ago.
> --- a/xen/common/hypfs.c
> +++ b/xen/common/hypfs.c
> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
> .dir = &hypfs_root,
> };
>
> +static struct hypfs_dir hypfs_buildinfo = {
> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list),
> +};
> +
> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
> {
> int ret = -ENOENT;
> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
>
> return ret;
> }
> +
> +static int __init hypfs_init(void)
> +{
> + int ret;
> +
> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
> + BUG_ON(ret);
> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
> + BUG_ON(ret);
> +
> + return 0;
> +}
> +__initcall(hypfs_init);
Hmm, do you really want to centralize population of the file system
here, rather than having the individual components take care of it?
> --- a/xen/tools/Makefile
> +++ b/xen/tools/Makefile
> @@ -1,13 +1,18 @@
>
> include $(XEN_ROOT)/Config.mk
>
> +PROGS = symbols bin2c
> +
> .PHONY: default
> default:
> - $(MAKE) symbols
> + $(MAKE) $(PROGS)
Could I ask you to take the opportunity and do away with the
unnecessary (as it seems to me) make recursion? $(PROGS) could
easily become a dependency of "default" afaict.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs Juergen Gross
@ 2019-11-12 14:28 ` Jan Beulich
2019-11-12 16:46 ` Jürgen Groß
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-11-12 14:28 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
On 02.10.2019 13:20, Juergen Gross wrote:
> Add support to read values of hypervisor runtime parameters via the
> hypervisor file system for all unsigned integer type runtime parameters.
What about string ones (which you seem to handle in the code,
but see also there)?
> @@ -320,6 +321,44 @@ int cmdline_strcmp(const char *frag, const char *name)
> }
> }
>
> +static struct hypfs_dir hypfs_params = {
> + .list = LIST_HEAD_INIT(hypfs_params.list),
> +};
> +
> +static int __init runtime_param_hypfs_add(void)
> +{
> + const struct kernel_param *param;
> + int ret;
> +
> + ret = hypfs_new_dir(&hypfs_root, "params", &hypfs_params);
> + BUG_ON(ret);
> +
> + for ( param = __param_start; param < __param_end; param++ )
> + {
> + switch ( param->type )
> + {
> + case OPT_UINT:
> + if ( param->len == sizeof(unsigned int) )
> + ret = hypfs_new_entry_uint(&hypfs_params, param->name,
> + (unsigned int *)(param->par.var));
Stray pair or parentheses. I also don't see the need for the cast,
with the "var" union member being "void *".
> + break;
> +
> + case OPT_STR:
> + ret = hypfs_new_entry_uint(&hypfs_params, param->name,
> + param->par.var);
hypfs_new_entry_string()?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
2019-11-12 13:48 ` Jan Beulich
@ 2019-11-12 16:06 ` Jürgen Groß
2019-11-13 14:07 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Jürgen Groß @ 2019-11-12 16:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 12.11.19 14:48, Jan Beulich wrote:
> On 02.10.2019 13:20, Juergen Gross wrote:
>> --- /dev/null
>> +++ b/xen/common/hypfs.c
>> @@ -0,0 +1,318 @@
>> +/******************************************************************************
>> + *
>> + * hypfs.c
>> + *
>> + * Simple sysfs-like file system for the hypervisor.
>> + */
>> +
>> +#include <xen/lib.h>
>> +#include <xen/hypfs.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/hypercall.h>
>> +#include <public/hypfs.h>
>> +
>> +static DEFINE_SPINLOCK(hypfs_lock);
>> +
>> +struct hypfs_dir hypfs_root = {
>> + .list = LIST_HEAD_INIT(hypfs_root.list),
>> +};
>> +
>> +static struct hypfs_entry hypfs_root_entry = {
>> + .type = hypfs_type_dir,
>> + .name = "",
>> + .list = LIST_HEAD_INIT(hypfs_root_entry.list),
>> + .parent = &hypfs_root,
>> + .dir = &hypfs_root,
>> +};
>
> This looks to be used only in hypfs_get_entry(). Unless there are
> plans to have further uses, it should be moved there.
Okay.
>
> I'm also somewhat puzzled by "name" being an empty string; this
> too would look less suspicious if this wasn't a file scope variable.
>
>> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>> +{
>> + int ret = -ENOENT;
>> + struct list_head *l;
>> +
>> + if ( !new->content )
>> + return -EINVAL;
>> +
>> + spin_lock(&hypfs_lock);
>> +
>> + list_for_each ( l, &parent->list )
>> + {
>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>
> const?
Hmm, is this true when I add a new entry to it? l is part of *e
after all.
>
>> + int cmp = strcmp(e->name, new->name);
>> +
>> + if ( cmp > 0 )
>> + {
>> + ret = 0;
>> + list_add_tail(&new->list, l);
>> + break;
>> + }
>> + if ( cmp == 0 )
>> + {
>> + ret = -EEXIST;
>> + break;
>> + }
>> + }
>> +
>> + if ( ret == -ENOENT )
>> + {
>> + ret = 0;
>> + list_add_tail(&new->list, &parent->list);
>> + }
>> +
>> + if ( !ret )
>> + {
>> + unsigned int sz = strlen(new->name) + 1;
>> +
>> + parent->content_size += sizeof(struct xen_hypfs_direntry) +
>> + ROUNDUP(sz, 4);
>
> What is this literal 4 coming from? DYM alignof(struct xen_hypfs_direntry)?
Yes.
>
>> + new->parent = parent;
>> + }
>> +
>> + spin_unlock(&hypfs_lock);
>> +
>> + return ret;
>> +}
>> +
>> +int hypfs_new_entry_any(struct hypfs_dir *parent, const char *name,
>> + enum hypfs_entry_type type, void *content)
>
> Perhaps drop the _any suffix?
Okay.
>
>> +{
>> + int ret;
>> + struct hypfs_entry *new;
>> +
>> + if ( strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") )
>> + return -EINVAL;
>> +
>> + new = xzalloc(struct hypfs_entry);
>> + if ( !new )
>> + return -ENOMEM;
>> +
>> + new->name = name;
>> + new->type = type;
>> + new->content = content;
>> +
>> + ret = hypfs_add_entry(parent, new);
>> +
>> + if ( ret )
>> + xfree(new);
>> +
>> + return ret;
>> +}
>> +
>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>> + char *val)
>
> The last parameter here and below being non-const is because of the
> intended write support?
Yes.
>
>> +{
>> + return hypfs_new_entry_any(parent, name, hypfs_type_string, val);
>> +}
>> +
>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>> + unsigned int *val)
>> +{
>> + return hypfs_new_entry_any(parent, name, hypfs_type_uint, val);
>> +}
>> +
>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>> + struct hypfs_dir *dir)
>> +{
>> + if ( !dir )
>> + dir = xzalloc(struct hypfs_dir);
>> +
>> + return hypfs_new_entry_any(parent, name, hypfs_type_dir, dir);
>> +}
>> +
>> +static int hypfs_get_path_user(char *buf, XEN_GUEST_HANDLE_PARAM(void) uaddr,
>> + unsigned long len)
>> +{
>> + if ( len > XEN_HYPFS_MAX_PATHLEN )
>> + return -EINVAL;
>> +
>> + if ( copy_from_guest(buf, uaddr, len) )
>> + return -EFAULT;
>> +
>> + buf[len - 1] = 0;
>
> In the public interface description you have "including trailing zero
> byte". I think instead of putting one there you should check there's
> one.
Okay.
>
>> + return 0;
>> +}
>> +
>> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry *dir,
>> + char *path)
>
> const?
Yes.
>
>> +{
>> + char *slash;
>
> const?
Okay.
>
>> + struct hypfs_entry *entry;
>> + struct list_head *l;
>> + unsigned int name_len;
>> +
>> + if ( *path == 0 )
>
> Please either use !*path or be consistent with code a few lines
> down and use '\0'.
Okay.
>
>> + return dir;
>> +
>> + if ( dir->type != hypfs_type_dir )
>> + return NULL;
>> +
>> + slash = strchr(path, '/');
>> + if ( !slash )
>> + slash = strchr(path, '\0');
>
> With this better name the variable "end" or some such?
Fine with me.
>
>> + name_len = slash - path;
>> +
>> + list_for_each ( l, &dir->dir->list )
>> + {
>> + int cmp;
>> +
>> + entry = list_entry(l, struct hypfs_entry, list);
>
> Why not list_for_each_entry(), eliminating the need for the "l"
> helper variable?
Ah, of course!
>
>> + cmp = strncmp(path, entry->name, name_len);
>> + if ( cmp < 0 )
>> + return NULL;
>> + if ( cmp > 0 )
>> + continue;
>> + if ( strlen(entry->name) == name_len )
>> + return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
>
> Perhaps slightly shorter
>
> if ( cmp == 0 && strlen(entry->name) == name_len )
> return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
>
> ?
Okay.
>
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +struct hypfs_entry *hypfs_get_entry(char *path)
>
> const?
Yes.
>
>> +{
>> + if ( path[0] != '/' )
>> + return NULL;
>> +
>> + return hypfs_get_entry_rel(&hypfs_root_entry, path + 1);
>> +}
>> +
>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>> +{
>> + unsigned int len = 0;
>> +
>> + switch ( entry->type )
>> + {
>> + case hypfs_type_dir:
>> + len = entry->dir->content_size;
>> + break;
>> + case hypfs_type_string:
>> + len = strlen(entry->str_val) + 1;
>> + break;
>> + case hypfs_type_uint:
>> + len = 11; /* longest possible printed value + 1 */
>
> Why would uint values be restricted to 32 bits? There are plenty of
> 64-bit numbers that might be of interest exposing through this
> interface (and even more so if down the road we were to re-use this
> for something debugfs-like). And even without this I think it would
> be better to not have a literal number here - it'll be close to
> unnoticeable (without someone remembering) when porting to an arch
> with unsigned int wider than 32 bits.
Support of 64-bit numbers would add "hypfs_type_ulong".
So basically something like "(sizeof(type) * 8 * 3 + 9) / 10 + 1" ?
(equivalent to: 10 bits make roughly 3 digits, round that up and
add 0-Byte). This is correct for 1, 2, 4 and 8 byte values. For 16
byte values the result is 40, but it should be 41.
>
>> + break;
>> + }
>> +
>> + return len;
>> +}
>> +
>> +long do_hypfs_op(unsigned int cmd,
>> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>> +{
>> + int ret;
>> + struct hypfs_entry *entry;
>> + unsigned int len;
>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>> +
>> + if ( !is_control_domain(current->domain) &&
>> + !is_hardware_domain(current->domain) )
>> + return -EPERM;
>
> Replace by an XSM check?
Yes, but I could need some help here. How do I add a new hypercall
in XSM?
>
>> + spin_lock(&hypfs_lock);
>
> Wouldn't this better be an r/w lock from the beginning, requiring
> only read access here?
Depending on the further use cases we might even end up with per-element
locks. I'm fine using a r/w lock for now.
>
>> + ret = hypfs_get_path_user(path, arg1, arg2);
>> + if ( ret )
>> + goto out;
>> +
>> + entry = hypfs_get_entry(path);
>> + if ( !entry )
>> + {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> +
>> + switch ( cmd )
>> + {
>> + case XEN_HYPFS_OP_read_contents:
>> + {
>> + char buf[12];
>> + char *val = buf;
>
> const void *?
Why void *? The result is always a string.
>
>> + len = hypfs_get_entry_len(entry);
>> + if ( len > arg4 )
>> + {
>> + ret = len;
>> + break;
>> + }
>> +
>> + switch ( entry->type )
>> + {
>> + case hypfs_type_dir:
>> + ret = -EISDIR;
>> + break;
>> + case hypfs_type_string:
>> + val = entry->str_val;
>> + break;
>> + case hypfs_type_uint:
>> + len = snprintf(buf, sizeof(buf), "%u", *entry->uint_val) + 1;
>> + break;
>> + }
>> +
>> + if ( !ret && copy_to_guest(arg3, val, len) )
>> + ret = -EFAULT;
>> +
>> + break;
>> + }
>> +
>> + case XEN_HYPFS_OP_read_dir:
>> + {
>> + struct list_head *l;
>> +
>> + if ( entry->type != hypfs_type_dir )
>> + {
>> + ret = -ENOTDIR;
>> + break;
>> + }
>> +
>> + len = entry->dir->content_size;
>> + if ( len > arg4 )
>> + {
>> + ret = len;
>> + break;
>> + }
>> +
>> + list_for_each ( l, &entry->dir->list )
>
> list_for_each_entry() again?
Yes.
>
>> + {
>> + struct xen_hypfs_direntry direntry;
>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>> + unsigned int e_len = strlen(e->name) + 1;
>> +
>> + e_len = sizeof(direntry) + ROUNDUP(e_len, 4);
>
> Literal 4 again. Perhaps you want to put the entire ROUNDUP(, 4)
> construct in a macro or function?
Yes, might be better in a function.
>
>> + direntry.flags = (e->type == hypfs_type_dir) ? XEN_HYPFS_ISDIR : 0;
>> + direntry.off_next = list_is_last(l, &entry->dir->list) ? 0 : e_len;
>> + direntry.content_len = hypfs_get_entry_len(e);
>> + if ( copy_to_guest(arg3, &direntry, 1) )
>> + {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + if ( copy_to_guest_offset(arg3, sizeof(direntry), e->name,
>> + strlen(e->name) + 1) )
>
> You calculate the length once already a few lines up. Please store into
> another local variable and reuse here.
Okay.
>
>> + {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + guest_handle_add_offset(arg3, e_len);
>
> I think it would be good to assert somewhere here that you don't go
> beyond the dir's stored content_size.
Fine with me.
>
>> --- /dev/null
>> +++ b/xen/include/public/hypfs.h
>> @@ -0,0 +1,123 @@
>> +/******************************************************************************
>> + * Xen Hypervisor Filesystem
>> + *
>> + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (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 __XEN_PUBLIC_HYPFS_H__
>> +#define __XEN_PUBLIC_HYPFS_H__
>> +
>> +#include "xen.h"
>> +
>> +/*
>> + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
>> + */
>> +
>> +/* Maximum length of a path in the filesystem. */
>> +#define XEN_HYPFS_MAX_PATHLEN 1024
>> +
>> +struct xen_hypfs_direntry {
>> + uint16_t flags;
>> +#define XEN_HYPFS_ISDIR 0x0001
>> +#define XEN_HYPFS_WRITEABLE 0x0002
>> + /* Offset in bytes to next entry (0 == this is the last entry). */
>> + uint16_t off_next;
>> + uint32_t content_len;
>> + char name[XEN_FLEX_ARRAY_DIM];
>> +};
>
> Are you certain we won't soon need further fields here? Expressing
> symlinks can perhaps be done via a new XEN_HYPFS_* flag, but there
> may be other things that would be better to provide for even if
> there's no implementation from the beginning. It's just the case
> that changing this structure after the fact is going to be
> impossible, and it'll take new sub-ops then instead.
Okay, I'll make flags uint32_t and add a "uint16_t off_name". This
allows adding more fields later (e.g. depending on flags).
>
>> +/*
>> + * Hypercall operations.
>> + */
>> +
>> +/*
>> + * XEN_HYPFS_OP_read_contents
>> + *
>> + * Read contents of a filesystem entry.
>> + *
>> + * Returns the contents of an entry in the buffer supplied by the caller.
>> + * Only text data with a trailing zero byte is returned.
>> + *
>> + * arg1: XEN_GUEST_HANDLE(path name)
>> + * arg2: length of path name (including trailing zero byte)
>> + * arg3: XEN_GUEST_HANDLE(content buffer)
>> + * arg4: content buffer size
>> + *
>> + * Possible return values:
>> + * 0: success
>> + * -EPERM: operation not permitted
>> + * -ENOENT: entry not found
>> + * -EACCESS: access to entry not permitted
>> + * -EISDIR: entry is a directory
>> + * -EINVAL: invalid parameter
>
> I'm not convinced enumerating possible return value is a good idea.
> Down the road we're certain to forget extending this list. Plus
> extension would, strictly speaking, not even be allowed if these
> enumerations are considered part of the interface.
Okay, I'll drop listing them.
>
>> + * positive value: content buffer was too small, returned value is needed size
>
> Positive return values are problematic when reaching INT_MAX. Are you
> convinced we want to have yet another instance?
Are you convinced we want to return more then 2G long strings in one go?
>
>> + */
>> +#define XEN_HYPFS_OP_read_contents 1
>> +
>> +/*
>> + * XEN_HYPFS_OP_read_dir
>> + *
>> + * Read directory entries of a directory.
>> + *
>> + * Returns a struct xen_fs_direntry for each entry in a directory.
>> + *
>> + * arg1: XEN_GUEST_HANDLE(path name)
>> + * arg2: length of path name (including trailing zero byte)
>> + * arg3: XEN_GUEST_HANDLE(content buffer)
>> + * arg4: content buffer size
>> + *
>> + * Possible return values:
>> + * 0: success
>> + * -EPERM: operation not permitted
>> + * -ENOENT: entry not found
>> + * -EACCESS: access to entry not permitted
>> + * -ENOTDIR: entry is not a directory
>> + * -EINVAL: invalid parameter
>> + * positive value: content buffer was too small, returned value is needed size
>> + */
>> +#define XEN_HYPFS_OP_read_dir 2
>> +
>> +/*
>> + * XEN_HYPFS_OP_read_contents
>
> XEN_HYPFS_OP_write_contents
Oh, of yourse!
>
>> + * Write contents of a filesystem entry.
>> + *
>> + * Writes an entry with the contents of a buffer supplied by the caller.
>> + * Only text data with a trailing zero byte can be written.
>> + *
>> + * arg1: XEN_GUEST_HANDLE(path name)
>> + * arg2: length of path name (including trailing zero byte)
>> + * arg3: XEN_GUEST_HANDLE(content buffer)
>> + * arg4: content buffer size
>
> The latest here (in contrast to the read counterpart) I think it becomes
> desirable to identify what's IN and what's OUT.
Yes.
>
>> --- /dev/null
>> +++ b/xen/include/xen/hypfs.h
>> @@ -0,0 +1,40 @@
>> +#ifndef __XEN_HYPFS_H__
>> +#define __XEN_HYPFS_H__
>> +
>> +#include <xen/list.h>
>> +
>> +struct hypfs_dir {
>> + unsigned int content_size;
>> + struct list_head list;
>> +};
>> +
>> +enum hypfs_entry_type {
>> + hypfs_type_dir,
>> + hypfs_type_string,
>> + hypfs_type_uint
>> +};
>> +
>> +struct hypfs_entry {
>> + enum hypfs_entry_type type;
>> + const char *name;
>> + struct list_head list;
>> + struct hypfs_dir *parent;
>
> Afaict you set this field, but you never use it anywhere. Why do you
> add it in the first place? (Initially I meant to ask whether this
> can be pointer-to-const.)
It will be needed for cases where the entry is being changed, e.g.
when support for custom runtime parameters is added.
>
>> + union {
>> + void *content;
>
> const?
>
>> + struct hypfs_dir *dir;
>
> const?
As already said above: mixing const and non-const pointers in a
union looks fishy to me.
>
>> + char *str_val;
>> + unsigned int *uint_val;
>> + };
>> +};
>> +
>> +extern struct hypfs_dir hypfs_root;
>> +
>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>> + struct hypfs_dir *dir);
>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>> + char *val);
>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>> + unsigned int *val);
>
> Thinking about the lack of const on the last parameters here again -
> if these are for the pointed to values to be modifiable through
> this interface, then how would the "owning" component learn of the
> value having changed? Not everyone may need this, but I think there
> would want to be a callback. Until then perhaps better to add const
> here, promising that the values won't change behind the backs of
> the owners.
That's what hypfs_lock is for (and maybe later per-element locks).
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
2019-11-12 14:22 ` Jan Beulich
@ 2019-11-12 16:45 ` Jürgen Groß
2019-11-13 14:12 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Jürgen Groß @ 2019-11-12 16:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
On 12.11.19 15:22, Jan Beulich wrote:
> On 02.10.2019 13:20, Juergen Gross wrote:
>> Add the /buildinfo/config entry to the hypervisor filesystem. This
>> entry contains the .config file used to build the hypervisor.
>
> I think this is the 2nd step ahead of the 1st: Much of the stuff
> exposed as XENVER_* sub-ops should manifest itself here ahead of
> exposing xen/.config.
Yes and no. This is meant as a replacement for my previous patch series
adding .config read support.
It is no problem to add other data as well, but the need for being able
to read .config contents was already agreed on.
>
>> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
>>
>> subdir-$(CONFIG_NEEDS_LIBELF) += libelf
>> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>> +
>> +config_data.c: ../.config
>> + ( echo "char xen_config_data[] ="; \
>> + ../tools/bin2c <$<; \
>> + echo ";" ) > $@
>
> This is the typical kind of construct that may break (a subsequent
> build attempt) when interrupted in the middle. This pretty clearly
> is a move-if-changed candidate, at the same time also avoiding a
> (cheap, but anyway) pointless re-build in case .config was touched
> without actually changing.
Okay.
>
> Furthermore is there a reason to expose this as plain text, when
> Linux exposes a gzip-ed version in /proc? The file isn't very
> large now, but this was also the case for Linux many years ago.
gzip data may contain bytes with 0x00. Supporting that would require a
different interface at all levels.
>
>> --- a/xen/common/hypfs.c
>> +++ b/xen/common/hypfs.c
>> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
>> .dir = &hypfs_root,
>> };
>>
>> +static struct hypfs_dir hypfs_buildinfo = {
>> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list),
>> +};
>> +
>> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>> {
>> int ret = -ENOENT;
>> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
>>
>> return ret;
>> }
>> +
>> +static int __init hypfs_init(void)
>> +{
>> + int ret;
>> +
>> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
>> + BUG_ON(ret);
>> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
>> + BUG_ON(ret);
>> +
>> + return 0;
>> +}
>> +__initcall(hypfs_init);
>
> Hmm, do you really want to centralize population of the file system
> here, rather than having the individual components take care of it?
I can add a new source, e.g. common/buildinfo.c if you like that better.
>
>> --- a/xen/tools/Makefile
>> +++ b/xen/tools/Makefile
>> @@ -1,13 +1,18 @@
>>
>> include $(XEN_ROOT)/Config.mk
>>
>> +PROGS = symbols bin2c
>> +
>> .PHONY: default
>> default:
>> - $(MAKE) symbols
>> + $(MAKE) $(PROGS)
>
> Could I ask you to take the opportunity and do away with the
> unnecessary (as it seems to me) make recursion? $(PROGS) could
> easily become a dependency of "default" afaict.
Fine with me.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs
2019-11-12 14:28 ` Jan Beulich
@ 2019-11-12 16:46 ` Jürgen Groß
0 siblings, 0 replies; 18+ messages in thread
From: Jürgen Groß @ 2019-11-12 16:46 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
On 12.11.19 15:28, Jan Beulich wrote:
> On 02.10.2019 13:20, Juergen Gross wrote:
>> Add support to read values of hypervisor runtime parameters via the
>> hypervisor file system for all unsigned integer type runtime parameters.
>
> What about string ones (which you seem to handle in the code,
> but see also there)?
Oh, right, this was a late addition.
>
>> @@ -320,6 +321,44 @@ int cmdline_strcmp(const char *frag, const char *name)
>> }
>> }
>>
>> +static struct hypfs_dir hypfs_params = {
>> + .list = LIST_HEAD_INIT(hypfs_params.list),
>> +};
>> +
>> +static int __init runtime_param_hypfs_add(void)
>> +{
>> + const struct kernel_param *param;
>> + int ret;
>> +
>> + ret = hypfs_new_dir(&hypfs_root, "params", &hypfs_params);
>> + BUG_ON(ret);
>> +
>> + for ( param = __param_start; param < __param_end; param++ )
>> + {
>> + switch ( param->type )
>> + {
>> + case OPT_UINT:
>> + if ( param->len == sizeof(unsigned int) )
>> + ret = hypfs_new_entry_uint(&hypfs_params, param->name,
>> + (unsigned int *)(param->par.var));
>
> Stray pair or parentheses. I also don't see the need for the cast,
> with the "var" union member being "void *".
Right, will drop the cast.
>
>> + break;
>> +
>> + case OPT_STR:
>> + ret = hypfs_new_entry_uint(&hypfs_params, param->name,
>> + param->par.var);
>
> hypfs_new_entry_string()?
Yes.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
2019-11-12 16:06 ` Jürgen Groß
@ 2019-11-13 14:07 ` Jan Beulich
2019-11-13 14:40 ` Jürgen Groß
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-11-13 14:07 UTC (permalink / raw)
To: Jürgen Groß
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel,
Daniel de Graaf, Volodymyr Babchuk, Roger Pau Monné
On 12.11.2019 17:06, Jürgen Groß wrote:
> On 12.11.19 14:48, Jan Beulich wrote:
>> On 02.10.2019 13:20, Juergen Gross wrote:
>>> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>>> +{
>>> + int ret = -ENOENT;
>>> + struct list_head *l;
>>> +
>>> + if ( !new->content )
>>> + return -EINVAL;
>>> +
>>> + spin_lock(&hypfs_lock);
>>> +
>>> + list_for_each ( l, &parent->list )
>>> + {
>>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>>
>> const?
>
> Hmm, is this true when I add a new entry to it? l is part of *e
> after all.
But you don't use e in a way requiring it to be non-const. Question
is (as I did ask elsewhere already) whether you wouldn't better use
list_for_each_entry() here in the first place.
>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>> +{
>>> + unsigned int len = 0;
>>> +
>>> + switch ( entry->type )
>>> + {
>>> + case hypfs_type_dir:
>>> + len = entry->dir->content_size;
>>> + break;
>>> + case hypfs_type_string:
>>> + len = strlen(entry->str_val) + 1;
>>> + break;
>>> + case hypfs_type_uint:
>>> + len = 11; /* longest possible printed value + 1 */
>>
>> Why would uint values be restricted to 32 bits? There are plenty of
>> 64-bit numbers that might be of interest exposing through this
>> interface (and even more so if down the road we were to re-use this
>> for something debugfs-like). And even without this I think it would
>> be better to not have a literal number here - it'll be close to
>> unnoticeable (without someone remembering) when porting to an arch
>> with unsigned int wider than 32 bits.
>
> Support of 64-bit numbers would add "hypfs_type_ulong".
At this layer I dislike making assumptions on the bitness of int
or long. Can we settle on either a type that's suitable for all
sensible values (would likely be unsigned long long) or use fixed
with identifications (hypfs_type_u32 et al)?
> So basically something like "(sizeof(type) * 8 * 3 + 9) / 10 + 1" ?
> (equivalent to: 10 bits make roughly 3 digits, round that up and
> add 0-Byte). This is correct for 1, 2, 4 and 8 byte values. For 16
> byte values the result is 40, but it should be 41.
That's one option. The other - especially worthwhile to consider
for large numbers - is to represent them in hex.
>>> + break;
>>> + }
>>> +
>>> + return len;
>>> +}
>>> +
>>> +long do_hypfs_op(unsigned int cmd,
>>> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
>>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>>> +{
>>> + int ret;
>>> + struct hypfs_entry *entry;
>>> + unsigned int len;
>>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>>> +
>>> + if ( !is_control_domain(current->domain) &&
>>> + !is_hardware_domain(current->domain) )
>>> + return -EPERM;
>>
>> Replace by an XSM check?
>
> Yes, but I could need some help here. How do I add a new hypercall
> in XSM?
I guess we should rope in Daniel (now Cc-ed).
>>
>>> + spin_lock(&hypfs_lock);
>>
>> Wouldn't this better be an r/w lock from the beginning, requiring
>> only read access here?
>
> Depending on the further use cases we might even end up with per-element
> locks. I'm fine using a r/w lock for now.
Indeed I was thinking that write-value support would want a per-entry
lock, with the global one only guarding the directory tree.
>>> + ret = hypfs_get_path_user(path, arg1, arg2);
>>> + if ( ret )
>>> + goto out;
>>> +
>>> + entry = hypfs_get_entry(path);
>>> + if ( !entry )
>>> + {
>>> + ret = -ENOENT;
>>> + goto out;
>>> + }
>>> +
>>> + switch ( cmd )
>>> + {
>>> + case XEN_HYPFS_OP_read_contents:
>>> + {
>>> + char buf[12];
>>> + char *val = buf;
>>
>> const void *?
>
> Why void *? The result is always a string.
const char * might be fine too, but the code really doesn't depend
on this being other than void afaics.
>>> + * positive value: content buffer was too small, returned value is needed size
>>
>> Positive return values are problematic when reaching INT_MAX. Are you
>> convinced we want to have yet another instance?
>
> Are you convinced we want to return more then 2G long strings in one go?
Counter question: Are you convinced we'll stick to just strings?
See the gzip-ing question on the later patch for example.
>>> +struct hypfs_entry {
>>> + enum hypfs_entry_type type;
>>> + const char *name;
>>> + struct list_head list;
>>> + struct hypfs_dir *parent;
>>
>> Afaict you set this field, but you never use it anywhere. Why do you
>> add it in the first place? (Initially I meant to ask whether this
>> can be pointer-to-const.)
>
> It will be needed for cases where the entry is being changed, e.g.
> when support for custom runtime parameters is added.
Can we defer its introduction until then?
>>> + union {
>>> + void *content;
>>
>> const?
>>
>>> + struct hypfs_dir *dir;
>>
>> const?
>
> As already said above: mixing const and non-const pointers in a
> union looks fishy to me.
Hmm, well, I can see your point, but I think it still can be viewed
to have its (perhaps largely documentation) value.
>>> + char *str_val;
>>> + unsigned int *uint_val;
>>> + };
>>> +};
>>> +
>>> +extern struct hypfs_dir hypfs_root;
>>> +
>>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>>> + struct hypfs_dir *dir);
>>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>>> + char *val);
>>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>>> + unsigned int *val);
>>
>> Thinking about the lack of const on the last parameters here again -
>> if these are for the pointed to values to be modifiable through
>> this interface, then how would the "owning" component learn of the
>> value having changed? Not everyone may need this, but I think there
>> would want to be a callback. Until then perhaps better to add const
>> here, promising that the values won't change behind the backs of
>> the owners.
>
> That's what hypfs_lock is for (and maybe later per-element locks).
I don't understand: Are you intending random code to acquire this
lock?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
2019-11-12 16:45 ` Jürgen Groß
@ 2019-11-13 14:12 ` Jan Beulich
2019-11-13 14:41 ` Jürgen Groß
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-11-13 14:12 UTC (permalink / raw)
To: Jürgen Groß
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
On 12.11.2019 17:45, Jürgen Groß wrote:
> On 12.11.19 15:22, Jan Beulich wrote:
>> On 02.10.2019 13:20, Juergen Gross wrote:
>>> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
>>>
>>> subdir-$(CONFIG_NEEDS_LIBELF) += libelf
>>> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>>> +
>>> +config_data.c: ../.config
>>> + ( echo "char xen_config_data[] ="; \
>>> + ../tools/bin2c <$<; \
>>> + echo ";" ) > $@
>>
>> Furthermore is there a reason to expose this as plain text, when
>> Linux exposes a gzip-ed version in /proc? The file isn't very
>> large now, but this was also the case for Linux many years ago.
>
> gzip data may contain bytes with 0x00. Supporting that would require a
> different interface at all levels.
Then perhaps better do so now, when the code is still in flux, than
after the fact, especially if "at all levels" is meant to also
include the public interface?
>>> --- a/xen/common/hypfs.c
>>> +++ b/xen/common/hypfs.c
>>> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
>>> .dir = &hypfs_root,
>>> };
>>>
>>> +static struct hypfs_dir hypfs_buildinfo = {
>>> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list),
>>> +};
>>> +
>>> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>>> {
>>> int ret = -ENOENT;
>>> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
>>>
>>> return ret;
>>> }
>>> +
>>> +static int __init hypfs_init(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
>>> + BUG_ON(ret);
>>> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
>>> + BUG_ON(ret);
>>> +
>>> + return 0;
>>> +}
>>> +__initcall(hypfs_init);
>>
>> Hmm, do you really want to centralize population of the file system
>> here, rather than having the individual components take care of it?
>
> I can add a new source, e.g. common/buildinfo.c if you like that better.
I was rather thinking of moving this into common/kernel.c, next to the
version hypercall handling, and together with exposing the suggested
values here ahead of exposing .config.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
2019-11-13 14:07 ` Jan Beulich
@ 2019-11-13 14:40 ` Jürgen Groß
2019-11-13 14:47 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Jürgen Groß @ 2019-11-13 14:40 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel,
Daniel de Graaf, Volodymyr Babchuk, Roger Pau Monné
On 13.11.19 15:07, Jan Beulich wrote:
> On 12.11.2019 17:06, Jürgen Groß wrote:
>> On 12.11.19 14:48, Jan Beulich wrote:
>>> On 02.10.2019 13:20, Juergen Gross wrote:
>>>> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>>>> +{
>>>> + int ret = -ENOENT;
>>>> + struct list_head *l;
>>>> +
>>>> + if ( !new->content )
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock(&hypfs_lock);
>>>> +
>>>> + list_for_each ( l, &parent->list )
>>>> + {
>>>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>>>
>>> const?
>>
>> Hmm, is this true when I add a new entry to it? l is part of *e
>> after all.
>
> But you don't use e in a way requiring it to be non-const. Question
> is (as I did ask elsewhere already) whether you wouldn't better use
> list_for_each_entry() here in the first place.
I already agreed on doing that.
>
>>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>>> +{
>>>> + unsigned int len = 0;
>>>> +
>>>> + switch ( entry->type )
>>>> + {
>>>> + case hypfs_type_dir:
>>>> + len = entry->dir->content_size;
>>>> + break;
>>>> + case hypfs_type_string:
>>>> + len = strlen(entry->str_val) + 1;
>>>> + break;
>>>> + case hypfs_type_uint:
>>>> + len = 11; /* longest possible printed value + 1 */
>>>
>>> Why would uint values be restricted to 32 bits? There are plenty of
>>> 64-bit numbers that might be of interest exposing through this
>>> interface (and even more so if down the road we were to re-use this
>>> for something debugfs-like). And even without this I think it would
>>> be better to not have a literal number here - it'll be close to
>>> unnoticeable (without someone remembering) when porting to an arch
>>> with unsigned int wider than 32 bits.
>>
>> Support of 64-bit numbers would add "hypfs_type_ulong".
>
> At this layer I dislike making assumptions on the bitness of int
> or long. Can we settle on either a type that's suitable for all
> sensible values (would likely be unsigned long long) or use fixed
> with identifications (hypfs_type_u32 et al)?
This is a problem with e.g. runtime parameters. The current int type
parameters are unsigned int. So changing the type to hypfs_type_u32
would then make assumptions about unsigned int bitness.
My plan was to have hypfs_type_* according to the definitions of the
variables pointed to. Maybe the sensible way to handle that would be
to have hypfs_type_u(sz) similar to boot/runtime parameter handling.
>> So basically something like "(sizeof(type) * 8 * 3 + 9) / 10 + 1" ?
>> (equivalent to: 10 bits make roughly 3 digits, round that up and
>> add 0-Byte). This is correct for 1, 2, 4 and 8 byte values. For 16
>> byte values the result is 40, but it should be 41.
>
> That's one option. The other - especially worthwhile to consider
> for large numbers - is to represent them in hex.
Hmm, with your request regarding .config, maybe just transferring the
binary value and size might be the best option.
>
>>>> + break;
>>>> + }
>>>> +
>>>> + return len;
>>>> +}
>>>> +
>>>> +long do_hypfs_op(unsigned int cmd,
>>>> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
>>>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>>>> +{
>>>> + int ret;
>>>> + struct hypfs_entry *entry;
>>>> + unsigned int len;
>>>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>>>> +
>>>> + if ( !is_control_domain(current->domain) &&
>>>> + !is_hardware_domain(current->domain) )
>>>> + return -EPERM;
>>>
>>> Replace by an XSM check?
>>
>> Yes, but I could need some help here. How do I add a new hypercall
>> in XSM?
>
> I guess we should rope in Daniel (now Cc-ed).
>
>>>
>>>> + spin_lock(&hypfs_lock);
>>>
>>> Wouldn't this better be an r/w lock from the beginning, requiring
>>> only read access here?
>>
>> Depending on the further use cases we might even end up with per-element
>> locks. I'm fine using a r/w lock for now.
>
> Indeed I was thinking that write-value support would want a per-entry
> lock, with the global one only guarding the directory tree.
>
>>>> + ret = hypfs_get_path_user(path, arg1, arg2);
>>>> + if ( ret )
>>>> + goto out;
>>>> +
>>>> + entry = hypfs_get_entry(path);
>>>> + if ( !entry )
>>>> + {
>>>> + ret = -ENOENT;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + switch ( cmd )
>>>> + {
>>>> + case XEN_HYPFS_OP_read_contents:
>>>> + {
>>>> + char buf[12];
>>>> + char *val = buf;
>>>
>>> const void *?
>>
>> Why void *? The result is always a string.
>
> const char * might be fine too, but the code really doesn't depend
> on this being other than void afaics.
>
>>>> + * positive value: content buffer was too small, returned value is needed size
>>>
>>> Positive return values are problematic when reaching INT_MAX. Are you
>>> convinced we want to have yet another instance?
>>
>> Are you convinced we want to return more then 2G long strings in one go?
>
> Counter question: Are you convinced we'll stick to just strings?
> See the gzip-ing question on the later patch for example.
Nevertheless I'm questioning the idea to support GB sized buffers. This
seems to be a perfect way to ask for problems.
But with binary value support we'd need a size reported anyway, so this
should be settled then.
>
>>>> +struct hypfs_entry {
>>>> + enum hypfs_entry_type type;
>>>> + const char *name;
>>>> + struct list_head list;
>>>> + struct hypfs_dir *parent;
>>>
>>> Afaict you set this field, but you never use it anywhere. Why do you
>>> add it in the first place? (Initially I meant to ask whether this
>>> can be pointer-to-const.)
>>
>> It will be needed for cases where the entry is being changed, e.g.
>> when support for custom runtime parameters is added.
>
> Can we defer its introduction until then?
Okay.
>
>>>> + union {
>>>> + void *content;
>>>
>>> const?
>>>
>>>> + struct hypfs_dir *dir;
>>>
>>> const?
>>
>> As already said above: mixing const and non-const pointers in a
>> union looks fishy to me.
>
> Hmm, well, I can see your point, but I think it still can be viewed
> to have its (perhaps largely documentation) value.
So the void pointer shouldn't be const IMO as it can be used as a
replacement for all of the other union members. And the dir member is
used as non const in case of adding an entry.
>
>>>> + char *str_val;
>>>> + unsigned int *uint_val;
>>>> + };
>>>> +};
>>>> +
>>>> +extern struct hypfs_dir hypfs_root;
>>>> +
>>>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>>>> + struct hypfs_dir *dir);
>>>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>>>> + char *val);
>>>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>>>> + unsigned int *val);
>>>
>>> Thinking about the lack of const on the last parameters here again -
>>> if these are for the pointed to values to be modifiable through
>>> this interface, then how would the "owning" component learn of the
>>> value having changed? Not everyone may need this, but I think there
>>> would want to be a callback. Until then perhaps better to add const
>>> here, promising that the values won't change behind the backs of
>>> the owners.
>>
>> That's what hypfs_lock is for (and maybe later per-element locks).
>
> I don't understand: Are you intending random code to acquire this
> lock?
Oh, now I understand your initial question better.
You are right, in general cases a callback might be needed (just the
same as with custom parameters).
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
2019-11-13 14:12 ` Jan Beulich
@ 2019-11-13 14:41 ` Jürgen Groß
0 siblings, 0 replies; 18+ messages in thread
From: Jürgen Groß @ 2019-11-13 14:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
On 13.11.19 15:12, Jan Beulich wrote:
> On 12.11.2019 17:45, Jürgen Groß wrote:
>> On 12.11.19 15:22, Jan Beulich wrote:
>>> On 02.10.2019 13:20, Juergen Gross wrote:
>>>> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
>>>>
>>>> subdir-$(CONFIG_NEEDS_LIBELF) += libelf
>>>> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>>>> +
>>>> +config_data.c: ../.config
>>>> + ( echo "char xen_config_data[] ="; \
>>>> + ../tools/bin2c <$<; \
>>>> + echo ";" ) > $@
>>>
>>> Furthermore is there a reason to expose this as plain text, when
>>> Linux exposes a gzip-ed version in /proc? The file isn't very
>>> large now, but this was also the case for Linux many years ago.
>>
>> gzip data may contain bytes with 0x00. Supporting that would require a
>> different interface at all levels.
>
> Then perhaps better do so now, when the code is still in flux, than
> after the fact, especially if "at all levels" is meant to also
> include the public interface?
I'll have a look into that.
>
>>>> --- a/xen/common/hypfs.c
>>>> +++ b/xen/common/hypfs.c
>>>> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
>>>> .dir = &hypfs_root,
>>>> };
>>>>
>>>> +static struct hypfs_dir hypfs_buildinfo = {
>>>> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list),
>>>> +};
>>>> +
>>>> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>>>> {
>>>> int ret = -ENOENT;
>>>> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
>>>>
>>>> return ret;
>>>> }
>>>> +
>>>> +static int __init hypfs_init(void)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
>>>> + BUG_ON(ret);
>>>> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
>>>> + BUG_ON(ret);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +__initcall(hypfs_init);
>>>
>>> Hmm, do you really want to centralize population of the file system
>>> here, rather than having the individual components take care of it?
>>
>> I can add a new source, e.g. common/buildinfo.c if you like that better.
>
> I was rather thinking of moving this into common/kernel.c, next to the
> version hypercall handling, and together with exposing the suggested
> values here ahead of exposing .config.
Okay.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
2019-11-13 14:40 ` Jürgen Groß
@ 2019-11-13 14:47 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-11-13 14:47 UTC (permalink / raw)
To: Jürgen Groß
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel,
Daniel de Graaf, Volodymyr Babchuk, Roger Pau Monné
On 13.11.2019 15:40, Jürgen Groß wrote:
> On 13.11.19 15:07, Jan Beulich wrote:
>> On 12.11.2019 17:06, Jürgen Groß wrote:
>>> On 12.11.19 14:48, Jan Beulich wrote:
>>>> On 02.10.2019 13:20, Juergen Gross wrote:
>>>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>>>> +{
>>>>> + unsigned int len = 0;
>>>>> +
>>>>> + switch ( entry->type )
>>>>> + {
>>>>> + case hypfs_type_dir:
>>>>> + len = entry->dir->content_size;
>>>>> + break;
>>>>> + case hypfs_type_string:
>>>>> + len = strlen(entry->str_val) + 1;
>>>>> + break;
>>>>> + case hypfs_type_uint:
>>>>> + len = 11; /* longest possible printed value + 1 */
>>>>
>>>> Why would uint values be restricted to 32 bits? There are plenty of
>>>> 64-bit numbers that might be of interest exposing through this
>>>> interface (and even more so if down the road we were to re-use this
>>>> for something debugfs-like). And even without this I think it would
>>>> be better to not have a literal number here - it'll be close to
>>>> unnoticeable (without someone remembering) when porting to an arch
>>>> with unsigned int wider than 32 bits.
>>>
>>> Support of 64-bit numbers would add "hypfs_type_ulong".
>>
>> At this layer I dislike making assumptions on the bitness of int
>> or long. Can we settle on either a type that's suitable for all
>> sensible values (would likely be unsigned long long) or use fixed
>> with identifications (hypfs_type_u32 et al)?
>
> This is a problem with e.g. runtime parameters. The current int type
> parameters are unsigned int. So changing the type to hypfs_type_u32
> would then make assumptions about unsigned int bitness.
>
> My plan was to have hypfs_type_* according to the definitions of the
> variables pointed to. Maybe the sensible way to handle that would be
> to have hypfs_type_u(sz) similar to boot/runtime parameter handling.
Agreed.
>>>>> + union {
>>>>> + void *content;
>>>>
>>>> const?
>>>>
>>>>> + struct hypfs_dir *dir;
>>>>
>>>> const?
>>>
>>> As already said above: mixing const and non-const pointers in a
>>> union looks fishy to me.
>>
>> Hmm, well, I can see your point, but I think it still can be viewed
>> to have its (perhaps largely documentation) value.
>
> So the void pointer shouldn't be const IMO as it can be used as a
> replacement for all of the other union members.
But this was exactly the reason why I considered it to become
const - to disallow such use when it's about changing a value.
> And the dir member is
> used as non const in case of adding an entry.
Well, if there indeed is such a use (which I had looked for but
apparently overlooked), then of course const shouldn't be added.
Hence the question mark in my initial reply.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-11-13 14:47 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-02 11:19 [Xen-devel] [PATCH v2 0/6] Add hypervisor sysfs-like support Juergen Gross
2019-10-02 11:19 ` [Xen-devel] [PATCH v2 1/6] docs: add feature document for Xen " Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support Juergen Gross
2019-11-12 13:48 ` Jan Beulich
2019-11-12 16:06 ` Jürgen Groß
2019-11-13 14:07 ` Jan Beulich
2019-11-13 14:40 ` Jürgen Groß
2019-11-13 14:47 ` Jan Beulich
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 3/6] libs: add libxenhypfs Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 4/6] tools: add xenfs tool Juergen Gross
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
2019-11-12 14:22 ` Jan Beulich
2019-11-12 16:45 ` Jürgen Groß
2019-11-13 14:12 ` Jan Beulich
2019-11-13 14:41 ` Jürgen Groß
2019-10-02 11:20 ` [Xen-devel] [PATCH v2 6/6] xen: add runtime parameter reading support to hypfs Juergen Gross
2019-11-12 14:28 ` Jan Beulich
2019-11-12 16:46 ` Jürgen Groß
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.