* [PATCH v8 00/12] Add hypervisor sysfs-like support
@ 2020-05-08 15:34 Juergen Gross
2020-05-08 15:34 ` [PATCH v8 01/12] xen/vmx: let opt_ept_ad always reflect the current setting Juergen Gross
` (11 more replies)
0 siblings, 12 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Kevin Tian, Stefano Stabellini, Julien Grall,
Jun Nakajima, Wei Liu, Andrew Cooper, Ian Jackson, George Dunlap,
Jan Beulich, Anthony PERARD, Daniel De Graaf, 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.
The series adds read-only nodes with buildinfo data and writable
nodes with runtime parameters. xl is switched to use the new file
system for modifying the runtime parameters and the old sysctl
interface for that purpose is dropped.
Changes in V8:
- addressed review comments
- added CONFIG_HYPFS config option
Changes in V7:
- old patch 1 already applied
- add new patch 1 (carved out and modified from patch 9)
- addressed review comments
- modified public interface to have a max write size instead of a
writable flag only
Changes in V6:
- added new patches 1, 10, 11, 12
- addressed review comments
- modified interface for creating nodes for runtime parameters
Changes in V5:
- switched to xsm for privilege check
Changes in V4:
- former patch 2 removed as already committed
- addressed review comments
Changes in V3:
- major rework, especially by supporting binary contents of entries
- added several new patches (1, 2, 7)
- full support of all runtime parameters
- support of writing entries (especially runtime parameters)
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 (12):
xen/vmx: let opt_ept_ad always reflect the current setting
xen: add a generic way to include binary files as variables
docs: add feature document for Xen hypervisor sysfs-like support
xen: add basic hypervisor filesystem support
libs: add libxenhypfs
tools: add xenfs tool
xen: provide version information in hypfs
xen: add /buildinfo/config entry to hypervisor filesystem
xen: add runtime parameter access support to hypfs
tools/libxl: use libxenhypfs for setting xen runtime parameters
tools/libxc: remove xc_set_parameters()
xen: remove XEN_SYSCTL_set_parameter support
.gitignore | 6 +
docs/features/hypervisorfs.pandoc | 92 +++++
docs/man/xenhypfs.1.pod | 61 ++++
docs/misc/hypfs-paths.pandoc | 165 +++++++++
tools/Rules.mk | 8 +-
tools/flask/policy/modules/dom0.te | 4 +-
tools/libs/Makefile | 1 +
tools/libs/hypfs/Makefile | 16 +
tools/libs/hypfs/core.c | 536 ++++++++++++++++++++++++++++
tools/libs/hypfs/include/xenhypfs.h | 90 +++++
tools/libs/hypfs/libxenhypfs.map | 10 +
tools/libs/hypfs/xenhypfs.pc.in | 10 +
tools/libxc/include/xenctrl.h | 1 -
tools/libxc/xc_misc.c | 21 --
tools/libxl/Makefile | 3 +-
tools/libxl/libxl.c | 53 ++-
tools/libxl/libxl_internal.h | 1 +
tools/libxl/xenlight.pc.in | 2 +-
tools/misc/Makefile | 6 +
tools/misc/xenhypfs.c | 192 ++++++++++
tools/xl/xl_misc.c | 1 -
xen/arch/arm/traps.c | 3 +
xen/arch/arm/xen.lds.S | 12 +-
xen/arch/x86/hvm/hypercall.c | 3 +
xen/arch/x86/hvm/vmx/vmcs.c | 47 ++-
xen/arch/x86/hvm/vmx/vmx.c | 4 +-
xen/arch/x86/hypercall.c | 3 +
xen/arch/x86/pv/domain.c | 24 +-
xen/arch/x86/pv/hypercall.c | 3 +
xen/arch/x86/xen.lds.S | 12 +-
xen/common/Kconfig | 22 ++
xen/common/Makefile | 13 +
xen/common/grant_table.c | 62 +++-
xen/common/hypfs.c | 385 ++++++++++++++++++++
xen/common/kernel.c | 82 ++++-
xen/common/sysctl.c | 36 --
xen/drivers/char/console.c | 77 +++-
xen/include/Makefile | 1 +
xen/include/asm-x86/hvm/vmx/vmcs.h | 3 +-
xen/include/public/hypfs.h | 127 +++++++
xen/include/public/sysctl.h | 19 +-
xen/include/public/xen.h | 1 +
xen/include/xen/hypercall.h | 10 +
xen/include/xen/hypfs.h | 124 +++++++
xen/include/xen/kernel.h | 3 +
xen/include/xen/lib.h | 1 -
xen/include/xen/param.h | 123 +++++--
xen/include/xlat.lst | 2 +
xen/include/xsm/dummy.h | 6 +
xen/include/xsm/xsm.h | 6 +
xen/tools/binfile | 41 +++
xen/xsm/dummy.c | 1 +
xen/xsm/flask/Makefile | 5 +-
xen/xsm/flask/flask-policy.S | 16 -
xen/xsm/flask/hooks.c | 9 +-
xen/xsm/flask/policy/access_vectors | 4 +-
56 files changed, 2379 insertions(+), 190 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 100755 xen/tools/binfile
delete mode 100644 xen/xsm/flask/flask-policy.S
--
2.26.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v8 01/12] xen/vmx: let opt_ept_ad always reflect the current setting
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 02/12] xen: add a generic way to include binary files as variables Juergen Gross
` (10 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Kevin Tian, Jun Nakajima, Wei Liu, Andrew Cooper,
Jan Beulich, Roger Pau Monné
In case opt_ept_ad has not been set explicitly by the user via command
line or runtime parameter, it is treated as "no" on Avoton cpus.
Change that handling by setting opt_ept_ad to 0 for this cpu type
explicitly if no user value has been set.
By putting this into the (renamed) boot time initialization of vmcs.c
_vmx_cpu_up() can be made static.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/hvm/vmx/vmcs.c | 22 +++++++++++++++-------
xen/arch/x86/hvm/vmx/vmx.c | 4 +---
xen/include/asm-x86/hvm/vmx/vmcs.h | 3 +--
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 4c23645454..221af9737a 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -315,10 +315,6 @@ static int vmx_init_vmcs_config(void)
if ( !opt_ept_ad )
_vmx_ept_vpid_cap &= ~VMX_EPT_AD_BIT;
- else if ( /* Work around Erratum AVR41 on Avoton processors. */
- boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x4d &&
- opt_ept_ad < 0 )
- _vmx_ept_vpid_cap &= ~VMX_EPT_AD_BIT;
/*
* Additional sanity checking before using EPT:
@@ -652,7 +648,7 @@ void vmx_cpu_dead(unsigned int cpu)
vmx_pi_desc_fixup(cpu);
}
-int _vmx_cpu_up(bool bsp)
+static int _vmx_cpu_up(bool bsp)
{
u32 eax, edx;
int rc, bios_locked, cpu = smp_processor_id();
@@ -2108,9 +2104,21 @@ static void vmcs_dump(unsigned char ch)
printk("**************************************\n");
}
-void __init setup_vmcs_dump(void)
+int __init vmx_vmcs_init(void)
{
- register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
+ int ret;
+
+ if ( opt_ept_ad < 0 )
+ /* Work around Erratum AVR41 on Avoton processors. */
+ opt_ept_ad = !(boot_cpu_data.x86 == 6 &&
+ boot_cpu_data.x86_model == 0x4d);
+
+ ret = _vmx_cpu_up(true);
+
+ if ( !ret )
+ register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
+
+ return ret;
}
static void __init __maybe_unused build_assertions(void)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 6efa80e422..11a4dd94cf 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2482,7 +2482,7 @@ const struct hvm_function_table * __init start_vmx(void)
{
set_in_cr4(X86_CR4_VMXE);
- if ( _vmx_cpu_up(true) )
+ if ( vmx_vmcs_init() )
{
printk("VMX: failed to initialise.\n");
return NULL;
@@ -2553,8 +2553,6 @@ const struct hvm_function_table * __init start_vmx(void)
vmx_function_table.get_guest_bndcfgs = vmx_get_guest_bndcfgs;
}
- setup_vmcs_dump();
-
lbr_tsx_fixup_check();
bdf93_fixup_check();
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 95c1dea7b8..906810592f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -21,11 +21,10 @@
#include <xen/mm.h>
extern void vmcs_dump_vcpu(struct vcpu *v);
-extern void setup_vmcs_dump(void);
+extern int vmx_vmcs_init(void);
extern int vmx_cpu_up_prepare(unsigned int cpu);
extern void vmx_cpu_dead(unsigned int cpu);
extern int vmx_cpu_up(void);
-extern int _vmx_cpu_up(bool bsp);
extern void vmx_cpu_down(void);
struct vmcs_struct {
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 02/12] xen: add a generic way to include binary files as variables
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
2020-05-08 15:34 ` [PATCH v8 01/12] xen/vmx: let opt_ept_ad always reflect the current setting Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 03/12] docs: add feature document for Xen hypervisor sysfs-like support Juergen Gross
` (9 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich,
Daniel De Graaf
Add a new script xen/tools/binfile for including a binary file at build
time being usable via a pointer and a size variable in the hypervisor.
Make use of that generic tool in xsm.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wl@xen.org>
---
V3:
- new patch
V4:
- add alignment parameter (Jan Beulich)
- use .Lend instead of . (Jan Beulich)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 1 +
xen/tools/binfile | 41 ++++++++++++++++++++++++++++++++++++
xen/xsm/flask/Makefile | 5 ++++-
xen/xsm/flask/flask-policy.S | 16 --------------
4 files changed, 46 insertions(+), 17 deletions(-)
create mode 100755 xen/tools/binfile
delete mode 100644 xen/xsm/flask/flask-policy.S
diff --git a/.gitignore b/.gitignore
index 9c8a31f896..4059829f6e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -314,6 +314,7 @@ xen/test/livepatch/*.livepatch
xen/tools/kconfig/.tmp_gtkcheck
xen/tools/kconfig/.tmp_qtcheck
xen/tools/symbols
+xen/xsm/flask/flask-policy.S
xen/xsm/flask/include/av_perm_to_string.h
xen/xsm/flask/include/av_permissions.h
xen/xsm/flask/include/class_to_string.h
diff --git a/xen/tools/binfile b/xen/tools/binfile
new file mode 100755
index 0000000000..7bb35a5178
--- /dev/null
+++ b/xen/tools/binfile
@@ -0,0 +1,41 @@
+#!/bin/sh
+# usage: binfile [-i] [-a <align>] <target-src.S> <binary-file> <varname>
+# -a <align> align data at 2^<align> boundary (default: byte alignment)
+# -i add to .init.rodata (default: .rodata) section
+
+section=""
+align=0
+
+OPTIND=1
+while getopts "ia:" opt; do
+ case "$opt" in
+ i)
+ section=".init"
+ ;;
+ a)
+ align=$OPTARG
+ ;;
+ esac
+done
+
+target=$1
+binsource=$2
+varname=$3
+
+cat <<EOF >$target
+#include <asm/asm_defns.h>
+
+ .section $section.rodata, "a", %progbits
+
+ .p2align $align
+ .global $varname
+$varname:
+ .incbin "$binsource"
+.Lend:
+
+ .type $varname, %object
+ .size $varname, .Lend - $varname
+
+ .global ${varname}_size
+ ASM_INT(${varname}_size, .Lend - $varname)
+EOF
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index eebfceecc5..d8486fc7e4 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -39,6 +39,9 @@ $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE
obj-bin-$(CONFIG_XSM_FLASK_POLICY) += flask-policy.o
flask-policy.o: policy.bin
+flask-policy.S: $(XEN_ROOT)/xen/tools/binfile
+ $(XEN_ROOT)/xen/tools/binfile -i $@ policy.bin xsm_flask_init_policy
+
FLASK_BUILD_DIR := $(CURDIR)
POLICY_SRC := $(FLASK_BUILD_DIR)/xenpolicy-$(XEN_FULLVERSION)
@@ -48,4 +51,4 @@ policy.bin: FORCE
.PHONY: clean
clean::
- rm -f $(ALL_H_FILES) *.o $(DEPS_RM) policy.* $(POLICY_SRC)
+ rm -f $(ALL_H_FILES) *.o $(DEPS_RM) policy.* $(POLICY_SRC) flask-policy.S
diff --git a/xen/xsm/flask/flask-policy.S b/xen/xsm/flask/flask-policy.S
deleted file mode 100644
index d38aa39964..0000000000
--- a/xen/xsm/flask/flask-policy.S
+++ /dev/null
@@ -1,16 +0,0 @@
-#include <asm/asm_defns.h>
-
- .section .init.rodata, "a", %progbits
-
-/* const unsigned char xsm_flask_init_policy[] __initconst */
- .global xsm_flask_init_policy
-xsm_flask_init_policy:
- .incbin "policy.bin"
-.Lend:
-
- .type xsm_flask_init_policy, %object
- .size xsm_flask_init_policy, . - xsm_flask_init_policy
-
-/* const unsigned int __initconst xsm_flask_init_policy_size */
- .global xsm_flask_init_policy_size
- ASM_INT(xsm_flask_init_policy_size, .Lend - xsm_flask_init_policy)
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 03/12] docs: add feature document for Xen hypervisor sysfs-like support
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
2020-05-08 15:34 ` [PATCH v8 01/12] xen/vmx: let opt_ept_ad always reflect the current setting Juergen Gross
2020-05-08 15:34 ` [PATCH v8 02/12] xen: add a generic way to include binary files as variables Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 04/12] xen: add basic hypervisor filesystem support Juergen Gross
` (8 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap,
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>
Acked-by: Julien Grall <jgrall@amazon.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)
V3:
- correct introduction (writable entries)
V4:
- add list specification
- add entry example (Julien Grall)
- correct date and Xen version (Julien Grall)
- add ARM64 as possible architecture (Julien Grall)
- add version description to the feature doc (Jan Beulich)
V8:
- clarify syntax used in hypfs-paths.pandoc (George Dunlap)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
docs/features/hypervisorfs.pandoc | 92 +++++++++++++++++++++++++
docs/misc/hypfs-paths.pandoc | 107 ++++++++++++++++++++++++++++++
2 files changed, 199 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..a0a0ead057
--- /dev/null
+++ b/docs/features/hypervisorfs.pandoc
@@ -0,0 +1,92 @@
+% 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. Entries and directories are created by the hypervisor,
+while the toolstack is able to use a hypercall to query the entry
+values or (if allowed by the hypervisor) to modify them.
+
+# 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. This hypercall supports a sub-command
+XEN_HYPFS_OP_get_version which will return the highest version of the
+interface supported by the hypervisor. Additions to the interface need
+to bump the interface version. The hypervisor is required to support the
+previous interface versions, too (this implies that additions will always
+require new sub-commands in order to allow the hypervisor to decide which
+version of the interface to use).
+
+* hypercall interface specification
+ * `xen/include/public/hypfs.h`
+* hypervisor internal files
+ * `xen/include/xen/hypfs.h`
+ * `xen/common/hypfs.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
+---------- -------- -------- -------------------------------------------
+2020-01-23 1 Xen 4.14 Document written
+---------- -------- -------- -------------------------------------------
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
new file mode 100644
index 0000000000..39539fa1b5
--- /dev/null
+++ b/docs/misc/hypfs-paths.pandoc
@@ -0,0 +1,107 @@
+# 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 (note that this represents
+only the syntax used in this document):
+
+* 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
+ ")".
+* {VALUE, VALUE, ... } -- a list of possible values separated by "," and
+ 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 | ARM64 | 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.
+
+So an entry could look like this:
+
+ /cpu-bugs/active-pv/xpti = ("No"|{"dom0", "domU", "PCID-on"}) [w,X86,PV]
+
+Possible values would be "No" or a list of "dom0", "domU", and "PCID-on" with
+the list elements separated by spaces, e.g. "dom0 PCID-on".
+The entry would be writable and it would exist on X86 only and only if the
+hypervisor is configured to support PV guests.
+
+## 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.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (2 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 03/12] docs: add feature document for Xen hypervisor sysfs-like support Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-14 7:59 ` Jan Beulich
2020-05-14 9:55 ` Jan Beulich
2020-05-08 15:34 ` [PATCH v8 05/12] libs: add libxenhypfs Juergen Gross
` (7 subsequent siblings)
11 siblings, 2 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich,
Daniel De Graaf, 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.
In order not to have to repeat the same pattern multiple times in case
adding a new node should BUG_ON() failure, the helpers for adding a
node (hypfs_add_dir() and hypfs_add_leaf()) get a nofault parameter
causing the BUG() in case of a failure.
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
V3:
- major rework, especially by supporting binary contents of entries
- addressed all comments
V4:
- sort #includes alphabetically (Wei Liu)
- add public interface structures to xlat.lst (Jan Beulich)
- let DIRENTRY_SIZE() add 1 for trailing nul byte (Jan Beulich)
- remove hypfs_add_entry() (Jan Beulich)
- len -> ulen (Jan Beulich)
- switch sequence of tests in hypfs_get_entry_rel() (Jan Beulich)
- add const qualifier (Jan Beulich)
- return -ENOBUFS if only direntry but no entry contents are returned
(Jan Beulich)
- use xmalloc() instead of xzalloc() (Jan Beulich)
- better error handling in hypfs_write_leaf() (Jan Beulich)
- return -EOPNOTSUPP for unknown sub-command (Jan Beulich)
- use plain integers for enum-like constants in public interface
(Jan Beulich)
- rename XEN_HYPFS_OP_read_contents to XEN_HYPFS_OP_read (Jan Beulich)
- add some comments in include/public/hypfs.h (Jan Beulich)
- use const_char for user parameter path (Jan Beulich)
- add helpers for XEN_HYPFS_TYPE_BOOL and XEN_HYPFS_TYPE_INT entry
definitions (Jan Beulich)
- make statically defined entries __read_mostly (Jan Beulich)
V5:
- switch to xsm for privilege check
V6:
- use memchr() for testing correct string length (Jan Beulich)
- reject writing to non-string leafs with wrong length (Jan Beulich)
- only support bools of natural size (Julien Grall)
- adjust blank padding in header (Jan Beulich)
- adjust comments in public header (Jan Beulich)
- rename hypfs_string_set() and add comment (Jan Beulich)
- add common HYPFS_INIT helper macro (Jan Beulich)
- really check structures added to xlat.lst (Jan Beulich)
- add missing xsm parts (Jan Beulich)
V7:
- simplify compat check (Jan Beulich)
- add max write size (Jan Beulich)
- better length testing of written string (Jan Beulich)
V8:
- add Kconfig item CONFIG_HYPFS (Jan Beulich)
- init write pointer in HYPFS_*_INIT_WRITABLE() (Jan Beulich)
- expand write ASSERT()s (Jan Beulich)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/flask/policy/modules/dom0.te | 2 +-
xen/arch/arm/traps.c | 3 +
xen/arch/x86/hvm/hypercall.c | 3 +
xen/arch/x86/hypercall.c | 3 +
xen/arch/x86/pv/hypercall.c | 3 +
xen/common/Kconfig | 11 +
xen/common/Makefile | 1 +
xen/common/hypfs.c | 354 ++++++++++++++++++++++++++++
xen/include/Makefile | 1 +
xen/include/public/hypfs.h | 127 ++++++++++
xen/include/public/xen.h | 1 +
xen/include/xen/hypercall.h | 10 +
xen/include/xen/hypfs.h | 122 ++++++++++
xen/include/xlat.lst | 2 +
xen/include/xsm/dummy.h | 6 +
xen/include/xsm/xsm.h | 6 +
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 6 +
xen/xsm/flask/policy/access_vectors | 2 +
19 files changed, 663 insertions(+), 1 deletion(-)
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/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 272f6a4f75..20925e38a2 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -11,7 +11,7 @@ allow dom0_t xen_t:xen {
mtrr_del mtrr_read microcode physinfo quirk writeconsole readapic
writeapic privprofile nonprivprofile kexec firmware sleep frequency
getidle debug getcpuinfo heap pm_op mca_op lockprof cpupool_op
- getscheduler setscheduler
+ getscheduler setscheduler hypfs_op
};
allow dom0_t xen_t:xen2 {
resource_op psr_cmt_op psr_alloc pmu_ctrl get_symbol
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 30c4c1830b..8f40d0e0b6 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1381,6 +1381,9 @@ static arm_hypercall_t arm_hypercall_table[] = {
#ifdef CONFIG_ARGO
HYPERCALL(argo_op, 5),
#endif
+#ifdef CONFIG_HYPFS
+ HYPERCALL(hypfs_op, 5),
+#endif
};
#ifndef NDEBUG
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index c41c2179c9..b6ccaf4457 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -150,6 +150,9 @@ static const hypercall_table_t hvm_hypercall_table[] = {
#endif
HYPERCALL(xenpmu_op),
COMPAT_CALL(dm_op),
+#ifdef CONFIG_HYPFS
+ HYPERCALL(hypfs_op),
+#endif
HYPERCALL(arch_1)
};
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 7f299d45c6..dd00983005 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -72,6 +72,9 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
#ifdef CONFIG_HVM
ARGS(hvm_op, 2),
ARGS(dm_op, 3),
+#endif
+#ifdef CONFIG_HYPFS
+ ARGS(hypfs_op, 5),
#endif
ARGS(mca, 1),
ARGS(arch_1, 1),
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index b0d1d0ed77..53a52360fa 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -84,6 +84,9 @@ const hypercall_table_t pv_hypercall_table[] = {
#ifdef CONFIG_HVM
HYPERCALL(hvm_op),
COMPAT_CALL(dm_op),
+#endif
+#ifdef CONFIG_HYPFS
+ HYPERCALL(hypfs_op),
#endif
HYPERCALL(mca),
HYPERCALL(arch_1),
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index a6914fcae9..2515e053c8 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -116,6 +116,17 @@ config SPECULATIVE_HARDEN_BRANCH
endmenu
+config HYPFS
+ bool "Hypervisor file system support"
+ default y
+ ---help---
+ Support Xen hypervisor file system. This file system is used to
+ present various hypervisor internal data to dom0 and in some
+ cases to allow modifying settings. Disabling the support might
+ result in some features not being available.
+
+ If unsure, say Y.
+
config KEXEC
bool "kexec support"
default y
diff --git a/xen/common/Makefile b/xen/common/Makefile
index e8cde65370..98b7904dcd 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -10,6 +10,7 @@ obj-y += domain.o
obj-y += event_2l.o
obj-y += event_channel.o
obj-y += event_fifo.o
+obj-$(CONFIG_HYPFS) += 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..a873d507d1
--- /dev/null
+++ b/xen/common/hypfs.c
@@ -0,0 +1,354 @@
+/******************************************************************************
+ *
+ * hypfs.c
+ *
+ * Simple sysfs-like file system for the hypervisor.
+ */
+
+#include <xen/err.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
+#include <xen/hypfs.h>
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+#include <public/hypfs.h>
+
+#ifdef CONFIG_COMPAT
+#include <compat/hypfs.h>
+CHECK_hypfs_dirlistentry;
+#endif
+
+#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
+#define DIRENTRY_SIZE(name_len) \
+ (DIRENTRY_NAME_OFF + \
+ ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
+
+static DEFINE_RWLOCK(hypfs_lock);
+
+HYPFS_DIR_INIT(hypfs_root, "");
+
+static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new)
+{
+ int ret = -ENOENT;
+ struct hypfs_entry *e;
+
+ write_lock(&hypfs_lock);
+
+ list_for_each_entry ( e, &parent->dirlist, list )
+ {
+ int cmp = strcmp(e->name, new->name);
+
+ if ( cmp > 0 )
+ {
+ ret = 0;
+ list_add_tail(&new->list, &e->list);
+ break;
+ }
+ if ( cmp == 0 )
+ {
+ ret = -EEXIST;
+ break;
+ }
+ }
+
+ if ( ret == -ENOENT )
+ {
+ ret = 0;
+ list_add_tail(&new->list, &parent->dirlist);
+ }
+
+ if ( !ret )
+ {
+ unsigned int sz = strlen(new->name);
+
+ parent->e.size += DIRENTRY_SIZE(sz);
+ }
+
+ write_unlock(&hypfs_lock);
+
+ return ret;
+}
+
+int hypfs_add_dir(struct hypfs_entry_dir *parent,
+ struct hypfs_entry_dir *dir, bool nofault)
+{
+ int ret;
+
+ ret = add_entry(parent, &dir->e);
+ BUG_ON(nofault && ret);
+
+ return ret;
+}
+
+int hypfs_add_leaf(struct hypfs_entry_dir *parent,
+ struct hypfs_entry_leaf *leaf, bool nofault)
+{
+ int ret;
+
+ if ( !leaf->content )
+ ret = -EINVAL;
+ else
+ ret = add_entry(parent, &leaf->e);
+ BUG_ON(nofault && ret);
+
+ return ret;
+}
+
+static int hypfs_get_path_user(char *buf,
+ XEN_GUEST_HANDLE_PARAM(const_char) uaddr,
+ unsigned long ulen)
+{
+ if ( ulen > XEN_HYPFS_MAX_PATHLEN )
+ return -EINVAL;
+
+ if ( copy_from_guest(buf, uaddr, ulen) )
+ return -EFAULT;
+
+ if ( memchr(buf, 0, ulen) != buf + ulen - 1 )
+ return -EINVAL;
+
+ return 0;
+}
+
+static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
+ const char *path)
+{
+ const char *end;
+ struct hypfs_entry *entry;
+ unsigned int name_len;
+
+ if ( dir->e.type != XEN_HYPFS_TYPE_DIR )
+ return NULL;
+
+ if ( !*path )
+ return &dir->e;
+
+ end = strchr(path, '/');
+ if ( !end )
+ end = strchr(path, '\0');
+ name_len = end - path;
+
+ list_for_each_entry ( entry, &dir->dirlist, list )
+ {
+ int cmp = strncmp(path, entry->name, name_len);
+ struct hypfs_entry_dir *d = container_of(entry,
+ struct hypfs_entry_dir, e);
+
+ if ( cmp < 0 )
+ return NULL;
+ if ( !cmp && strlen(entry->name) == name_len )
+ return *end ? hypfs_get_entry_rel(d, end + 1) : entry;
+ }
+
+ return NULL;
+}
+
+struct hypfs_entry *hypfs_get_entry(const char *path)
+{
+ if ( path[0] != '/' )
+ return NULL;
+
+ return hypfs_get_entry_rel(&hypfs_root, path + 1);
+}
+
+int hypfs_read_dir(const struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+ const struct hypfs_entry_dir *d;
+ const struct hypfs_entry *e;
+ unsigned int size = entry->size;
+
+ d = container_of(entry, const struct hypfs_entry_dir, e);
+
+ list_for_each_entry ( e, &d->dirlist, list )
+ {
+ struct xen_hypfs_dirlistentry direntry;
+ unsigned int e_namelen = strlen(e->name);
+ unsigned int e_len = DIRENTRY_SIZE(e_namelen);
+
+ direntry.e.pad = 0;
+ direntry.e.type = e->type;
+ direntry.e.encoding = e->encoding;
+ direntry.e.content_len = e->size;
+ direntry.e.max_write_len = e->max_size;
+ direntry.off_next = list_is_last(&e->list, &d->dirlist) ? 0 : e_len;
+ if ( copy_to_guest(uaddr, &direntry, 1) )
+ return -EFAULT;
+
+ if ( copy_to_guest_offset(uaddr, DIRENTRY_NAME_OFF,
+ e->name, e_namelen + 1) )
+ return -EFAULT;
+
+ guest_handle_add_offset(uaddr, e_len);
+
+ ASSERT(e_len <= size);
+ size -= e_len;
+ }
+
+ return 0;
+}
+
+int hypfs_read_leaf(const struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+ const struct hypfs_entry_leaf *l;
+
+ l = container_of(entry, const struct hypfs_entry_leaf, e);
+
+ return copy_to_guest(uaddr, l->content, entry->size) ? -EFAULT: 0;
+}
+
+static int hypfs_read(const struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
+{
+ struct xen_hypfs_direntry e;
+ long ret = -EINVAL;
+
+ if ( ulen < sizeof(e) )
+ goto out;
+
+ e.pad = 0;
+ e.type = entry->type;
+ e.encoding = entry->encoding;
+ e.content_len = entry->size;
+ e.max_write_len = entry->max_size;
+
+ ret = -EFAULT;
+ if ( copy_to_guest(uaddr, &e, 1) )
+ goto out;
+
+ ret = -ENOBUFS;
+ if ( ulen < entry->size + sizeof(e) )
+ goto out;
+
+ guest_handle_add_offset(uaddr, sizeof(e));
+
+ ret = entry->read(entry, uaddr);
+
+ out:
+ return ret;
+}
+
+int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
+{
+ char *buf;
+ int ret;
+
+ if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
+ leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
+ return -EDOM;
+
+ buf = xmalloc_array(char, ulen);
+ if ( !buf )
+ return -ENOMEM;
+
+ ret = -EFAULT;
+ if ( copy_from_guest(buf, uaddr, ulen) )
+ goto out;
+
+ ret = -EINVAL;
+ if ( leaf->e.type == XEN_HYPFS_TYPE_STRING &&
+ memchr(buf, 0, ulen) != (buf + ulen - 1) )
+ goto out;
+
+ ret = 0;
+ memcpy(leaf->write_ptr, buf, ulen);
+ leaf->e.size = ulen;
+
+ out:
+ xfree(buf);
+ return ret;
+}
+
+int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
+{
+ bool buf;
+
+ ASSERT(leaf->e.type == XEN_HYPFS_TYPE_BOOL &&
+ leaf->e.size == sizeof(bool) &&
+ leaf->e.max_size == sizeof(bool) );
+
+ if ( ulen != leaf->e.max_size )
+ return -EDOM;
+
+ if ( copy_from_guest(&buf, uaddr, ulen) )
+ return -EFAULT;
+
+ *(bool *)leaf->write_ptr = buf;
+
+ return 0;
+}
+
+static int hypfs_write(struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
+{
+ struct hypfs_entry_leaf *l;
+
+ if ( !entry->write )
+ return -EACCES;
+
+ ASSERT(entry->max_size);
+
+ if ( ulen > entry->max_size )
+ return -ENOSPC;
+
+ l = container_of(entry, struct hypfs_entry_leaf, e);
+
+ return entry->write(l, uaddr, ulen);
+}
+
+long do_hypfs_op(unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(const_char) arg1, unsigned long arg2,
+ XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
+{
+ int ret;
+ struct hypfs_entry *entry;
+ static char path[XEN_HYPFS_MAX_PATHLEN];
+
+ if ( xsm_hypfs_op(XSM_PRIV) )
+ return -EPERM;
+
+ if ( cmd == XEN_HYPFS_OP_get_version )
+ return XEN_HYPFS_VERSION;
+
+ if ( cmd == XEN_HYPFS_OP_write_contents )
+ write_lock(&hypfs_lock);
+ else
+ read_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:
+ ret = hypfs_read(entry, arg3, arg4);
+ break;
+
+ case XEN_HYPFS_OP_write_contents:
+ ret = hypfs_write(entry, arg3, arg4);
+ break;
+
+ default:
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ out:
+ if ( cmd == XEN_HYPFS_OP_write_contents )
+ write_unlock(&hypfs_lock);
+ else
+ read_unlock(&hypfs_lock);
+
+ return ret;
+}
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 2a10725d68..089314dc72 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -9,6 +9,7 @@ headers-y := \
compat/event_channel.h \
compat/features.h \
compat/grant_table.h \
+ compat/hypfs.h \
compat/kexec.h \
compat/memory.h \
compat/nmi.h \
diff --git a/xen/include/public/hypfs.h b/xen/include/public/hypfs.h
new file mode 100644
index 0000000000..f1c1b3e935
--- /dev/null
+++ b/xen/include/public/hypfs.h
@@ -0,0 +1,127 @@
+/******************************************************************************
+ * 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.
+ */
+
+/* Highest version number of the hypfs interface currently defined. */
+#define XEN_HYPFS_VERSION 1
+
+/* Maximum length of a path in the filesystem. */
+#define XEN_HYPFS_MAX_PATHLEN 1024
+
+struct xen_hypfs_direntry {
+ uint8_t type;
+#define XEN_HYPFS_TYPE_DIR 0
+#define XEN_HYPFS_TYPE_BLOB 1
+#define XEN_HYPFS_TYPE_STRING 2
+#define XEN_HYPFS_TYPE_UINT 3
+#define XEN_HYPFS_TYPE_INT 4
+#define XEN_HYPFS_TYPE_BOOL 5
+ uint8_t encoding;
+#define XEN_HYPFS_ENC_PLAIN 0
+#define XEN_HYPFS_ENC_GZIP 1
+ uint16_t pad; /* Returned as 0. */
+ uint32_t content_len; /* Current length of data. */
+ uint32_t max_write_len; /* Max. length for writes (0 if read-only). */
+};
+
+struct xen_hypfs_dirlistentry {
+ struct xen_hypfs_direntry e;
+ /* Offset in bytes to next entry (0 == this is the last entry). */
+ uint16_t off_next;
+ /* Zero terminated entry name, possibly with some padding for alignment. */
+ char name[XEN_FLEX_ARRAY_DIM];
+};
+
+/*
+ * Hypercall operations.
+ */
+
+/*
+ * XEN_HYPFS_OP_get_version
+ *
+ * Read highest interface version supported by the hypervisor.
+ *
+ * Possible return values:
+ * >0: highest supported interface version
+ * <0: negative Xen errno value
+ */
+#define XEN_HYPFS_OP_get_version 0
+
+/*
+ * XEN_HYPFS_OP_read
+ *
+ * Read a filesystem entry.
+ *
+ * Returns the direntry and contents of an entry in the buffer supplied by the
+ * caller (struct xen_hypfs_direntry with the contents following directly
+ * after it).
+ * The data buffer must be at least the size of the direntry returned. If the
+ * data buffer was not large enough for all the data -ENOBUFS and no entry
+ * data is returned, but the direntry will contain the needed size for the
+ * returned data.
+ * The format of the contents is according to its entry type and encoding.
+ * The contents of a directory are multiple struct xen_hypfs_dirlistentry
+ * items.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(data buffer written by hypervisor)
+ * arg4: data buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * <0 : negative Xen errno value
+ */
+#define XEN_HYPFS_OP_read 1
+
+/*
+ * XEN_HYPFS_OP_write_contents
+ *
+ * Write contents of a filesystem entry.
+ *
+ * Writes an entry with the contents of a buffer supplied by the caller.
+ * The data type and encoding can't be changed. The size can be changed only
+ * for blobs and strings.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer read by hypervisor)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * <0 : negative Xen errno value
+ */
+#define XEN_HYPFS_OP_write_contents 2
+
+#endif /* __XEN_PUBLIC_HYPFS_H__ */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 75b1619d0d..945ef30273 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 d82a293377..655acc7f47 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -150,6 +150,16 @@ do_dm_op(
unsigned int nr_bufs,
XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
+#ifdef CONFIG_HYPFS
+extern long
+do_hypfs_op(
+ unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(const_char) arg1,
+ unsigned long arg2,
+ XEN_GUEST_HANDLE_PARAM(void) arg3,
+ unsigned long arg4);
+#endif
+
#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..279bcf3b4c
--- /dev/null
+++ b/xen/include/xen/hypfs.h
@@ -0,0 +1,122 @@
+#ifndef __XEN_HYPFS_H__
+#define __XEN_HYPFS_H__
+
+#ifdef CONFIG_HYPFS
+#include <xen/list.h>
+#include <xen/string.h>
+#include <public/hypfs.h>
+
+struct hypfs_entry_leaf;
+
+struct hypfs_entry {
+ unsigned short type;
+ unsigned short encoding;
+ unsigned int size;
+ unsigned int max_size;
+ const char *name;
+ struct list_head list;
+ int (*read)(const struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr);
+ int (*write)(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
+};
+
+struct hypfs_entry_leaf {
+ struct hypfs_entry e;
+ union {
+ const void *content;
+ void *write_ptr;
+ };
+};
+
+struct hypfs_entry_dir {
+ struct hypfs_entry e;
+ struct list_head dirlist;
+};
+
+#define HYPFS_DIR_INIT(var, nam) \
+ struct hypfs_entry_dir __read_mostly var = { \
+ .e.type = XEN_HYPFS_TYPE_DIR, \
+ .e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .e.name = nam, \
+ .e.size = 0, \
+ .e.max_size = 0, \
+ .e.list = LIST_HEAD_INIT(var.e.list), \
+ .e.read = hypfs_read_dir, \
+ .dirlist = LIST_HEAD_INIT(var.dirlist), \
+ }
+
+#define HYPFS_VARSIZE_INIT(var, typ, nam, msz) \
+ struct hypfs_entry_leaf __read_mostly var = { \
+ .e.type = typ, \
+ .e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .e.name = nam, \
+ .e.max_size = msz, \
+ .e.read = hypfs_read_leaf, \
+ }
+
+/* Content and size need to be set via hypfs_string_set_reference(). */
+#define HYPFS_STRING_INIT(var, nam) \
+ HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0)
+
+/*
+ * Set content and size of a XEN_HYPFS_TYPE_STRING node. The node will point
+ * to str, so any later modification of *str should be followed by a call
+ * to hypfs_string_set_reference() in order to update the size of the node
+ * data.
+ */
+static inline void hypfs_string_set_reference(struct hypfs_entry_leaf *leaf,
+ const char *str)
+{
+ leaf->content = str;
+ leaf->e.size = strlen(str) + 1;
+}
+
+#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
+ struct hypfs_entry_leaf __read_mostly var = { \
+ .e.type = typ, \
+ .e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .e.name = nam, \
+ .e.size = sizeof(contvar), \
+ .e.max_size = wr ? sizeof(contvar) : 0, \
+ .e.read = hypfs_read_leaf, \
+ .e.write = wr, \
+ .content = &contvar, \
+ }
+
+#define HYPFS_UINT_INIT(var, nam, contvar) \
+ HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, NULL)
+#define HYPFS_UINT_INIT_WRITABLE(var, nam, contvar) \
+ HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, \
+ hypfs_write_leaf)
+
+#define HYPFS_INT_INIT(var, nam, contvar) \
+ HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar, NULL)
+#define HYPFS_INT_INIT_WRITABLE(var, nam, contvar) \
+ HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar, \
+ hypfs_write_leaf)
+
+#define HYPFS_BOOL_INIT(var, nam, contvar) \
+ HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, NULL)
+#define HYPFS_BOOL_INIT_WRITABLE(var, nam, contvar) \
+ HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
+ hypfs_write_bool)
+
+extern struct hypfs_entry_dir hypfs_root;
+
+struct hypfs_entry *hypfs_get_entry(const char *path);
+int hypfs_add_dir(struct hypfs_entry_dir *parent,
+ struct hypfs_entry_dir *dir, bool nofault);
+int hypfs_add_leaf(struct hypfs_entry_dir *parent,
+ struct hypfs_entry_leaf *leaf, bool nofault);
+int hypfs_read_dir(const struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr);
+int hypfs_read_leaf(const struct hypfs_entry *entry,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr);
+int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
+int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
+#endif
+
+#endif /* __XEN_HYPFS_H__ */
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 95f5e5592b..0921d4a8d0 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -86,6 +86,8 @@
? vcpu_hvm_context hvm/hvm_vcpu.h
? vcpu_hvm_x86_32 hvm/hvm_vcpu.h
? vcpu_hvm_x86_64 hvm/hvm_vcpu.h
+? hypfs_direntry hypfs.h
+? hypfs_dirlistentry hypfs.h
? kexec_exec kexec.h
! kexec_image kexec.h
! kexec_range kexec.h
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 295dd67c48..2368acebed 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -434,6 +434,12 @@ static XSM_INLINE int xsm_page_offline(XSM_DEFAULT_ARG uint32_t cmd)
return xsm_default_action(action, current->domain, NULL);
}
+static XSM_INLINE int xsm_hypfs_op(XSM_DEFAULT_VOID)
+{
+ XSM_ASSERT_ACTION(XSM_PRIV);
+ return xsm_default_action(action, current->domain, NULL);
+}
+
static XSM_INLINE long xsm_do_xsm_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) op)
{
return -ENOSYS;
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..a80bcf3e42 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -127,6 +127,7 @@ struct xsm_operations {
int (*resource_setup_misc) (void);
int (*page_offline)(uint32_t cmd);
+ int (*hypfs_op)(void);
long (*do_xsm_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op);
#ifdef CONFIG_COMPAT
@@ -536,6 +537,11 @@ static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
return xsm_ops->page_offline(cmd);
}
+static inline int xsm_hypfs_op(xsm_default_t def)
+{
+ return xsm_ops->hypfs_op();
+}
+
static inline long xsm_do_xsm_op (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op)
{
return xsm_ops->do_xsm_op(op);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 5705e52791..d4cce68089 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -103,6 +103,7 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, resource_setup_misc);
set_to_dummy_if_null(ops, page_offline);
+ set_to_dummy_if_null(ops, hypfs_op);
set_to_dummy_if_null(ops, hvm_param);
set_to_dummy_if_null(ops, hvm_control);
set_to_dummy_if_null(ops, hvm_param_nested);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 4649e6fd95..a2c78e445c 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1173,6 +1173,11 @@ static inline int flask_page_offline(uint32_t cmd)
}
}
+static inline int flask_hypfs_op(void)
+{
+ return domain_has_xen(current->domain, XEN__HYPFS_OP);
+}
+
static int flask_add_to_physmap(struct domain *d1, struct domain *d2)
{
return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
@@ -1812,6 +1817,7 @@ static struct xsm_operations flask_ops = {
.resource_setup_misc = flask_resource_setup_misc,
.page_offline = flask_page_offline,
+ .hypfs_op = flask_hypfs_op,
.hvm_param = flask_hvm_param,
.hvm_control = flask_hvm_param,
.hvm_param_nested = flask_hvm_param_nested,
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index c055c14c26..c9e385fb9b 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -67,6 +67,8 @@ class xen
lockprof
# XEN_SYSCTL_cpupool_op
cpupool_op
+# hypfs hypercall
+ hypfs_op
# XEN_SYSCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_getinfo, XEN_SYSCTL_sched_id, XEN_DOMCTL_SCHEDOP_getvcpuinfo
getscheduler
# XEN_SYSCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_putinfo, XEN_DOMCTL_SCHEDOP_putvcpuinfo
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 05/12] libs: add libxenhypfs
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (3 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 04/12] xen: add basic hypervisor filesystem support Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 06/12] tools: add xenfs tool Juergen Gross
` (6 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich
Add the new library libxenhypfs for access to the hypervisor filesystem.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wl@xen.org>
---
V1:
- rename to libxenhypfs
- add xenhypfs_write()
V3:
- major rework due to new hypervisor interface
- add decompression capability
V4:
- add dependency to libz in pkgconfig file (Wei Liu)
V7:
- don't assume hypervisor's sizeof(bool) is same as in user land
V8:
- add some comments regarding the semantics of the lib functions
(George Dunlap)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 2 +
tools/Rules.mk | 6 +
tools/libs/Makefile | 1 +
tools/libs/hypfs/Makefile | 16 +
tools/libs/hypfs/core.c | 536 ++++++++++++++++++++++++++++
tools/libs/hypfs/include/xenhypfs.h | 90 +++++
tools/libs/hypfs/libxenhypfs.map | 10 +
tools/libs/hypfs/xenhypfs.pc.in | 10 +
8 files changed, 671 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/.gitignore b/.gitignore
index 4059829f6e..41b1d4c8a1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -110,6 +110,8 @@ tools/libs/evtchn/headers.chk
tools/libs/evtchn/xenevtchn.pc
tools/libs/gnttab/headers.chk
tools/libs/gnttab/xengnttab.pc
+tools/libs/hypfs/headers.chk
+tools/libs/hypfs/xenhypfs.pc
tools/libs/call/headers.chk
tools/libs/call/xencall.pc
tools/libs/foreignmemory/headers.chk
diff --git a/tools/Rules.mk b/tools/Rules.mk
index 5b8cf748ad..ad6073fcad 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
@@ -132,6 +133,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..06dd449929
--- /dev/null
+++ b/tools/libs/hypfs/Makefile
@@ -0,0 +1,16 @@
+XEN_ROOT = $(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+MAJOR = 1
+MINOR = 0
+LIBNAME := hypfs
+USELIBS := toollog toolcore call
+
+APPEND_LDFLAGS += -lz
+
+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..befdc66e96
--- /dev/null
+++ b/tools/libs/hypfs/core.c
@@ -0,0 +1,536 @@
+/*
+ * 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
+
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <inttypes.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+#include <zlib.h>
+
+#include <xentoollog.h>
+#include <xenhypfs.h>
+#include <xencall.h>
+#include <xentoolcore_internal.h>
+
+#include <xen/xen.h>
+#include <xen/hypfs.h>
+
+#define BUF_SIZE 4096
+
+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;
+
+ /* No need to remember supported version, we only support V1. */
+ if (xencall1(fshdl->xcall, __HYPERVISOR_hypfs_op,
+ XEN_HYPFS_OP_get_version) < 0)
+ 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_inflate(void *in_data, size_t *sz)
+{
+ unsigned char *workbuf;
+ void *content = NULL;
+ unsigned int out_sz;
+ z_stream z = { .opaque = NULL };
+ int ret;
+
+ workbuf = malloc(BUF_SIZE);
+ if (!workbuf)
+ return NULL;
+
+ z.next_in = in_data;
+ z.avail_in = *sz;
+ ret = inflateInit2(&z, MAX_WBITS + 32); /* 32 == gzip */
+
+ for (*sz = 0; ret == Z_OK; *sz += out_sz) {
+ z.next_out = workbuf;
+ z.avail_out = BUF_SIZE;
+ ret = inflate(&z, Z_SYNC_FLUSH);
+ if (ret != Z_OK && ret != Z_STREAM_END)
+ break;
+
+ out_sz = z.next_out - workbuf;
+ content = realloc(content, *sz + out_sz);
+ if (!content) {
+ ret = Z_MEM_ERROR;
+ break;
+ }
+ memcpy(content + *sz, workbuf, out_sz);
+ }
+
+ inflateEnd(&z);
+ if (ret != Z_STREAM_END) {
+ free(content);
+ content = NULL;
+ errno = EIO;
+ }
+ free(workbuf);
+ return content;
+}
+
+static void xenhypfs_set_attrs(struct xen_hypfs_direntry *entry,
+ struct xenhypfs_dirent *dirent)
+{
+ dirent->size = entry->content_len;
+
+ switch(entry->type) {
+ case XEN_HYPFS_TYPE_DIR:
+ dirent->type = xenhypfs_type_dir;
+ break;
+ case XEN_HYPFS_TYPE_BLOB:
+ dirent->type = xenhypfs_type_blob;
+ break;
+ case XEN_HYPFS_TYPE_STRING:
+ dirent->type = xenhypfs_type_string;
+ break;
+ case XEN_HYPFS_TYPE_UINT:
+ dirent->type = xenhypfs_type_uint;
+ break;
+ case XEN_HYPFS_TYPE_INT:
+ dirent->type = xenhypfs_type_int;
+ break;
+ case XEN_HYPFS_TYPE_BOOL:
+ dirent->type = xenhypfs_type_bool;
+ break;
+ default:
+ dirent->type = xenhypfs_type_blob;
+ }
+
+ switch (entry->encoding) {
+ case XEN_HYPFS_ENC_PLAIN:
+ dirent->encoding = xenhypfs_enc_plain;
+ break;
+ case XEN_HYPFS_ENC_GZIP:
+ dirent->encoding = xenhypfs_enc_gzip;
+ break;
+ default:
+ dirent->encoding = xenhypfs_enc_plain;
+ dirent->type = xenhypfs_type_blob;
+ }
+
+ dirent->is_writable = entry->max_write_len;
+}
+
+void *xenhypfs_read_raw(xenhypfs_handle *fshdl, const char *path,
+ struct xenhypfs_dirent **dirent)
+{
+ void *retbuf = NULL, *content = NULL;
+ char *path_buf = NULL;
+ const char *name;
+ struct xen_hypfs_direntry *entry;
+ int ret;
+ int sz, path_sz;
+
+ *dirent = NULL;
+ ret = xenhypfs_get_pathbuf(fshdl, path, &path_buf);
+ if (ret < 0)
+ goto out;
+
+ path_sz = ret;
+
+ for (sz = BUF_SIZE;; sz = sizeof(*entry) + entry->content_len) {
+ if (retbuf)
+ xencall_free_buffer(fshdl->xcall, retbuf);
+
+ retbuf = xencall_alloc_buffer(fshdl->xcall, sz);
+ if (!retbuf) {
+ errno = ENOMEM;
+ goto out;
+ }
+ entry = retbuf;
+
+ ret = xencall5(fshdl->xcall, __HYPERVISOR_hypfs_op, XEN_HYPFS_OP_read,
+ (unsigned long)path_buf, path_sz,
+ (unsigned long)retbuf, sz);
+ if (!ret)
+ break;
+
+ if (ret != ENOBUFS) {
+ errno = -ret;
+ goto out;
+ }
+ }
+
+ content = malloc(entry->content_len);
+ if (!content)
+ goto out;
+ memcpy(content, entry + 1, entry->content_len);
+
+ name = strrchr(path, '/');
+ if (!name)
+ name = path;
+ else {
+ name++;
+ if (!*name)
+ name--;
+ }
+ *dirent = calloc(1, sizeof(struct xenhypfs_dirent) + strlen(name) + 1);
+ if (!*dirent) {
+ free(content);
+ content = NULL;
+ errno = ENOMEM;
+ goto out;
+ }
+ (*dirent)->name = (char *)(*dirent + 1);
+ strcpy((*dirent)->name, name);
+ xenhypfs_set_attrs(entry, *dirent);
+
+ out:
+ ret = errno;
+ xencall_free_buffer(fshdl->xcall, path_buf);
+ xencall_free_buffer(fshdl->xcall, retbuf);
+ errno = ret;
+
+ return content;
+}
+
+char *xenhypfs_read(xenhypfs_handle *fshdl, const char *path)
+{
+ char *buf, *ret_buf = NULL;
+ struct xenhypfs_dirent *dirent;
+ int ret;
+
+ buf = xenhypfs_read_raw(fshdl, path, &dirent);
+ if (!buf)
+ goto out;
+
+ switch (dirent->encoding) {
+ case xenhypfs_enc_plain:
+ break;
+ case xenhypfs_enc_gzip:
+ ret_buf = xenhypfs_inflate(buf, &dirent->size);
+ if (!ret_buf)
+ goto out;
+ free(buf);
+ buf = ret_buf;
+ ret_buf = NULL;
+ break;
+ }
+
+ switch (dirent->type) {
+ case xenhypfs_type_dir:
+ errno = EISDIR;
+ break;
+ case xenhypfs_type_blob:
+ errno = EDOM;
+ break;
+ case xenhypfs_type_string:
+ ret_buf = buf;
+ buf = NULL;
+ break;
+ case xenhypfs_type_uint:
+ case xenhypfs_type_bool:
+ switch (dirent->size) {
+ case 1:
+ ret = asprintf(&ret_buf, "%"PRIu8, *(uint8_t *)buf);
+ break;
+ case 2:
+ ret = asprintf(&ret_buf, "%"PRIu16, *(uint16_t *)buf);
+ break;
+ case 4:
+ ret = asprintf(&ret_buf, "%"PRIu32, *(uint32_t *)buf);
+ break;
+ case 8:
+ ret = asprintf(&ret_buf, "%"PRIu64, *(uint64_t *)buf);
+ break;
+ default:
+ ret = -1;
+ errno = EDOM;
+ }
+ if (ret < 0)
+ ret_buf = NULL;
+ break;
+ case xenhypfs_type_int:
+ switch (dirent->size) {
+ case 1:
+ ret = asprintf(&ret_buf, "%"PRId8, *(int8_t *)buf);
+ break;
+ case 2:
+ ret = asprintf(&ret_buf, "%"PRId16, *(int16_t *)buf);
+ break;
+ case 4:
+ ret = asprintf(&ret_buf, "%"PRId32, *(int32_t *)buf);
+ break;
+ case 8:
+ ret = asprintf(&ret_buf, "%"PRId64, *(int64_t *)buf);
+ break;
+ default:
+ ret = -1;
+ errno = EDOM;
+ }
+ if (ret < 0)
+ ret_buf = NULL;
+ break;
+ }
+
+ out:
+ ret = errno;
+ free(buf);
+ free(dirent);
+ 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, *dirent;
+ unsigned int n = 0, name_sz = 0;
+ struct xen_hypfs_dirlistentry *entry;
+
+ buf = xenhypfs_read_raw(fshdl, path, &dirent);
+ if (!buf)
+ goto out;
+
+ if (dirent->type != xenhypfs_type_dir ||
+ dirent->encoding != xenhypfs_enc_plain) {
+ errno = ENOTDIR;
+ goto out;
+ }
+
+ if (dirent->size) {
+ 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;
+ xenhypfs_set_attrs(&entry->e, ret_buf + n);
+ ret_buf[n].name = names;
+ strcpy(names, entry->name);
+ names += strlen(entry->name) + 1;
+ curr += entry->off_next;
+ }
+
+ out:
+ ret = errno;
+ free(buf);
+ free(dirent);
+ errno = ret;
+
+ return ret_buf;
+}
+
+int xenhypfs_write(xenhypfs_handle *fshdl, const char *path, const char *val)
+{
+ void *buf = NULL;
+ char *path_buf = NULL, *val_end;
+ int ret, saved_errno;
+ int sz, path_sz;
+ struct xen_hypfs_direntry *entry;
+ uint64_t mask;
+
+ ret = xenhypfs_get_pathbuf(fshdl, path, &path_buf);
+ if (ret < 0)
+ goto out;
+
+ path_sz = ret;
+ ret = -1;
+
+ sz = BUF_SIZE;
+ buf = xencall_alloc_buffer(fshdl->xcall, sz);
+ if (!buf) {
+ errno = ENOMEM;
+ goto out;
+ }
+
+ ret = xencall5(fshdl->xcall, __HYPERVISOR_hypfs_op, XEN_HYPFS_OP_read,
+ (unsigned long)path_buf, path_sz,
+ (unsigned long)buf, sizeof(*entry));
+ if (ret && errno != ENOBUFS)
+ goto out;
+ ret = -1;
+ entry = buf;
+ if (!entry->max_write_len) {
+ errno = EACCES;
+ goto out;
+ }
+ if (entry->encoding != XEN_HYPFS_ENC_PLAIN) {
+ /* Writing compressed data currently not supported. */
+ errno = EDOM;
+ goto out;
+ }
+
+ switch (entry->type) {
+ case XEN_HYPFS_TYPE_STRING:
+ if (sz < strlen(val) + 1) {
+ sz = strlen(val) + 1;
+ xencall_free_buffer(fshdl->xcall, buf);
+ buf = xencall_alloc_buffer(fshdl->xcall, sz);
+ if (!buf) {
+ errno = ENOMEM;
+ goto out;
+ }
+ }
+ sz = strlen(val) + 1;
+ strcpy(buf, val);
+ break;
+ case XEN_HYPFS_TYPE_UINT:
+ sz = entry->content_len;
+ errno = 0;
+ *(unsigned long long *)buf = strtoull(val, &val_end, 0);
+ if (errno || !*val || *val_end)
+ goto out;
+ mask = ~0ULL << (8 * sz);
+ if ((*(uint64_t *)buf & mask) && ((*(uint64_t *)buf & mask) != mask)) {
+ errno = ERANGE;
+ goto out;
+ }
+ break;
+ case XEN_HYPFS_TYPE_INT:
+ sz = entry->content_len;
+ errno = 0;
+ *(unsigned long long *)buf = strtoll(val, &val_end, 0);
+ if (errno || !*val || *val_end)
+ goto out;
+ mask = (sz == 8) ? 0 : ~0ULL << (8 * sz);
+ if ((*(uint64_t *)buf & mask) && ((*(uint64_t *)buf & mask) != mask)) {
+ errno = ERANGE;
+ goto out;
+ }
+ break;
+ case XEN_HYPFS_TYPE_BOOL:
+ sz = entry->content_len;
+ *(unsigned long long *)buf = 0;
+ if (!strcmp(val, "1") || !strcmp(val, "on") || !strcmp(val, "yes") ||
+ !strcmp(val, "true") || !strcmp(val, "enable"))
+ *(unsigned long long *)buf = 1;
+ else if (strcmp(val, "0") && strcmp(val, "no") && strcmp(val, "off") &&
+ strcmp(val, "false") && strcmp(val, "disable")) {
+ errno = EDOM;
+ goto out;
+ }
+ break;
+ default:
+ /* No support for other types (yet). */
+ errno = EDOM;
+ goto out;
+ }
+
+ 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..ab157edceb
--- /dev/null
+++ b/tools/libs/hypfs/include/xenhypfs.h
@@ -0,0 +1,90 @@
+/*
+ * 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 <sys/types.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;
+ size_t size;
+ enum {
+ xenhypfs_type_dir,
+ xenhypfs_type_blob,
+ xenhypfs_type_string,
+ xenhypfs_type_uint,
+ xenhypfs_type_int,
+ xenhypfs_type_bool
+ } type;
+ enum {
+ xenhypfs_enc_plain,
+ xenhypfs_enc_gzip
+ } encoding;
+ bool is_writable;
+};
+
+xenhypfs_handle *xenhypfs_open(struct xentoollog_logger *logger,
+ unsigned int open_flags);
+int xenhypfs_close(xenhypfs_handle *fshdl);
+
+/*
+ * Return the raw contents of a Xen hypfs entry and its dirent containing
+ * the size, type and encoding.
+ * Returned buffer and dirent should be freed via free().
+ */
+void *xenhypfs_read_raw(xenhypfs_handle *fshdl, const char *path,
+ struct xenhypfs_dirent **dirent);
+
+/*
+ * Return the contents of a Xen hypfs entry as a string.
+ * Returned buffer should be freed via free().
+ */
+char *xenhypfs_read(xenhypfs_handle *fshdl, const char *path);
+
+/*
+ * Return the contents of a Xen hypfs directory in form of an array of
+ * dirents.
+ * Returned buffer should be freed via free().
+ */
+struct xenhypfs_dirent *xenhypfs_readdir(xenhypfs_handle *fshdl,
+ const char *path,
+ unsigned int *num_entries);
+
+/*
+ * Write a Xen hypfs entry with a value. The value is converted from a string
+ * to the appropriate type.
+ */
+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..47f1edda3e
--- /dev/null
+++ b/tools/libs/hypfs/libxenhypfs.map
@@ -0,0 +1,10 @@
+VERS_1.0 {
+ global:
+ xenhypfs_open;
+ xenhypfs_close;
+ xenhypfs_read_raw;
+ 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..92a262c7a2
--- /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,z
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 06/12] tools: add xenfs tool
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (4 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 05/12] libs: add libxenhypfs Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 07/12] xen: provide version information in hypfs Juergen Gross
` (5 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich
Add the xenfs tool for accessing the hypervisor filesystem.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wl@xen.org>
---
V1:
- rename to xenhypfs
- don't use "--" for subcommands
- add write support
V2:
- escape non-printable characters per default with cat subcommand
(Ian Jackson)
- add -b option to cat subcommand (Ian Jackson)
- add man page
V3:
- adapt to new hypfs interface
V7:
- added missing bool for ls output
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 1 +
docs/man/xenhypfs.1.pod | 61 +++++++++++++
tools/misc/Makefile | 6 ++
tools/misc/xenhypfs.c | 192 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 260 insertions(+)
create mode 100644 docs/man/xenhypfs.1.pod
create mode 100644 tools/misc/xenhypfs.c
diff --git a/.gitignore b/.gitignore
index 41b1d4c8a1..ce3ef23d45 100644
--- a/.gitignore
+++ b/.gitignore
@@ -368,6 +368,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..158b901f42
--- /dev/null
+++ b/tools/misc/xenhypfs.c
@@ -0,0 +1,192 @@
+#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 char *xenhypfs_type(struct xenhypfs_dirent *ent)
+{
+ char *res;
+
+ switch (ent->type) {
+ case xenhypfs_type_dir:
+ res = "<dir> ";
+ break;
+ case xenhypfs_type_blob:
+ res = "<blob> ";
+ break;
+ case xenhypfs_type_string:
+ res = "<string>";
+ break;
+ case xenhypfs_type_uint:
+ res = "<uint> ";
+ break;
+ case xenhypfs_type_int:
+ res = "<int> ";
+ break;
+ case xenhypfs_type_bool:
+ res = "<bool> ";
+ break;
+ default:
+ res = "<\?\?\?> ";
+ break;
+ }
+
+ return res;
+}
+
+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("%s r%c %s\n", xenhypfs_type(ent + i),
+ ent[i].is_writable ? 'w' : '-', 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].type == xenhypfs_type_dir ? "/" : "");
+ if (ent[i].type == xenhypfs_type_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.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 07/12] xen: provide version information in hypfs
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (5 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 06/12] tools: add xenfs tool Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
` (4 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich
Provide version and compile information in /buildinfo/ node of the
Xen hypervisor file system. As this information is accessible by dom0
only no additional security problem arises.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V3:
- new patch
V4:
- add __read_mostly annotations (Jan Beulich)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
docs/misc/hypfs-paths.pandoc | 45 ++++++++++++++++++++++++++++++++++
xen/common/kernel.c | 47 ++++++++++++++++++++++++++++++++++++
2 files changed, 92 insertions(+)
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index 39539fa1b5..d730caf394 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -105,3 +105,48 @@ 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/changeset = STRING
+
+Git commit of the hypervisor.
+
+#### /buildinfo/compileinfo/
+
+A directory containing information about compilation of Xen.
+
+#### /buildinfo/compileinfo/compile_by = STRING
+
+Information who compiled the hypervisor.
+
+#### /buildinfo/compileinfo/compile_date = STRING
+
+Date of the hypervisor compilation.
+
+#### /buildinfo/compileinfo/compile_domain = STRING
+
+Information about the compile domain.
+
+#### /buildinfo/compileinfo/compiler = STRING
+
+The compiler used to build Xen.
+
+#### /buildinfo/version/
+
+A directory containing version information of the hypervisor.
+
+#### /buildinfo/version/extra = STRING
+
+Extra version information.
+
+#### /buildinfo/version/major = INTEGER
+
+The major version of Xen.
+
+#### /buildinfo/version/minor = INTEGER
+
+The minor version of Xen.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 572e3fc07d..db7bd23fcb 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -13,6 +13,7 @@
#include <xen/paging.h>
#include <xen/guest_access.h>
#include <xen/hypercall.h>
+#include <xen/hypfs.h>
#include <xsm/xsm.h>
#include <asm/current.h>
#include <public/version.h>
@@ -373,6 +374,52 @@ void __init do_initcalls(void)
(*call)();
}
+#ifdef CONFIG_HYPFS
+static unsigned int __read_mostly major_version;
+static unsigned int __read_mostly minor_version;
+
+static HYPFS_DIR_INIT(buildinfo, "buildinfo");
+static HYPFS_DIR_INIT(compileinfo, "compileinfo");
+static HYPFS_DIR_INIT(version, "version");
+static HYPFS_UINT_INIT(major, "major", major_version);
+static HYPFS_UINT_INIT(minor, "minor", minor_version);
+static HYPFS_STRING_INIT(changeset, "changeset");
+static HYPFS_STRING_INIT(compiler, "compiler");
+static HYPFS_STRING_INIT(compile_by, "compile_by");
+static HYPFS_STRING_INIT(compile_date, "compile_date");
+static HYPFS_STRING_INIT(compile_domain, "compile_domain");
+static HYPFS_STRING_INIT(extra, "extra");
+
+static int __init buildinfo_init(void)
+{
+ hypfs_add_dir(&hypfs_root, &buildinfo, true);
+
+ hypfs_string_set_reference(&changeset, xen_changeset());
+ hypfs_add_leaf(&buildinfo, &changeset, true);
+
+ hypfs_add_dir(&buildinfo, &compileinfo, true);
+ hypfs_string_set_reference(&compiler, xen_compiler());
+ hypfs_string_set_reference(&compile_by, xen_compile_by());
+ hypfs_string_set_reference(&compile_date, xen_compile_date());
+ hypfs_string_set_reference(&compile_domain, xen_compile_domain());
+ hypfs_add_leaf(&compileinfo, &compiler, true);
+ hypfs_add_leaf(&compileinfo, &compile_by, true);
+ hypfs_add_leaf(&compileinfo, &compile_date, true);
+ hypfs_add_leaf(&compileinfo, &compile_domain, true);
+
+ major_version = xen_major_version();
+ minor_version = xen_minor_version();
+ hypfs_add_dir(&buildinfo, &version, true);
+ hypfs_string_set_reference(&extra, xen_extra_version());
+ hypfs_add_leaf(&version, &extra, true);
+ hypfs_add_leaf(&version, &major, true);
+ hypfs_add_leaf(&version, &minor, true);
+
+ return 0;
+}
+__initcall(buildinfo_init);
+#endif
+
# define DO(fn) long do_##fn
#endif
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (6 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 07/12] xen: provide version information in hypfs Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-14 9:32 ` Jan Beulich
2020-05-08 15:34 ` [PATCH v8 09/12] xen: add runtime parameter access support to hypfs Juergen Gross
` (3 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, 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>
---
V3:
- store data in gzip format
- use binfile mechanism to create data file
- move code to kernel.c
V6:
- add config item for the /buildinfo/config (Jan Beulich)
- make config related variables const in kernel.h (Jan Beulich)
V7:
- update doc (Jan Beulich)
- use "rm -f" in Makefile (Jan Beulich)
V8:
- add dependency top CONFIG_HYPFS
- use macro for definition of leaf (Jan Beulich)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 2 ++
docs/misc/hypfs-paths.pandoc | 4 ++++
xen/common/Kconfig | 11 +++++++++++
xen/common/Makefile | 12 ++++++++++++
xen/common/kernel.c | 11 +++++++++++
xen/include/xen/kernel.h | 3 +++
6 files changed, 43 insertions(+)
diff --git a/.gitignore b/.gitignore
index ce3ef23d45..a58de1fd4a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -298,6 +298,8 @@ xen/arch/*/efi/boot.c
xen/arch/*/efi/compat.c
xen/arch/*/efi/efi.h
xen/arch/*/efi/runtime.c
+xen/common/config_data.S
+xen/common/config.gz
xen/include/headers*.chk
xen/include/asm
xen/include/asm-*/asm-offsets.h
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index d730caf394..9a76bc383b 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -135,6 +135,10 @@ Information about the compile domain.
The compiler used to build Xen.
+#### /buildinfo/config = STRING [CONFIG_HYPFS_CONFIG]
+
+The contents of the `xen/.config` file at the time of the hypervisor build.
+
#### /buildinfo/version/
A directory containing version information of the hypervisor.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 2515e053c8..b36cd74a24 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -127,6 +127,17 @@ config HYPFS
If unsure, say Y.
+config HYPFS_CONFIG
+ bool "Provide hypervisor .config via hypfs entry"
+ default y
+ depends on HYPFS
+ ---help---
+ When enabled the contents of the .config file used to build the
+ hypervisor are provided via the hypfs entry /buildinfo/config.
+
+ Disable this option in case you want to spare some memory or you
+ want to hide the .config contents from dom0.
+
config KEXEC
bool "kexec support"
default y
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 98b7904dcd..d6d7bad2a9 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-$(CONFIG_HYPFS_CONFIG) += config_data.o
obj-$(CONFIG_CORE_PARKING) += core_parking.o
obj-y += cpu.o
obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
@@ -73,3 +74,14 @@ obj-$(CONFIG_UBSAN) += ubsan/
obj-$(CONFIG_NEEDS_LIBELF) += libelf/
obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/
+
+config.gz: ../.config
+ gzip -c $< >$@
+
+config_data.o: config.gz
+
+config_data.S: $(XEN_ROOT)/xen/tools/binfile
+ $(XEN_ROOT)/xen/tools/binfile $@ config.gz xen_config_data
+
+clean::
+ rm -f config_data.S config.gz 2>/dev/null
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index db7bd23fcb..f8f41820d5 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -390,6 +390,10 @@ static HYPFS_STRING_INIT(compile_date, "compile_date");
static HYPFS_STRING_INIT(compile_domain, "compile_domain");
static HYPFS_STRING_INIT(extra, "extra");
+#ifdef CONFIG_HYPFS_CONFIG
+static HYPFS_STRING_INIT(config, "config");
+#endif
+
static int __init buildinfo_init(void)
{
hypfs_add_dir(&hypfs_root, &buildinfo, true);
@@ -415,6 +419,13 @@ static int __init buildinfo_init(void)
hypfs_add_leaf(&version, &major, true);
hypfs_add_leaf(&version, &minor, true);
+#ifdef CONFIG_HYPFS_CONFIG
+ config.e.encoding = XEN_HYPFS_ENC_GZIP;
+ config.e.size = xen_config_data_size;
+ config.content = &xen_config_data;
+ hypfs_add_leaf(&buildinfo, &config, true);
+#endif
+
return 0;
}
__initcall(buildinfo_init);
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64da9f..02e3281f52 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -100,5 +100,8 @@ extern enum system_state {
bool_t is_active_kernel_text(unsigned long addr);
+extern const char xen_config_data;
+extern const unsigned int xen_config_data_size;
+
#endif /* _LINUX_KERNEL_H */
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (7 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-14 10:20 ` Jan Beulich
2020-05-08 15:34 ` [PATCH v8 10/12] tools/libxl: use libxenhypfs for setting xen runtime parameters Juergen Gross
` (2 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Kevin Tian, Stefano Stabellini, Julien Grall,
Jun Nakajima, Wei Liu, Andrew Cooper, Ian Jackson, George Dunlap,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
Add support to read and modify values of hypervisor runtime parameters
via the hypervisor file system.
As runtime parameters can be modified via a sysctl, too, this path has
to take the hypfs rw_lock as writer.
For custom runtime parameters the connection between the parameter
value and the file system is done via an init function which will set
the initial value (if needed) and the leaf properties.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- complete rework
- support custom parameters, too
- support parameter writing
V6:
- rewording in docs/misc/hypfs-paths.pandoc (Jan Beulich)
- use memchr() (Jan Beulich)
- use strlcat() (Jan Beulich)
- rework to use a custom parameter init function instead of a reference
to a content variable, allowing to drop default strings
- style correction (Jan Beulich)
- dropping param_append_str() in favor of a custom function at its only
use site
V7:
- fine tune some parameter initializations (Jan Beulich)
- call custom_runtime_set_var() after updating the value
- modify alignment in Arm linker script to 4 (Jan Beulich)
V8:
- modify alignment in Arm linker script to 8 (Julien Grall)
- fix ept runtime parameter reporting (Jan Beulich)
- rebase to support CONFIG_HYPFS
Signed-off-by: Juergen Gross <jgross@suse.com>
---
docs/misc/hypfs-paths.pandoc | 9 +++
xen/arch/arm/xen.lds.S | 7 ++
xen/arch/x86/hvm/vmx/vmcs.c | 29 +++++++-
xen/arch/x86/pv/domain.c | 24 ++++++-
xen/arch/x86/xen.lds.S | 7 ++
xen/common/grant_table.c | 62 ++++++++++++++----
xen/common/hypfs.c | 41 ++++++++++++
xen/common/kernel.c | 27 +++++++-
xen/drivers/char/console.c | 77 ++++++++++++++++++++--
xen/include/xen/hypfs.h | 7 ++
xen/include/xen/param.h | 124 ++++++++++++++++++++++++++++++++++-
11 files changed, 390 insertions(+), 24 deletions(-)
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index 9a76bc383b..a111c6f25c 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -154,3 +154,12 @@ The major version of Xen.
#### /buildinfo/version/minor = INTEGER
The minor version of Xen.
+
+#### /params/
+
+A directory of runtime parameters.
+
+#### /params/*
+
+The individual parameters. The description of the different parameters can be
+found in `docs/misc/xen-command-line.pandoc`.
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index a497f6a48d..0a6efe96cf 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -89,6 +89,13 @@ SECTIONS
__start_schedulers_array = .;
*(.data.schedulers)
__end_schedulers_array = .;
+
+#ifdef CONFIG_HYPFS
+ . = ALIGN(8);
+ __paramhypfs_start = .;
+ *(.data.paramhypfs)
+ __paramhypfs_end = .;
+#endif
*(.data.rel)
*(.data.rel.*)
CONSTRUCTORS
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 221af9737a..1746385d13 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -71,6 +71,27 @@ static bool __read_mostly opt_ept_pml = true;
static s8 __read_mostly opt_ept_ad = -1;
int8_t __read_mostly opt_ept_exec_sp = -1;
+#ifdef CONFIG_HYPFS
+static char opt_ept_setting[10];
+
+static void update_ept_param(void)
+{
+ if ( opt_ept_exec_sp >= 0 )
+ snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
+ opt_ept_exec_sp);
+}
+
+static void __init init_ept_param(struct param_hypfs *par)
+{
+ update_ept_param();
+ custom_runtime_set_var(par, opt_ept_setting);
+}
+#else
+static void update_ept_param(void)
+{
+}
+#endif
+
static int __init parse_ept_param(const char *s)
{
const char *ss;
@@ -97,6 +118,9 @@ static int __init parse_ept_param(const char *s)
}
custom_param("ept", parse_ept_param);
+static int parse_ept_param_runtime(const char *s);
+custom_runtime_only_param("ept", parse_ept_param_runtime, init_ept_param);
+
static int parse_ept_param_runtime(const char *s)
{
struct domain *d;
@@ -115,6 +139,10 @@ static int parse_ept_param_runtime(const char *s)
opt_ept_exec_sp = val;
+ update_ept_param();
+ custom_runtime_set_var(param_2_parfs(parse_ept_param_runtime),
+ opt_ept_setting);
+
rcu_read_lock(&domlist_read_lock);
for_each_domain ( d )
{
@@ -144,7 +172,6 @@ static int parse_ept_param_runtime(const char *s)
return 0;
}
-custom_runtime_only_param("ept", parse_ept_param_runtime);
/* Dynamic (run-time adjusted) execution control flags. */
u32 vmx_pin_based_exec_control __read_mostly;
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 0a4a5bd001..6bfce9a76a 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -52,9 +52,27 @@ static __read_mostly enum {
PCID_OFF,
PCID_ALL,
PCID_XPTI,
- PCID_NOXPTI
+ PCID_NOXPTI,
+ PCID_END
} opt_pcid = PCID_XPTI;
+#ifdef CONFIG_HYPFS
+static const char opt_pcid_2_string[PCID_END][7] = {
+ [PCID_OFF] = "off",
+ [PCID_ALL] = "on",
+ [PCID_XPTI] = "xpti",
+ [PCID_NOXPTI] = "noxpti"
+};
+
+static void __init opt_pcid_init(struct param_hypfs *par)
+{
+ custom_runtime_set_var(par, opt_pcid_2_string[opt_pcid]);
+}
+#endif
+
+static int parse_pcid(const char *s);
+custom_runtime_param("pcid", parse_pcid, opt_pcid_init);
+
static int parse_pcid(const char *s)
{
int rc = 0;
@@ -87,9 +105,11 @@ static int parse_pcid(const char *s)
break;
}
+ custom_runtime_set_var(param_2_parfs(parse_pcid),
+ opt_pcid_2_string[opt_pcid]);
+
return rc;
}
-custom_runtime_param("pcid", parse_pcid);
static void noreturn continue_nonidle_domain(struct vcpu *v)
{
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 0e3a733cab..3ed020e26b 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -279,6 +279,13 @@ SECTIONS
__start_schedulers_array = .;
*(.data.schedulers)
__end_schedulers_array = .;
+
+#ifdef CONFIG_HYPFS
+ . = ALIGN(8);
+ __paramhypfs_start = .;
+ *(.data.paramhypfs)
+ __paramhypfs_end = .;
+#endif
} :text
DECL_SECTION(.data) {
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5ef7ff940d..a9eeee8ff9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -85,8 +85,43 @@ struct grant_table {
struct grant_table_arch arch;
};
-static int parse_gnttab_limit(const char *param, const char *arg,
- unsigned int *valp)
+unsigned int __read_mostly opt_max_grant_frames = 64;
+static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
+
+#ifdef CONFIG_HYPFS
+#define GRANT_CUSTOM_VAL_SZ 12
+static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
+static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
+
+static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
+ char *parval)
+{
+ snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
+ custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
+}
+
+static void __init gnttab_max_frames_init(struct param_hypfs *par)
+{
+ update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
+}
+
+static void __init max_maptrack_frames_init(struct param_hypfs *par)
+{
+ update_gnttab_par(par, opt_max_maptrack_frames,
+ opt_max_maptrack_frames_val);
+}
+#else
+#define opt_max_grant_frames_val NULL
+#define opt_max_maptrack_frames_val NULL
+
+static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
+ char *parval)
+{
+}
+#endif
+
+static int parse_gnttab_limit(struct param_hypfs *par, const char *arg,
+ unsigned int *valp, char *parval)
{
const char *e;
unsigned long val;
@@ -99,28 +134,33 @@ static int parse_gnttab_limit(const char *param, const char *arg,
return -ERANGE;
*valp = val;
+ update_gnttab_par(par, val, parval);
return 0;
}
-unsigned int __read_mostly opt_max_grant_frames = 64;
+static int parse_gnttab_max_frames(const char *arg);
+custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames,
+ gnttab_max_frames_init);
static int parse_gnttab_max_frames(const char *arg)
{
- return parse_gnttab_limit("gnttab_max_frames", arg,
- &opt_max_grant_frames);
+ return parse_gnttab_limit(param_2_parfs(parse_gnttab_max_frames),
+ arg, &opt_max_grant_frames,
+ opt_max_grant_frames_val);
}
-custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames);
-static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
+static int parse_gnttab_max_maptrack_frames(const char *arg);
+custom_runtime_param("gnttab_max_maptrack_frames",
+ parse_gnttab_max_maptrack_frames,
+ max_maptrack_frames_init);
static int parse_gnttab_max_maptrack_frames(const char *arg)
{
- return parse_gnttab_limit("gnttab_max_maptrack_frames", arg,
- &opt_max_maptrack_frames);
+ return parse_gnttab_limit(param_2_parfs(parse_gnttab_max_maptrack_frames),
+ arg, &opt_max_maptrack_frames,
+ opt_max_maptrack_frames_val);
}
-custom_runtime_param("gnttab_max_maptrack_frames",
- parse_gnttab_max_maptrack_frames);
#ifndef GNTTAB_MAX_VERSION
#define GNTTAB_MAX_VERSION 2
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index a873d507d1..e4de0a9eef 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -10,6 +10,7 @@
#include <xen/hypercall.h>
#include <xen/hypfs.h>
#include <xen/lib.h>
+#include <xen/param.h>
#include <xen/rwlock.h>
#include <public/hypfs.h>
@@ -281,6 +282,36 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
return 0;
}
+int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
+{
+ struct param_hypfs *p;
+ char *buf;
+ int ret;
+
+ buf = xzalloc_array(char, ulen);
+ if ( !buf )
+ return -ENOMEM;
+
+ ret = -EFAULT;
+ if ( copy_from_guest(buf, uaddr, ulen) )
+ goto out;
+
+ ret = -EDOM;
+ if ( memchr(buf, 0, ulen) != (buf + ulen - 1) )
+ goto out;
+
+ p = container_of(leaf, struct param_hypfs, hypfs);
+ ret = p->param->par.func(buf);
+
+ if ( !ret )
+ leaf->e.size = ulen;
+
+ out:
+ xfree(buf);
+ return ret;
+}
+
static int hypfs_write(struct hypfs_entry *entry,
XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
{
@@ -352,3 +383,13 @@ long do_hypfs_op(unsigned int cmd,
return ret;
}
+
+void hypfs_write_lock(void)
+{
+ write_lock(&hypfs_lock);
+}
+
+void hypfs_write_unlock(void)
+{
+ write_unlock(&hypfs_lock);
+}
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f8f41820d5..3b8b0d8ca5 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -198,7 +198,13 @@ static void __init _cmdline_parse(const char *cmdline)
int runtime_parse(const char *line)
{
- return parse_params(line, __param_start, __param_end);
+ int ret;
+
+ hypfs_write_lock();
+ ret = parse_params(line, __param_start, __param_end);
+ hypfs_write_unlock();
+
+ return ret;
}
/**
@@ -429,6 +435,25 @@ static int __init buildinfo_init(void)
return 0;
}
__initcall(buildinfo_init);
+
+static HYPFS_DIR_INIT(params, "params");
+
+static int __init param_init(void)
+{
+ struct param_hypfs *param;
+
+ hypfs_add_dir(&hypfs_root, ¶ms, true);
+
+ for ( param = __paramhypfs_start; param < __paramhypfs_end; param++ )
+ {
+ if ( param->init_leaf )
+ param->init_leaf(param);
+ hypfs_add_leaf(¶ms, ¶m->hypfs, true);
+ }
+
+ return 0;
+}
+__initcall(param_init);
#endif
# define DO(fn) long do_##fn
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 913ae1b66a..692b7278f6 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -75,12 +75,35 @@ enum con_timestamp_mode
TSM_DATE_MS, /* [YYYY-MM-DD HH:MM:SS.mmm] */
TSM_BOOT, /* [SSSSSS.uuuuuu] */
TSM_RAW, /* [XXXXXXXXXXXXXXXX] */
+ TSM_END
};
static enum con_timestamp_mode __read_mostly opt_con_timestamp_mode = TSM_NONE;
+#ifdef CONFIG_HYPFS
+static const char con_timestamp_mode_2_string[TSM_END][7] = {
+ [TSM_NONE] = "none",
+ [TSM_DATE] = "date",
+ [TSM_DATE_MS] = "datems",
+ [TSM_BOOT] = "boot",
+ [TSM_RAW] = "raw"
+};
+
+static void con_timestamp_mode_upd(struct param_hypfs *par)
+{
+ const char *val = con_timestamp_mode_2_string[opt_con_timestamp_mode];
+
+ custom_runtime_set_var_sz(par, val, 7);
+}
+#else
+static void con_timestamp_mode_upd(struct param_hypfs *par)
+{
+}
+#endif
+
static int parse_console_timestamps(const char *s);
-custom_runtime_param("console_timestamps", parse_console_timestamps);
+custom_runtime_param("console_timestamps", parse_console_timestamps,
+ con_timestamp_mode_upd);
/* conring_size: allows a large console ring than default (16kB). */
static uint32_t __initdata opt_conring_size;
@@ -143,6 +166,39 @@ static int __read_mostly xenlog_guest_lower_thresh =
static int parse_loglvl(const char *s);
static int parse_guest_loglvl(const char *s);
+#ifdef CONFIG_HYPFS
+#define LOGLVL_VAL_SZ 16
+static char xenlog_val[LOGLVL_VAL_SZ];
+static char xenlog_guest_val[LOGLVL_VAL_SZ];
+
+static char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
+
+static void xenlog_update_val(int lower, int upper, char *val)
+{
+ snprintf(val, LOGLVL_VAL_SZ, "%s/%s", lvl2opt[lower], lvl2opt[upper]);
+}
+
+static void __init xenlog_init(struct param_hypfs *par)
+{
+ xenlog_update_val(xenlog_lower_thresh, xenlog_upper_thresh, xenlog_val);
+ custom_runtime_set_var(par, xenlog_val);
+}
+
+static void __init xenlog_guest_init(struct param_hypfs *par)
+{
+ xenlog_update_val(xenlog_guest_lower_thresh, xenlog_guest_upper_thresh,
+ xenlog_guest_val);
+ custom_runtime_set_var(par, xenlog_guest_val);
+}
+#else
+#define xenlog_val NULL
+#define xenlog_guest_val NULL
+
+static void xenlog_update_val(int lower, int upper, char *val)
+{
+}
+#endif
+
/*
* <lvl> := none|error|warning|info|debug|all
* loglvl=<lvl_print_always>[/<lvl_print_ratelimit>]
@@ -151,8 +207,8 @@ static int parse_guest_loglvl(const char *s);
* Similar definitions for guest_loglvl, but applies to guest tracing.
* Defaults: loglvl=warning ; guest_loglvl=none/warning
*/
-custom_runtime_param("loglvl", parse_loglvl);
-custom_runtime_param("guest_loglvl", parse_guest_loglvl);
+custom_runtime_param("loglvl", parse_loglvl, xenlog_init);
+custom_runtime_param("guest_loglvl", parse_guest_loglvl, xenlog_guest_init);
static atomic_t print_everything = ATOMIC_INIT(0);
@@ -173,7 +229,7 @@ static int __parse_loglvl(const char *s, const char **ps)
return 2; /* sane fallback */
}
-static int _parse_loglvl(const char *s, int *lower, int *upper)
+static int _parse_loglvl(const char *s, int *lower, int *upper, char *val)
{
*lower = *upper = __parse_loglvl(s, &s);
if ( *s == '/' )
@@ -181,18 +237,21 @@ static int _parse_loglvl(const char *s, int *lower, int *upper)
if ( *upper < *lower )
*upper = *lower;
+ xenlog_update_val(*lower, *upper, val);
+
return *s ? -EINVAL : 0;
}
static int parse_loglvl(const char *s)
{
- return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh);
+ return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
+ xenlog_val);
}
static int parse_guest_loglvl(const char *s)
{
return _parse_loglvl(s, &xenlog_guest_lower_thresh,
- &xenlog_guest_upper_thresh);
+ &xenlog_guest_upper_thresh, xenlog_guest_val);
}
static char *loglvl_str(int lvl)
@@ -727,13 +786,17 @@ static int printk_prefix_check(char *p, char **pp)
static int parse_console_timestamps(const char *s)
{
+ struct param_hypfs *par = param_2_parfs(parse_console_timestamps);
+
switch ( parse_bool(s, NULL) )
{
case 0:
opt_con_timestamp_mode = TSM_NONE;
+ con_timestamp_mode_upd(par);
return 0;
case 1:
opt_con_timestamp_mode = TSM_DATE;
+ con_timestamp_mode_upd(par);
return 0;
}
if ( *s == '\0' || /* Compat for old booleanparam() */
@@ -750,6 +813,8 @@ static int parse_console_timestamps(const char *s)
else
return -EINVAL;
+ con_timestamp_mode_upd(par);
+
return 0;
}
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index 279bcf3b4c..095a0c9bee 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -117,6 +117,13 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
+int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
+ XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
+void hypfs_write_lock(void);
+void hypfs_write_unlock(void);
+#else
+static inline void hypfs_write_lock(void) {}
+static inline void hypfs_write_unlock(void) {}
#endif
#endif /* __XEN_HYPFS_H__ */
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index a1dc3ba8f0..4893de9bd5 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -1,6 +1,7 @@
#ifndef _XEN_PARAM_H
#define _XEN_PARAM_H
+#include <xen/hypfs.h>
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/stdbool.h>
@@ -80,7 +81,120 @@ extern const struct kernel_param __param_start[], __param_end[];
#define __rtparam __param(__dataparam)
-#define custom_runtime_only_param(_name, _var) \
+#ifdef CONFIG_HYPFS
+
+struct param_hypfs {
+ const struct kernel_param *param;
+ struct hypfs_entry_leaf hypfs;
+ void (*init_leaf)(struct param_hypfs *par);
+};
+
+extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
+
+#define __paramhypfs __used_section(".data.paramhypfs")
+
+#define __paramfs static __paramhypfs \
+ __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs
+
+#define custom_runtime_set_var_sz(parfs, var, sz) \
+ { \
+ (parfs)->hypfs.content = var; \
+ (parfs)->hypfs.e.max_size = sz; \
+ (parfs)->hypfs.e.size = strlen(var) + 1; \
+ }
+#define custom_runtime_set_var(parfs, var) \
+ custom_runtime_set_var_sz(parfs, var, sizeof(var))
+
+#define param_2_parfs(par) &__parfs_##par
+
+/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). */
+#define custom_runtime_only_param(_name, _var, initfunc) \
+ __rtparam __rtpar_##_var = \
+ { .name = _name, \
+ .type = OPT_CUSTOM, \
+ .par.func = _var }; \
+ __paramfs __parfs_##_var = \
+ { .param = &__rtpar_##_var, \
+ .init_leaf = initfunc, \
+ .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
+ .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .hypfs.e.name = _name, \
+ .hypfs.e.read = hypfs_read_leaf, \
+ .hypfs.e.write = hypfs_write_custom }
+#define boolean_runtime_only_param(_name, _var) \
+ __rtparam __rtpar_##_var = \
+ { .name = _name, \
+ .type = OPT_BOOL, \
+ .len = sizeof(_var) + \
+ BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
+ .par.var = &_var }; \
+ __paramfs __parfs_##_var = \
+ { .param = &__rtpar_##_var, \
+ .hypfs.e.type = XEN_HYPFS_TYPE_BOOL, \
+ .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .hypfs.e.name = _name, \
+ .hypfs.e.size = sizeof(_var), \
+ .hypfs.e.max_size = sizeof(_var), \
+ .hypfs.e.read = hypfs_read_leaf, \
+ .hypfs.e.write = hypfs_write_bool, \
+ .hypfs.content = &_var }
+#define integer_runtime_only_param(_name, _var) \
+ __rtparam __rtpar_##_var = \
+ { .name = _name, \
+ .type = OPT_UINT, \
+ .len = sizeof(_var), \
+ .par.var = &_var }; \
+ __paramfs __parfs_##_var = \
+ { .param = &__rtpar_##_var, \
+ .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
+ .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .hypfs.e.name = _name, \
+ .hypfs.e.size = sizeof(_var), \
+ .hypfs.e.max_size = sizeof(_var), \
+ .hypfs.e.read = hypfs_read_leaf, \
+ .hypfs.e.write = hypfs_write_leaf, \
+ .hypfs.content = &_var }
+#define size_runtime_only_param(_name, _var) \
+ __rtparam __rtpar_##_var = \
+ { .name = _name, \
+ .type = OPT_SIZE, \
+ .len = sizeof(_var), \
+ .par.var = &_var }; \
+ __paramfs __parfs_##_var = \
+ { .param = &__rtpar_##_var, \
+ .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
+ .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .hypfs.e.name = _name, \
+ .hypfs.e.size = sizeof(_var), \
+ .hypfs.e.max_size = sizeof(_var), \
+ .hypfs.e.read = hypfs_read_leaf, \
+ .hypfs.e.write = hypfs_write_leaf, \
+ .hypfs.content = &_var }
+#define string_runtime_only_param(_name, _var) \
+ __rtparam __rtpar_##_var = \
+ { .name = _name, \
+ .type = OPT_STR, \
+ .len = sizeof(_var), \
+ .par.var = &_var }; \
+ __paramfs __parfs_##_var = \
+ { .param = &__rtpar_##_var, \
+ .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
+ .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
+ .hypfs.e.name = _name, \
+ .hypfs.e.size = sizeof(_var), \
+ .hypfs.e.max_size = sizeof(_var), \
+ .hypfs.e.read = hypfs_read_leaf, \
+ .hypfs.e.write = hypfs_write_leaf, \
+ .hypfs.content = &_var }
+
+#else
+
+struct param_hypfs {
+};
+
+#define param_2_parfs(par) NULL
+
+#define custom_runtime_only_param(_name, _var, initfunc) \
__rtparam __rtpar_##_var = \
{ .name = _name, \
.type = OPT_CUSTOM, \
@@ -111,9 +225,13 @@ extern const struct kernel_param __param_start[], __param_end[];
.len = sizeof(_var), \
.par.var = &_var }
-#define custom_runtime_param(_name, _var) \
+#define custom_runtime_set_var(parfs, var)
+
+#endif
+
+#define custom_runtime_param(_name, _var, initfunc) \
custom_param(_name, _var); \
- custom_runtime_only_param(_name, _var)
+ custom_runtime_only_param(_name, _var, initfunc)
#define boolean_runtime_param(_name, _var) \
boolean_param(_name, _var); \
boolean_runtime_only_param(_name, _var)
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 10/12] tools/libxl: use libxenhypfs for setting xen runtime parameters
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (8 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 09/12] xen: add runtime parameter access support to hypfs Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 11/12] tools/libxc: remove xc_set_parameters() Juergen Gross
2020-05-08 15:34 ` [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support Juergen Gross
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Anthony PERARD, Ian Jackson, Wei Liu
Instead of xc_set_parameters() use xenhypfs_write() for setting
parameters of the hypervisor.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V6:
- new patch
---
tools/Rules.mk | 2 +-
tools/libxl/Makefile | 3 +-
tools/libxl/libxl.c | 53 ++++++++++++++++++++++++++++++++----
tools/libxl/libxl_internal.h | 1 +
tools/libxl/xenlight.pc.in | 2 +-
tools/xl/xl_misc.c | 1 -
6 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/tools/Rules.mk b/tools/Rules.mk
index ad6073fcad..883a193f9e 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -178,7 +178,7 @@ CFLAGS += -O2 -fomit-frame-pointer
endif
CFLAGS_libxenlight = -I$(XEN_XENLIGHT) $(CFLAGS_libxenctrl) $(CFLAGS_xeninclude)
-SHDEPS_libxenlight = $(SHLIB_libxenctrl) $(SHLIB_libxenstore)
+SHDEPS_libxenlight = $(SHLIB_libxenctrl) $(SHLIB_libxenstore) $(SHLIB_libxenhypfs)
LDLIBS_libxenlight = $(SHDEPS_libxenlight) $(XEN_XENLIGHT)/libxenlight$(libextension)
SHLIB_libxenlight = $(SHDEPS_libxenlight) -Wl,-rpath-link=$(XEN_XENLIGHT)
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 69fcf21577..a89ebab0b4 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
endif
LIBXL_LIBS =
-LIBXL_LIBS = $(LDLIBS_libxentoollog) $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
+LIBXL_LIBS = $(LDLIBS_libxentoollog) $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenhypfs) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
ifeq ($(CONFIG_LIBNL),y)
LIBXL_LIBS += $(LIBNL3_LIBS)
endif
@@ -33,6 +33,7 @@ CFLAGS_LIBXL += $(CFLAGS_libxentoolcore)
CFLAGS_LIBXL += $(CFLAGS_libxenevtchn)
CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
CFLAGS_LIBXL += $(CFLAGS_libxenguest)
+CFLAGS_LIBXL += $(CFLAGS_libxenhypfs)
CFLAGS_LIBXL += $(CFLAGS_libxenstore)
ifeq ($(CONFIG_LIBNL),y)
CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f60fd3e4fd..621acc88f3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -663,15 +663,56 @@ int libxl_set_parameters(libxl_ctx *ctx, char *params)
{
int ret;
GC_INIT(ctx);
+ char *par, *val, *end, *path;
+ xenhypfs_handle *hypfs;
- ret = xc_set_parameters(ctx->xch, params);
- if (ret < 0) {
- LOGEV(ERROR, ret, "setting parameters");
- GC_FREE;
- return ERROR_FAIL;
+ hypfs = xenhypfs_open(ctx->lg, 0);
+ if (!hypfs) {
+ LOGE(ERROR, "opening Xen hypfs");
+ ret = ERROR_FAIL;
+ goto out;
}
+
+ while (isblank(*params))
+ params++;
+
+ for (par = params; *par; par = end) {
+ end = strchr(par, ' ');
+ if (!end)
+ end = par + strlen(par);
+
+ val = strchr(par, '=');
+ if (val > end)
+ val = NULL;
+ if (!val && !strncmp(par, "no", 2)) {
+ path = libxl__sprintf(gc, "/params/%s", par + 2);
+ path[end - par - 2 + 8] = 0;
+ val = "no";
+ par += 2;
+ } else {
+ path = libxl__sprintf(gc, "/params/%s", par);
+ path[val - par + 8] = 0;
+ val = libxl__strndup(gc, val + 1, end - val - 1);
+ }
+
+ LOG(DEBUG, "setting node \"%s\" to value \"%s\"", path, val);
+ ret = xenhypfs_write(hypfs, path, val);
+ if (ret < 0) {
+ LOGE(ERROR, "setting parameters");
+ ret = ERROR_FAIL;
+ goto out;
+ }
+
+ while (isblank(*end))
+ end++;
+ }
+
+ ret = 0;
+
+out:
+ xenhypfs_close(hypfs);
GC_FREE;
- return 0;
+ return ret;
}
static int fd_set_flags(libxl_ctx *ctx, int fd,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e5effd2ad1..b85b771659 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -56,6 +56,7 @@
#define XC_WANT_COMPAT_MAP_FOREIGN_API
#include <xenctrl.h>
#include <xenguest.h>
+#include <xenhypfs.h>
#include <xc_dom.h>
#include <xen-tools/libs.h>
diff --git a/tools/libxl/xenlight.pc.in b/tools/libxl/xenlight.pc.in
index c0f769fd20..6b351ba096 100644
--- a/tools/libxl/xenlight.pc.in
+++ b/tools/libxl/xenlight.pc.in
@@ -9,4 +9,4 @@ Description: The Xenlight library for Xen hypervisor
Version: @@version@@
Cflags: -I${includedir}
Libs: @@libsflag@@${libdir} -lxenlight
-Requires.private: xentoollog,xenevtchn,xencontrol,xenguest,xenstore
+Requires.private: xentoollog,xenevtchn,xencontrol,xenguest,xenstore,xenhypfs
diff --git a/tools/xl/xl_misc.c b/tools/xl/xl_misc.c
index 20ed605f4f..08f0fb6dc9 100644
--- a/tools/xl/xl_misc.c
+++ b/tools/xl/xl_misc.c
@@ -168,7 +168,6 @@ int main_set_parameters(int argc, char **argv)
if (libxl_set_parameters(ctx, params)) {
fprintf(stderr, "cannot set parameters: %s\n", params);
- fprintf(stderr, "Use \"xl dmesg\" to look for possible reason.\n");
return EXIT_FAILURE;
}
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 11/12] tools/libxc: remove xc_set_parameters()
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (9 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 10/12] tools/libxl: use libxenhypfs for setting xen runtime parameters Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-08 15:34 ` [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support Juergen Gross
11 siblings, 0 replies; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Ian Jackson, Wei Liu
There is no user of xc_set_parameters() left, so remove it.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V6:
- new patch
---
tools/libxc/include/xenctrl.h | 1 -
tools/libxc/xc_misc.c | 21 ---------------------
2 files changed, 22 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 0a6ff93229..feb20319b6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1226,7 +1226,6 @@ int xc_readconsolering(xc_interface *xch,
int clear, int incremental, uint32_t *pindex);
int xc_send_debug_keys(xc_interface *xch, const char *keys);
-int xc_set_parameters(xc_interface *xch, const char *params);
typedef struct xen_sysctl_physinfo xc_physinfo_t;
typedef struct xen_sysctl_cputopo xc_cputopo_t;
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index fe477bf344..3820394413 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -187,27 +187,6 @@ int xc_send_debug_keys(xc_interface *xch, const char *keys)
return ret;
}
-int xc_set_parameters(xc_interface *xch, const char *params)
-{
- int ret, len = strlen(params);
- DECLARE_SYSCTL;
- DECLARE_HYPERCALL_BOUNCE_IN(params, len);
-
- if ( xc_hypercall_bounce_pre(xch, params) )
- return -1;
-
- sysctl.cmd = XEN_SYSCTL_set_parameter;
- set_xen_guest_handle(sysctl.u.set_parameter.params, params);
- sysctl.u.set_parameter.size = len;
- memset(sysctl.u.set_parameter.pad, 0, sizeof(sysctl.u.set_parameter.pad));
-
- ret = do_sysctl(xch, &sysctl);
-
- xc_hypercall_bounce_post(xch, params);
-
- return ret;
-}
-
int xc_physinfo(xc_interface *xch,
xc_physinfo_t *put_info)
{
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
` (10 preceding siblings ...)
2020-05-08 15:34 ` [PATCH v8 11/12] tools/libxc: remove xc_set_parameters() Juergen Gross
@ 2020-05-08 15:34 ` Juergen Gross
2020-05-14 13:03 ` Jan Beulich
11 siblings, 1 reply; 31+ messages in thread
From: Juergen Gross @ 2020-05-08 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Kevin Tian, Stefano Stabellini, Julien Grall,
Jun Nakajima, Wei Liu, Andrew Cooper, Ian Jackson, George Dunlap,
Jan Beulich, Daniel De Graaf, Volodymyr Babchuk,
Roger Pau Monné
The functionality of XEN_SYSCTL_set_parameter is available via hypfs
now, so it can be removed.
This allows to remove the kernel_param structure for runtime parameters
by putting the now only used structure element into the hypfs node
structure of the runtime parameters.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V6:
- new patch
V7:
- only comment out definition of XEN_SYSCTL_set_parameter (Jan Beulich)
V8:
- rebase to use CONFIG_HYPFS
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/flask/policy/modules/dom0.te | 2 +-
xen/arch/arm/xen.lds.S | 5 --
xen/arch/x86/hvm/vmx/vmcs.c | 38 ++++++-------
xen/arch/x86/xen.lds.S | 5 --
xen/common/hypfs.c | 12 +---
xen/common/kernel.c | 11 ----
xen/common/sysctl.c | 36 ------------
xen/include/public/sysctl.h | 19 +------
xen/include/xen/hypfs.h | 5 --
xen/include/xen/lib.h | 1 -
xen/include/xen/param.h | 87 +++++------------------------
xen/xsm/flask/hooks.c | 3 -
xen/xsm/flask/policy/access_vectors | 2 -
13 files changed, 34 insertions(+), 192 deletions(-)
diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 20925e38a2..0a63ce15b6 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -16,7 +16,7 @@ allow dom0_t xen_t:xen {
allow dom0_t xen_t:xen2 {
resource_op psr_cmt_op psr_alloc pmu_ctrl get_symbol
get_cpu_levelling_caps get_cpu_featureset livepatch_op
- coverage_op set_parameter
+ coverage_op
};
# Allow dom0 to use all XENVER_ subops that have checks.
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 0a6efe96cf..a795497ac8 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -54,11 +54,6 @@ SECTIONS
*(.data.rel.ro)
*(.data.rel.ro.*)
- . = ALIGN(POINTER_ALIGN);
- __param_start = .;
- *(.data.param)
- __param_end = .;
-
__proc_info_start = .;
*(.proc.info)
__proc_info_end = .;
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 1746385d13..ca94c2bedc 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -71,27 +71,6 @@ static bool __read_mostly opt_ept_pml = true;
static s8 __read_mostly opt_ept_ad = -1;
int8_t __read_mostly opt_ept_exec_sp = -1;
-#ifdef CONFIG_HYPFS
-static char opt_ept_setting[10];
-
-static void update_ept_param(void)
-{
- if ( opt_ept_exec_sp >= 0 )
- snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
- opt_ept_exec_sp);
-}
-
-static void __init init_ept_param(struct param_hypfs *par)
-{
- update_ept_param();
- custom_runtime_set_var(par, opt_ept_setting);
-}
-#else
-static void update_ept_param(void)
-{
-}
-#endif
-
static int __init parse_ept_param(const char *s)
{
const char *ss;
@@ -118,6 +97,22 @@ static int __init parse_ept_param(const char *s)
}
custom_param("ept", parse_ept_param);
+#ifdef CONFIG_HYPFS
+static char opt_ept_setting[10];
+
+static void update_ept_param(void)
+{
+ if ( opt_ept_exec_sp >= 0 )
+ snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
+ opt_ept_exec_sp);
+}
+
+static void __init init_ept_param(struct param_hypfs *par)
+{
+ update_ept_param();
+ custom_runtime_set_var(par, opt_ept_setting);
+}
+
static int parse_ept_param_runtime(const char *s);
custom_runtime_only_param("ept", parse_ept_param_runtime, init_ept_param);
@@ -172,6 +167,7 @@ static int parse_ept_param_runtime(const char *s)
return 0;
}
+#endif
/* Dynamic (run-time adjusted) execution control flags. */
u32 vmx_pin_based_exec_control __read_mostly;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 3ed020e26b..0273f79152 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -128,11 +128,6 @@ SECTIONS
*(.ex_table.pre)
__stop___pre_ex_table = .;
- . = ALIGN(POINTER_ALIGN);
- __param_start = .;
- *(.data.param)
- __param_end = .;
-
#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
. = ALIGN(POINTER_ALIGN);
__start_vpci_array = .;
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index e4de0a9eef..434b6c1308 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -302,7 +302,7 @@ int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
goto out;
p = container_of(leaf, struct param_hypfs, hypfs);
- ret = p->param->par.func(buf);
+ ret = p->func(buf);
if ( !ret )
leaf->e.size = ulen;
@@ -383,13 +383,3 @@ long do_hypfs_op(unsigned int cmd,
return ret;
}
-
-void hypfs_write_lock(void)
-{
- write_lock(&hypfs_lock);
-}
-
-void hypfs_write_unlock(void)
-{
- write_unlock(&hypfs_lock);
-}
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 3b8b0d8ca5..bb37c7c9bb 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -196,17 +196,6 @@ static void __init _cmdline_parse(const char *cmdline)
parse_params(cmdline, __setup_start, __setup_end);
}
-int runtime_parse(const char *line)
-{
- int ret;
-
- hypfs_write_lock();
- ret = parse_params(line, __param_start, __param_end);
- hypfs_write_unlock();
-
- return ret;
-}
-
/**
* cmdline_parse -- parses the xen command line.
* If CONFIG_CMDLINE is set, it would be parsed prior to @cmdline.
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 1c6a817476..ec916424e5 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -471,42 +471,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
copyback = 1;
break;
- case XEN_SYSCTL_set_parameter:
- {
-#define XEN_SET_PARAMETER_MAX_SIZE 1023
- char *params;
-
- if ( op->u.set_parameter.pad[0] || op->u.set_parameter.pad[1] ||
- op->u.set_parameter.pad[2] )
- {
- ret = -EINVAL;
- break;
- }
- if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
- {
- ret = -E2BIG;
- break;
- }
- params = xmalloc_bytes(op->u.set_parameter.size + 1);
- if ( !params )
- {
- ret = -ENOMEM;
- break;
- }
- if ( copy_from_guest(params, op->u.set_parameter.params,
- op->u.set_parameter.size) )
- ret = -EFAULT;
- else
- {
- params[op->u.set_parameter.size] = 0;
- ret = runtime_parse(params);
- }
-
- xfree(params);
-
- break;
- }
-
default:
ret = arch_do_sysctl(op, u_sysctl);
copyback = 0;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 3a08c512e8..f635c0c2db 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1026,22 +1026,6 @@ struct xen_sysctl_livepatch_op {
} u;
};
-/*
- * XEN_SYSCTL_set_parameter
- *
- * Change hypervisor parameters at runtime.
- * The input string is parsed similar to the boot parameters.
- * Parameters are a single string terminated by a NUL byte of max. size
- * characters. Multiple settings can be specified by separating them
- * with blanks.
- */
-
-struct xen_sysctl_set_parameter {
- XEN_GUEST_HANDLE_64(const_char) params; /* IN: pointer to parameters. */
- uint16_t size; /* IN: size of parameters. */
- uint16_t pad[3]; /* IN: MUST be zero. */
-};
-
#if defined(__i386__) || defined(__x86_64__)
/*
* XEN_SYSCTL_get_cpu_policy (x86 specific)
@@ -1106,7 +1090,7 @@ struct xen_sysctl {
#define XEN_SYSCTL_get_cpu_levelling_caps 25
#define XEN_SYSCTL_get_cpu_featureset 26
#define XEN_SYSCTL_livepatch_op 27
-#define XEN_SYSCTL_set_parameter 28
+/* #define XEN_SYSCTL_set_parameter 28 */
#define XEN_SYSCTL_get_cpu_policy 29
uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
union {
@@ -1135,7 +1119,6 @@ struct xen_sysctl {
struct xen_sysctl_cpu_levelling_caps cpu_levelling_caps;
struct xen_sysctl_cpu_featureset cpu_featureset;
struct xen_sysctl_livepatch_op livepatch;
- struct xen_sysctl_set_parameter set_parameter;
#if defined(__i386__) || defined(__x86_64__)
struct xen_sysctl_cpu_policy cpu_policy;
#endif
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index 095a0c9bee..3ba1eb4971 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -119,11 +119,6 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
-void hypfs_write_lock(void);
-void hypfs_write_unlock(void);
-#else
-static inline void hypfs_write_lock(void) {}
-static inline void hypfs_write_unlock(void) {}
#endif
#endif /* __XEN_HYPFS_H__ */
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 2d7a054931..e5b0a007b8 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -75,7 +75,6 @@
struct domain;
void cmdline_parse(const char *cmdline);
-int runtime_parse(const char *line);
int parse_bool(const char *s, const char *e);
/**
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index 4893de9bd5..ca032bf4f3 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -27,9 +27,6 @@ struct kernel_param {
};
extern const struct kernel_param __setup_start[], __setup_end[];
-extern const struct kernel_param __param_start[], __param_end[];
-
-#define __dataparam __used_section(".data.param")
#define __param(att) static const att \
__attribute__((__aligned__(sizeof(void *)))) struct kernel_param
@@ -79,14 +76,12 @@ extern const struct kernel_param __param_start[], __param_end[];
{ .name = setup_str_ign, \
.type = OPT_IGNORE }
-#define __rtparam __param(__dataparam)
-
#ifdef CONFIG_HYPFS
struct param_hypfs {
- const struct kernel_param *param;
struct hypfs_entry_leaf hypfs;
void (*init_leaf)(struct param_hypfs *par);
+ int (*func)(const char *);
};
extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
@@ -109,28 +104,17 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). */
#define custom_runtime_only_param(_name, _var, initfunc) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_CUSTOM, \
- .par.func = _var }; \
__paramfs __parfs_##_var = \
- { .param = &__rtpar_##_var, \
- .init_leaf = initfunc, \
- .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
+ { .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
.hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
.hypfs.e.name = _name, \
.hypfs.e.read = hypfs_read_leaf, \
- .hypfs.e.write = hypfs_write_custom }
+ .hypfs.e.write = hypfs_write_custom, \
+ .init_leaf = initfunc, \
+ .func = _var }
#define boolean_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_BOOL, \
- .len = sizeof(_var) + \
- BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
- .par.var = &_var }; \
__paramfs __parfs_##_var = \
- { .param = &__rtpar_##_var, \
- .hypfs.e.type = XEN_HYPFS_TYPE_BOOL, \
+ { .hypfs.e.type = XEN_HYPFS_TYPE_BOOL, \
.hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
.hypfs.e.name = _name, \
.hypfs.e.size = sizeof(_var), \
@@ -139,14 +123,8 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
.hypfs.e.write = hypfs_write_bool, \
.hypfs.content = &_var }
#define integer_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_UINT, \
- .len = sizeof(_var), \
- .par.var = &_var }; \
__paramfs __parfs_##_var = \
- { .param = &__rtpar_##_var, \
- .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
+ { .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
.hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
.hypfs.e.name = _name, \
.hypfs.e.size = sizeof(_var), \
@@ -155,14 +133,8 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
.hypfs.e.write = hypfs_write_leaf, \
.hypfs.content = &_var }
#define size_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_SIZE, \
- .len = sizeof(_var), \
- .par.var = &_var }; \
__paramfs __parfs_##_var = \
- { .param = &__rtpar_##_var, \
- .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
+ { .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
.hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
.hypfs.e.name = _name, \
.hypfs.e.size = sizeof(_var), \
@@ -171,14 +143,8 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
.hypfs.e.write = hypfs_write_leaf, \
.hypfs.content = &_var }
#define string_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_STR, \
- .len = sizeof(_var), \
- .par.var = &_var }; \
__paramfs __parfs_##_var = \
- { .param = &__rtpar_##_var, \
- .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
+ { .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
.hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
.hypfs.e.name = _name, \
.hypfs.e.size = sizeof(_var), \
@@ -194,36 +160,11 @@ struct param_hypfs {
#define param_2_parfs(par) NULL
-#define custom_runtime_only_param(_name, _var, initfunc) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_CUSTOM, \
- .par.func = _var }
-#define boolean_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_BOOL, \
- .len = sizeof(_var) + \
- BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
- .par.var = &_var }
-#define integer_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_UINT, \
- .len = sizeof(_var), \
- .par.var = &_var }
-#define size_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_SIZE, \
- .len = sizeof(_var), \
- .par.var = &_var }
-#define string_runtime_only_param(_name, _var) \
- __rtparam __rtpar_##_var = \
- { .name = _name, \
- .type = OPT_STR, \
- .len = sizeof(_var), \
- .par.var = &_var }
+#define custom_runtime_only_param(_name, _var, initfunc)
+#define boolean_runtime_only_param(_name, _var)
+#define integer_runtime_only_param(_name, _var)
+#define size_runtime_only_param(_name, _var)
+#define string_runtime_only_param(_name, _var)
#define custom_runtime_set_var(parfs, var)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index a2c78e445c..a314bf85ce 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -822,9 +822,6 @@ static int flask_sysctl(int cmd)
case XEN_SYSCTL_coverage_op:
return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
XEN2__COVERAGE_OP, NULL);
- case XEN_SYSCTL_set_parameter:
- return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
- XEN2__SET_PARAMETER, NULL);
default:
return avc_unknown_permission("sysctl", cmd);
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index c9e385fb9b..b87c99ea98 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -99,8 +99,6 @@ class xen2
livepatch_op
# XEN_SYSCTL_coverage_op
coverage_op
-# XEN_SYSCTL_set_parameter
- set_parameter
}
# Classes domain and domain2 consist of operations that a domain performs on
--
2.26.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-08 15:34 ` [PATCH v8 04/12] xen: add basic hypervisor filesystem support Juergen Gross
@ 2020-05-14 7:59 ` Jan Beulich
2020-05-14 9:50 ` Jürgen Groß
2020-05-14 9:55 ` Jan Beulich
1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 7:59 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 08.05.2020 17:34, Juergen Gross wrote:
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -10,6 +10,7 @@ obj-y += domain.o
> obj-y += event_2l.o
> obj-y += event_channel.o
> obj-y += event_fifo.o
> +obj-$(CONFIG_HYPFS) += hypfs.o
> obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
> obj-$(CONFIG_GRANT_TABLE) += grant_table.o
> obj-y += guestcopy.o
I guess I could/should have noticed this earlier - this isn't the
correct insertion point, considering that we try to keep these
alphabetically sorted.
> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
> + const char *path)
> +{
> + const char *end;
> + struct hypfs_entry *entry;
> + unsigned int name_len;
> +
> + if ( dir->e.type != XEN_HYPFS_TYPE_DIR )
> + return NULL;
> +
> + if ( !*path )
> + return &dir->e;
> +
> + end = strchr(path, '/');
> + if ( !end )
> + end = strchr(path, '\0');
> + name_len = end - path;
> +
> + list_for_each_entry ( entry, &dir->dirlist, list )
> + {
> + int cmp = strncmp(path, entry->name, name_len);
> + struct hypfs_entry_dir *d = container_of(entry,
> + struct hypfs_entry_dir, e);
> +
> + if ( cmp < 0 )
> + return NULL;
> + if ( !cmp && strlen(entry->name) == name_len )
> + return *end ? hypfs_get_entry_rel(d, end + 1) : entry;
The compiler may translate this into a tail call, but shouldn't
we be worried about the nesting depth in case it doesn't?
Perhaps this doesn't need addressing here, but rather by limiting
the depth at which new entries can be created?
> +int hypfs_read_dir(const struct hypfs_entry *entry,
> + XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +{
> + const struct hypfs_entry_dir *d;
> + const struct hypfs_entry *e;
> + unsigned int size = entry->size;
> +
> + d = container_of(entry, const struct hypfs_entry_dir, e);
> +
> + list_for_each_entry ( e, &d->dirlist, list )
This function, in particular because of being non-static, makes
me wonder how, with add_entry() taking a lock, it can be safe
without any locking. Initially I thought the justification might
be because all adding of entries is an init-time-only thing, but
various involved functions aren't marked __init (and it is at
least not implausible that down the road we might see new
entries getting added during certain hotplug operations).
I do realize that do_hypfs_op() takes the necessary read lock,
but then you're still building on the assumption that the
function is reachable through only that code path, despite
being non-static. An ASSERT() here would be the minimum I guess,
but with read locks now being recursive I don't see why you
couldn't read-lock here again.
The same goes for other non-static functions, albeit things may
become more interesting for functions living on the
XEN_HYPFS_OP_write_contents path (because write locks aren't
recursive [yet]). I notice even hypfs_get_entry() is non-static,
albeit I can't see why that is (perhaps a later patch needs it).
> +int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> + char *buf;
> + int ret;
> +
> + if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
> + leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
> + return -EDOM;
hypfs_write() checks ulen against max_size, but the function
being non-static makes me once again wonder whether at the very
leaset an ASSERT() wouldn't be wanted here.
> + buf = xmalloc_array(char, ulen);
> + if ( !buf )
> + return -ENOMEM;
> +
> + ret = -EFAULT;
> + if ( copy_from_guest(buf, uaddr, ulen) )
> + goto out;
> +
> + ret = -EINVAL;
> + if ( leaf->e.type == XEN_HYPFS_TYPE_STRING &&
> + memchr(buf, 0, ulen) != (buf + ulen - 1) )
> + goto out;
How does this check play with gzip-ed strings?
> +long do_hypfs_op(unsigned int cmd,
> + XEN_GUEST_HANDLE_PARAM(const_char) arg1, unsigned long arg2,
> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
> +{
> + int ret;
> + struct hypfs_entry *entry;
> + static char path[XEN_HYPFS_MAX_PATHLEN];
> +
> + if ( xsm_hypfs_op(XSM_PRIV) )
> + return -EPERM;
> +
> + if ( cmd == XEN_HYPFS_OP_get_version )
> + return XEN_HYPFS_VERSION;
Check that all args are zero/null for this sub-op, to allow future
extension?
> +#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
> + struct hypfs_entry_leaf __read_mostly var = { \
> + .e.type = typ, \
> + .e.encoding = XEN_HYPFS_ENC_PLAIN, \
> + .e.name = nam, \
> + .e.size = sizeof(contvar), \
> + .e.max_size = wr ? sizeof(contvar) : 0, \
> + .e.read = hypfs_read_leaf, \
> + .e.write = wr, \
> + .content = &contvar, \
> + }
At the example of this, some of the macros look like they want
parentheses added around uses of some of their parameters.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem
2020-05-08 15:34 ` [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
@ 2020-05-14 9:32 ` Jan Beulich
2020-05-14 9:52 ` Jürgen Groß
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 9:32 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel
On 08.05.2020 17:34, Juergen Gross wrote:
> 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a remark and one further adjustment:
> @@ -73,3 +74,14 @@ obj-$(CONFIG_UBSAN) += ubsan/
>
> obj-$(CONFIG_NEEDS_LIBELF) += libelf/
> obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/
> +
> +config.gz: ../.config
> + gzip -c $< >$@
> +
> +config_data.o: config.gz
> +
> +config_data.S: $(XEN_ROOT)/xen/tools/binfile
> + $(XEN_ROOT)/xen/tools/binfile $@ config.gz xen_config_data
This will want changing to the changed build infrastructure, such
that in default (non-verbose, non-silent) mode a line gets output
to stdout. But I'd be fine with this getting done subsequently.
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -100,5 +100,8 @@ extern enum system_state {
>
> bool_t is_active_kernel_text(unsigned long addr);
>
> +extern const char xen_config_data;
Surely you meant to add [] here, and then possibly omit the & on
the line using the symbol?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-14 7:59 ` Jan Beulich
@ 2020-05-14 9:50 ` Jürgen Groß
2020-05-14 11:58 ` Jan Beulich
2020-05-15 5:33 ` Jürgen Groß
0 siblings, 2 replies; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 9:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 09:59, Jan Beulich wrote:
> On 08.05.2020 17:34, Juergen Gross wrote:
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -10,6 +10,7 @@ obj-y += domain.o
>> obj-y += event_2l.o
>> obj-y += event_channel.o
>> obj-y += event_fifo.o
>> +obj-$(CONFIG_HYPFS) += hypfs.o
>> obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
>> obj-$(CONFIG_GRANT_TABLE) += grant_table.o
>> obj-y += guestcopy.o
>
> I guess I could/should have noticed this earlier - this isn't the
> correct insertion point, considering that we try to keep these
> alphabetically sorted.
Oh, the placement seems to be leftover from an early version when the
file was named fs.c or so.
>
>> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
>> + const char *path)
>> +{
>> + const char *end;
>> + struct hypfs_entry *entry;
>> + unsigned int name_len;
>> +
>> + if ( dir->e.type != XEN_HYPFS_TYPE_DIR )
>> + return NULL;
>> +
>> + if ( !*path )
>> + return &dir->e;
>> +
>> + end = strchr(path, '/');
>> + if ( !end )
>> + end = strchr(path, '\0');
>> + name_len = end - path;
>> +
>> + list_for_each_entry ( entry, &dir->dirlist, list )
>> + {
>> + int cmp = strncmp(path, entry->name, name_len);
>> + struct hypfs_entry_dir *d = container_of(entry,
>> + struct hypfs_entry_dir, e);
>> +
>> + if ( cmp < 0 )
>> + return NULL;
>> + if ( !cmp && strlen(entry->name) == name_len )
>> + return *end ? hypfs_get_entry_rel(d, end + 1) : entry;
>
> The compiler may translate this into a tail call, but shouldn't
> we be worried about the nesting depth in case it doesn't?
> Perhaps this doesn't need addressing here, but rather by limiting
> the depth at which new entries can be created?
Hmm, it should be possible to solve this without recursion. I'll have a
try.
>
>> +int hypfs_read_dir(const struct hypfs_entry *entry,
>> + XEN_GUEST_HANDLE_PARAM(void) uaddr)
>> +{
>> + const struct hypfs_entry_dir *d;
>> + const struct hypfs_entry *e;
>> + unsigned int size = entry->size;
>> +
>> + d = container_of(entry, const struct hypfs_entry_dir, e);
>> +
>> + list_for_each_entry ( e, &d->dirlist, list )
>
> This function, in particular because of being non-static, makes
> me wonder how, with add_entry() taking a lock, it can be safe
> without any locking. Initially I thought the justification might
> be because all adding of entries is an init-time-only thing, but
> various involved functions aren't marked __init (and it is at
> least not implausible that down the road we might see new
> entries getting added during certain hotplug operations).
>
> I do realize that do_hypfs_op() takes the necessary read lock,
> but then you're still building on the assumption that the
> function is reachable through only that code path, despite
> being non-static. An ASSERT() here would be the minimum I guess,
> but with read locks now being recursive I don't see why you
> couldn't read-lock here again.
Right, will add the read-lock.
>
> The same goes for other non-static functions, albeit things may
> become more interesting for functions living on the
> XEN_HYPFS_OP_write_contents path (because write locks aren't
Adding an ASSERT() in this regard should be rather easy.
> recursive [yet]). I notice even hypfs_get_entry() is non-static,
> albeit I can't see why that is (perhaps a later patch needs it).
No need for non-static, will change to static.
>
>> +int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
>> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
>> +{
>> + char *buf;
>> + int ret;
>> +
>> + if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
>> + leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
>> + return -EDOM;
>
> hypfs_write() checks ulen against max_size, but the function
> being non-static makes me once again wonder whether at the very
> leaset an ASSERT() wouldn't be wanted here.
Okay.
>
>> + buf = xmalloc_array(char, ulen);
>> + if ( !buf )
>> + return -ENOMEM;
>> +
>> + ret = -EFAULT;
>> + if ( copy_from_guest(buf, uaddr, ulen) )
>> + goto out;
>> +
>> + ret = -EINVAL;
>> + if ( leaf->e.type == XEN_HYPFS_TYPE_STRING &&
>> + memchr(buf, 0, ulen) != (buf + ulen - 1) )
>> + goto out;
>
> How does this check play with gzip-ed strings?
Hmm, right, will add a check for XEN_HYPFS_ENC_PLAIN.
>
>> +long do_hypfs_op(unsigned int cmd,
>> + XEN_GUEST_HANDLE_PARAM(const_char) arg1, unsigned long arg2,
>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>> +{
>> + int ret;
>> + struct hypfs_entry *entry;
>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>> +
>> + if ( xsm_hypfs_op(XSM_PRIV) )
>> + return -EPERM;
>> +
>> + if ( cmd == XEN_HYPFS_OP_get_version )
>> + return XEN_HYPFS_VERSION;
>
> Check that all args are zero/null for this sub-op, to allow future
> extension?
Yes, good idea.
>
>> +#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
>> + struct hypfs_entry_leaf __read_mostly var = { \
>> + .e.type = typ, \
>> + .e.encoding = XEN_HYPFS_ENC_PLAIN, \
>> + .e.name = nam, \
>> + .e.size = sizeof(contvar), \
>> + .e.max_size = wr ? sizeof(contvar) : 0, \
>> + .e.read = hypfs_read_leaf, \
>> + .e.write = wr, \
>> + .content = &contvar, \
>> + }
>
> At the example of this, some of the macros look like they want
> parentheses added around uses of some of their parameters.
Hmm, which ones? As I've understood from previous patch reviews by you
you only want those parameters with parantheses where they are really
needed.
- var is a plain variable, so no parantheses
- typ _should_ be a XEN_HYPFS_TYPE_* define, so probably no parantheses
(its usage in the macro doesn't call for using parantheses anyway)
- nam might be a candidate, but I can't come up with a reason to put it
in parantheses here
- contvar has to be a variable (otherwise sizeof(contvar) wouldn't
work), so no parantheses
- wr is a function pointer or NULL, so no parantheses
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem
2020-05-14 9:32 ` Jan Beulich
@ 2020-05-14 9:52 ` Jürgen Groß
0 siblings, 0 replies; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 9:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel
On 14.05.20 11:32, Jan Beulich wrote:
> On 08.05.2020 17:34, Juergen Gross wrote:
>> 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>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a remark and one further adjustment:
>
>> @@ -73,3 +74,14 @@ obj-$(CONFIG_UBSAN) += ubsan/
>>
>> obj-$(CONFIG_NEEDS_LIBELF) += libelf/
>> obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/
>> +
>> +config.gz: ../.config
>> + gzip -c $< >$@
>> +
>> +config_data.o: config.gz
>> +
>> +config_data.S: $(XEN_ROOT)/xen/tools/binfile
>> + $(XEN_ROOT)/xen/tools/binfile $@ config.gz xen_config_data
>
> This will want changing to the changed build infrastructure, such
> that in default (non-verbose, non-silent) mode a line gets output
> to stdout. But I'd be fine with this getting done subsequently.
>
>> --- a/xen/include/xen/kernel.h
>> +++ b/xen/include/xen/kernel.h
>> @@ -100,5 +100,8 @@ extern enum system_state {
>>
>> bool_t is_active_kernel_text(unsigned long addr);
>>
>> +extern const char xen_config_data;
>
> Surely you meant to add [] here, and then possibly omit the & on
> the line using the symbol?
Of course. ;-p
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-08 15:34 ` [PATCH v8 04/12] xen: add basic hypervisor filesystem support Juergen Gross
2020-05-14 7:59 ` Jan Beulich
@ 2020-05-14 9:55 ` Jan Beulich
2020-05-14 9:58 ` Jürgen Groß
1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 9:55 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 08.05.2020 17:34, Juergen Gross wrote:
> --- /dev/null
> +++ b/xen/include/xen/hypfs.h
> @@ -0,0 +1,122 @@
> +#ifndef __XEN_HYPFS_H__
> +#define __XEN_HYPFS_H__
> +
> +#ifdef CONFIG_HYPFS
> +#include <xen/list.h>
> +#include <xen/string.h>
> +#include <public/hypfs.h>
> +
> +struct hypfs_entry_leaf;
> +
> +struct hypfs_entry {
> + unsigned short type;
> + unsigned short encoding;
> + unsigned int size;
> + unsigned int max_size;
Btw with these, ...
> + const char *name;
> + struct list_head list;
> + int (*read)(const struct hypfs_entry *entry,
> + XEN_GUEST_HANDLE_PARAM(void) uaddr);
> + int (*write)(struct hypfs_entry_leaf *leaf,
> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
... why unsigned long here (noticed while looking at patch 9)?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-14 9:55 ` Jan Beulich
@ 2020-05-14 9:58 ` Jürgen Groß
0 siblings, 0 replies; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 9:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 11:55, Jan Beulich wrote:
> On 08.05.2020 17:34, Juergen Gross wrote:
>> --- /dev/null
>> +++ b/xen/include/xen/hypfs.h
>> @@ -0,0 +1,122 @@
>> +#ifndef __XEN_HYPFS_H__
>> +#define __XEN_HYPFS_H__
>> +
>> +#ifdef CONFIG_HYPFS
>> +#include <xen/list.h>
>> +#include <xen/string.h>
>> +#include <public/hypfs.h>
>> +
>> +struct hypfs_entry_leaf;
>> +
>> +struct hypfs_entry {
>> + unsigned short type;
>> + unsigned short encoding;
>> + unsigned int size;
>> + unsigned int max_size;
>
> Btw with these, ...
>
>> + const char *name;
>> + struct list_head list;
>> + int (*read)(const struct hypfs_entry *entry,
>> + XEN_GUEST_HANDLE_PARAM(void) uaddr);
>> + int (*write)(struct hypfs_entry_leaf *leaf,
>> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen);
>
> ... why unsigned long here (noticed while looking at patch 9)?
Indeed, will switch to unsigned int.
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-08 15:34 ` [PATCH v8 09/12] xen: add runtime parameter access support to hypfs Juergen Gross
@ 2020-05-14 10:20 ` Jan Beulich
2020-05-14 11:48 ` Jürgen Groß
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 10:20 UTC (permalink / raw)
To: Juergen Gross
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 08.05.2020 17:34, Juergen Gross wrote:
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -89,6 +89,13 @@ SECTIONS
> __start_schedulers_array = .;
> *(.data.schedulers)
> __end_schedulers_array = .;
> +
> +#ifdef CONFIG_HYPFS
> + . = ALIGN(8);
> + __paramhypfs_start = .;
> + *(.data.paramhypfs)
> + __paramhypfs_end = .;
> +#endif
> *(.data.rel)
> *(.data.rel.*)
> CONSTRUCTORS
I'm not the maintainer of this code, but I think it would be better
if there was either no blank line inserted, or two (a 2nd one after
your insertion).
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -52,9 +52,27 @@ static __read_mostly enum {
> PCID_OFF,
> PCID_ALL,
> PCID_XPTI,
> - PCID_NOXPTI
> + PCID_NOXPTI,
> + PCID_END
> } opt_pcid = PCID_XPTI;
Is this change really needed? The only use looks to be ...
> +#ifdef CONFIG_HYPFS
> +static const char opt_pcid_2_string[PCID_END][7] = {
... here, yet the arry would end up the same when using [][7].
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -85,8 +85,43 @@ struct grant_table {
> struct grant_table_arch arch;
> };
>
> -static int parse_gnttab_limit(const char *param, const char *arg,
> - unsigned int *valp)
> +unsigned int __read_mostly opt_max_grant_frames = 64;
> +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
> +
> +#ifdef CONFIG_HYPFS
> +#define GRANT_CUSTOM_VAL_SZ 12
> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
> +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
> +
> +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
> + char *parval)
> +{
> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
> + custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
> +}
> +
> +static void __init gnttab_max_frames_init(struct param_hypfs *par)
> +{
> + update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
> +}
> +
> +static void __init max_maptrack_frames_init(struct param_hypfs *par)
> +{
> + update_gnttab_par(par, opt_max_maptrack_frames,
> + opt_max_maptrack_frames_val);
> +}
> +#else
> +#define opt_max_grant_frames_val NULL
> +#define opt_max_maptrack_frames_val NULL
This looks latently dangerous to me (in case new uses of these
two identifiers appeared), but I guess my alternative suggestion
will be at best controversial, too:
#define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
#define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, valp)
(placed right here)
> @@ -281,6 +282,36 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
> return 0;
> }
>
> +int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> + struct param_hypfs *p;
> + char *buf;
> + int ret;
> +
> + buf = xzalloc_array(char, ulen);
> + if ( !buf )
> + return -ENOMEM;
> +
> + ret = -EFAULT;
> + if ( copy_from_guest(buf, uaddr, ulen) )
As just indicated in an extra reply to patch 4, ulen not getting
truncated here silently is well obscured (the max_size field type
and the check against it elsewhere looks to guarantee this).
> + goto out;
> +
> + ret = -EDOM;
> + if ( memchr(buf, 0, ulen) != (buf + ulen - 1) )
> + goto out;
> +
> + p = container_of(leaf, struct param_hypfs, hypfs);
> + ret = p->param->par.func(buf);
> +
> + if ( !ret )
> + leaf->e.size = ulen;
Why? For "ept", "no-exec-sp" would yield "exec-sp=0", and hence
you'd wrongly extend the size from what parse_ept_param_runtime()
has already set through custom_runtime_set_var(). It looks to me
as if there's no reason to update e.size here at all; it's the
par.func() handlers which need to take care of this.
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -75,12 +75,35 @@ enum con_timestamp_mode
> TSM_DATE_MS, /* [YYYY-MM-DD HH:MM:SS.mmm] */
> TSM_BOOT, /* [SSSSSS.uuuuuu] */
> TSM_RAW, /* [XXXXXXXXXXXXXXXX] */
> + TSM_END
> };
Just like for the PCID enumeration I don't think a sentinel is
needed here.
> static enum con_timestamp_mode __read_mostly opt_con_timestamp_mode = TSM_NONE;
>
> +#ifdef CONFIG_HYPFS
> +static const char con_timestamp_mode_2_string[TSM_END][7] = {
> + [TSM_NONE] = "none",
> + [TSM_DATE] = "date",
> + [TSM_DATE_MS] = "datems",
> + [TSM_BOOT] = "boot",
> + [TSM_RAW] = "raw"
Add a trailing comma please (and as I notice only now then also
in the similar PCID array).
To the subsequent code the gnttab comment applies as well.
> @@ -80,7 +81,120 @@ extern const struct kernel_param __param_start[], __param_end[];
>
> #define __rtparam __param(__dataparam)
>
> -#define custom_runtime_only_param(_name, _var) \
> +#ifdef CONFIG_HYPFS
> +
> +struct param_hypfs {
> + const struct kernel_param *param;
> + struct hypfs_entry_leaf hypfs;
> + void (*init_leaf)(struct param_hypfs *par);
> +};
> +
> +extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
> +
> +#define __paramhypfs __used_section(".data.paramhypfs")
> +
> +#define __paramfs static __paramhypfs \
> + __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs
Why the attribute?
> +#define custom_runtime_set_var_sz(parfs, var, sz) \
> + { \
> + (parfs)->hypfs.content = var; \
> + (parfs)->hypfs.e.max_size = sz; \
var and sz want parentheses around them.
> + (parfs)->hypfs.e.size = strlen(var) + 1; \
> + }
> +#define custom_runtime_set_var(parfs, var) \
> + custom_runtime_set_var_sz(parfs, var, sizeof(var))
> +
> +#define param_2_parfs(par) &__parfs_##par
> +
> +/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). */
> +#define custom_runtime_only_param(_name, _var, initfunc) \
I've started noticing it here, but the issue exists further up
(and down) as well - please can you avoid identifiers with
leading underscores that are in violation of the C standard?
Even more so that here you're not even consistent across
macro parameter names.
> + __rtparam __rtpar_##_var = \
> + { .name = _name, \
> + .type = OPT_CUSTOM, \
> + .par.func = _var }; \
> + __paramfs __parfs_##_var = \
> + { .param = &__rtpar_##_var, \
> + .init_leaf = initfunc, \
> + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
> + .hypfs.e.name = _name, \
> + .hypfs.e.read = hypfs_read_leaf, \
> + .hypfs.e.write = hypfs_write_custom }
> +#define boolean_runtime_only_param(_name, _var) \
> + __rtparam __rtpar_##_var = \
> + { .name = _name, \
> + .type = OPT_BOOL, \
> + .len = sizeof(_var) + \
> + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
> + .par.var = &_var }; \
> + __paramfs __parfs_##_var = \
> + { .param = &__rtpar_##_var, \
> + .hypfs.e.type = XEN_HYPFS_TYPE_BOOL, \
> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
> + .hypfs.e.name = _name, \
> + .hypfs.e.size = sizeof(_var), \
> + .hypfs.e.max_size = sizeof(_var), \
> + .hypfs.e.read = hypfs_read_leaf, \
> + .hypfs.e.write = hypfs_write_bool, \
> + .hypfs.content = &_var }
> +#define integer_runtime_only_param(_name, _var) \
> + __rtparam __rtpar_##_var = \
> + { .name = _name, \
> + .type = OPT_UINT, \
> + .len = sizeof(_var), \
> + .par.var = &_var }; \
> + __paramfs __parfs_##_var = \
> + { .param = &__rtpar_##_var, \
> + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
> + .hypfs.e.name = _name, \
> + .hypfs.e.size = sizeof(_var), \
> + .hypfs.e.max_size = sizeof(_var), \
> + .hypfs.e.read = hypfs_read_leaf, \
> + .hypfs.e.write = hypfs_write_leaf, \
> + .hypfs.content = &_var }
> +#define size_runtime_only_param(_name, _var) \
> + __rtparam __rtpar_##_var = \
> + { .name = _name, \
> + .type = OPT_SIZE, \
> + .len = sizeof(_var), \
> + .par.var = &_var }; \
> + __paramfs __parfs_##_var = \
> + { .param = &__rtpar_##_var, \
> + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
> + .hypfs.e.name = _name, \
> + .hypfs.e.size = sizeof(_var), \
> + .hypfs.e.max_size = sizeof(_var), \
> + .hypfs.e.read = hypfs_read_leaf, \
> + .hypfs.e.write = hypfs_write_leaf, \
> + .hypfs.content = &_var }
> +#define string_runtime_only_param(_name, _var) \
> + __rtparam __rtpar_##_var = \
> + { .name = _name, \
> + .type = OPT_STR, \
> + .len = sizeof(_var), \
> + .par.var = &_var }; \
> + __paramfs __parfs_##_var = \
> + { .param = &__rtpar_##_var, \
> + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
> + .hypfs.e.name = _name, \
> + .hypfs.e.size = sizeof(_var), \
Is this really correct here?
> + .hypfs.e.max_size = sizeof(_var), \
> + .hypfs.e.read = hypfs_read_leaf, \
> + .hypfs.e.write = hypfs_write_leaf, \
> + .hypfs.content = &_var }
> +
> +#else
> +
> +struct param_hypfs {
> +};
> +
> +#define param_2_parfs(par) NULL
Along the lines of the earlier comment, this looks latently dangerous.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-14 10:20 ` Jan Beulich
@ 2020-05-14 11:48 ` Jürgen Groß
2020-05-14 12:10 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 11:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 12:20, Jan Beulich wrote:
> On 08.05.2020 17:34, Juergen Gross wrote:
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -89,6 +89,13 @@ SECTIONS
>> __start_schedulers_array = .;
>> *(.data.schedulers)
>> __end_schedulers_array = .;
>> +
>> +#ifdef CONFIG_HYPFS
>> + . = ALIGN(8);
>> + __paramhypfs_start = .;
>> + *(.data.paramhypfs)
>> + __paramhypfs_end = .;
>> +#endif
>> *(.data.rel)
>> *(.data.rel.*)
>> CONSTRUCTORS
>
> I'm not the maintainer of this code, but I think it would be better
> if there was either no blank line inserted, or two (a 2nd one after
> your insertion).
Okay (either one).
>
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -52,9 +52,27 @@ static __read_mostly enum {
>> PCID_OFF,
>> PCID_ALL,
>> PCID_XPTI,
>> - PCID_NOXPTI
>> + PCID_NOXPTI,
>> + PCID_END
>> } opt_pcid = PCID_XPTI;
>
> Is this change really needed? The only use looks to be ...
>
>> +#ifdef CONFIG_HYPFS
>> +static const char opt_pcid_2_string[PCID_END][7] = {
>
> ... here, yet the arry would end up the same when using [][7].
Hmm, true.
>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -85,8 +85,43 @@ struct grant_table {
>> struct grant_table_arch arch;
>> };
>>
>> -static int parse_gnttab_limit(const char *param, const char *arg,
>> - unsigned int *valp)
>> +unsigned int __read_mostly opt_max_grant_frames = 64;
>> +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
>> +
>> +#ifdef CONFIG_HYPFS
>> +#define GRANT_CUSTOM_VAL_SZ 12
>> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
>> +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
>> +
>> +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
>> + char *parval)
>> +{
>> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
>> + custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
>> +}
>> +
>> +static void __init gnttab_max_frames_init(struct param_hypfs *par)
>> +{
>> + update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
>> +}
>> +
>> +static void __init max_maptrack_frames_init(struct param_hypfs *par)
>> +{
>> + update_gnttab_par(par, opt_max_maptrack_frames,
>> + opt_max_maptrack_frames_val);
>> +}
>> +#else
>> +#define opt_max_grant_frames_val NULL
>> +#define opt_max_maptrack_frames_val NULL
>
> This looks latently dangerous to me (in case new uses of these
> two identifiers appeared), but I guess my alternative suggestion
> will be at best controversial, too:
>
> #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
> #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, valp)
>
> (placed right here)
Who else would use those identifiers not related to hypfs?
>
>> @@ -281,6 +282,36 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
>> return 0;
>> }
>>
>> +int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
>> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
>> +{
>> + struct param_hypfs *p;
>> + char *buf;
>> + int ret;
>> +
>> + buf = xzalloc_array(char, ulen);
>> + if ( !buf )
>> + return -ENOMEM;
>> +
>> + ret = -EFAULT;
>> + if ( copy_from_guest(buf, uaddr, ulen) )
>
> As just indicated in an extra reply to patch 4, ulen not getting
> truncated here silently is well obscured (the max_size field type
> and the check against it elsewhere looks to guarantee this).
Yes, will change ulen to unsigned int.
>
>> + goto out;
>> +
>> + ret = -EDOM;
>> + if ( memchr(buf, 0, ulen) != (buf + ulen - 1) )
>> + goto out;
>> +
>> + p = container_of(leaf, struct param_hypfs, hypfs);
>> + ret = p->param->par.func(buf);
>> +
>> + if ( !ret )
>> + leaf->e.size = ulen;
>
> Why? For "ept", "no-exec-sp" would yield "exec-sp=0", and hence
> you'd wrongly extend the size from what parse_ept_param_runtime()
> has already set through custom_runtime_set_var(). It looks to me
> as if there's no reason to update e.size here at all; it's the
> par.func() handlers which need to take care of this.
Oh, indeed. Thanks for catching.
>
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -75,12 +75,35 @@ enum con_timestamp_mode
>> TSM_DATE_MS, /* [YYYY-MM-DD HH:MM:SS.mmm] */
>> TSM_BOOT, /* [SSSSSS.uuuuuu] */
>> TSM_RAW, /* [XXXXXXXXXXXXXXXX] */
>> + TSM_END
>> };
>
> Just like for the PCID enumeration I don't think a sentinel is
> needed here.
Yes.
>
>> static enum con_timestamp_mode __read_mostly opt_con_timestamp_mode = TSM_NONE;
>>
>> +#ifdef CONFIG_HYPFS
>> +static const char con_timestamp_mode_2_string[TSM_END][7] = {
>> + [TSM_NONE] = "none",
>> + [TSM_DATE] = "date",
>> + [TSM_DATE_MS] = "datems",
>> + [TSM_BOOT] = "boot",
>> + [TSM_RAW] = "raw"
>
> Add a trailing comma please (and as I notice only now then also
> in the similar PCID array).
>
> To the subsequent code the gnttab comment applies as well.
>
>> @@ -80,7 +81,120 @@ extern const struct kernel_param __param_start[], __param_end[];
>>
>> #define __rtparam __param(__dataparam)
>>
>> -#define custom_runtime_only_param(_name, _var) \
>> +#ifdef CONFIG_HYPFS
>> +
>> +struct param_hypfs {
>> + const struct kernel_param *param;
>> + struct hypfs_entry_leaf hypfs;
>> + void (*init_leaf)(struct param_hypfs *par);
>> +};
>> +
>> +extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
>> +
>> +#define __paramhypfs __used_section(".data.paramhypfs")
>> +
>> +#define __paramfs static __paramhypfs \
>> + __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs
>
> Why the attribute?
Oh, copy and paste leftover.
>
>> +#define custom_runtime_set_var_sz(parfs, var, sz) \
>> + { \
>> + (parfs)->hypfs.content = var; \
>> + (parfs)->hypfs.e.max_size = sz; \
>
> var and sz want parentheses around them.
Fine with me, but out of curiosity: what can go wrong without? Are
you thinking of multi-statement arguments?
>
>> + (parfs)->hypfs.e.size = strlen(var) + 1; \
>> + }
>> +#define custom_runtime_set_var(parfs, var) \
>> + custom_runtime_set_var_sz(parfs, var, sizeof(var))
>> +
>> +#define param_2_parfs(par) &__parfs_##par
>> +
>> +/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). */
>> +#define custom_runtime_only_param(_name, _var, initfunc) \
>
> I've started noticing it here, but the issue exists further up
> (and down) as well - please can you avoid identifiers with
> leading underscores that are in violation of the C standard?
> Even more so that here you're not even consistent across
> macro parameter names.
Basically I only extended the macro to take another parameter and I
omitted the underscore exactly for the reason you mentioned.
In case you like it better I can prepend a patch to the series dropping
all the leading single underscores from the macros in param.h.
>
>> + __rtparam __rtpar_##_var = \
>> + { .name = _name, \
>> + .type = OPT_CUSTOM, \
>> + .par.func = _var }; \
>> + __paramfs __parfs_##_var = \
>> + { .param = &__rtpar_##_var, \
>> + .init_leaf = initfunc, \
>> + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
>> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
>> + .hypfs.e.name = _name, \
>> + .hypfs.e.read = hypfs_read_leaf, \
>> + .hypfs.e.write = hypfs_write_custom }
>> +#define boolean_runtime_only_param(_name, _var) \
>> + __rtparam __rtpar_##_var = \
>> + { .name = _name, \
>> + .type = OPT_BOOL, \
>> + .len = sizeof(_var) + \
>> + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
>> + .par.var = &_var }; \
>> + __paramfs __parfs_##_var = \
>> + { .param = &__rtpar_##_var, \
>> + .hypfs.e.type = XEN_HYPFS_TYPE_BOOL, \
>> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
>> + .hypfs.e.name = _name, \
>> + .hypfs.e.size = sizeof(_var), \
>> + .hypfs.e.max_size = sizeof(_var), \
>> + .hypfs.e.read = hypfs_read_leaf, \
>> + .hypfs.e.write = hypfs_write_bool, \
>> + .hypfs.content = &_var }
>> +#define integer_runtime_only_param(_name, _var) \
>> + __rtparam __rtpar_##_var = \
>> + { .name = _name, \
>> + .type = OPT_UINT, \
>> + .len = sizeof(_var), \
>> + .par.var = &_var }; \
>> + __paramfs __parfs_##_var = \
>> + { .param = &__rtpar_##_var, \
>> + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
>> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
>> + .hypfs.e.name = _name, \
>> + .hypfs.e.size = sizeof(_var), \
>> + .hypfs.e.max_size = sizeof(_var), \
>> + .hypfs.e.read = hypfs_read_leaf, \
>> + .hypfs.e.write = hypfs_write_leaf, \
>> + .hypfs.content = &_var }
>> +#define size_runtime_only_param(_name, _var) \
>> + __rtparam __rtpar_##_var = \
>> + { .name = _name, \
>> + .type = OPT_SIZE, \
>> + .len = sizeof(_var), \
>> + .par.var = &_var }; \
>> + __paramfs __parfs_##_var = \
>> + { .param = &__rtpar_##_var, \
>> + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \
>> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
>> + .hypfs.e.name = _name, \
>> + .hypfs.e.size = sizeof(_var), \
>> + .hypfs.e.max_size = sizeof(_var), \
>> + .hypfs.e.read = hypfs_read_leaf, \
>> + .hypfs.e.write = hypfs_write_leaf, \
>> + .hypfs.content = &_var }
>> +#define string_runtime_only_param(_name, _var) \
>> + __rtparam __rtpar_##_var = \
>> + { .name = _name, \
>> + .type = OPT_STR, \
>> + .len = sizeof(_var), \
>> + .par.var = &_var }; \
>> + __paramfs __parfs_##_var = \
>> + { .param = &__rtpar_##_var, \
>> + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
>> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
>> + .hypfs.e.name = _name, \
>> + .hypfs.e.size = sizeof(_var), \
>
> Is this really correct here?
Hmm, right, 0 might be the better choice here, even if it shouldn't
really matter.
>
>> + .hypfs.e.max_size = sizeof(_var), \
>> + .hypfs.e.read = hypfs_read_leaf, \
>> + .hypfs.e.write = hypfs_write_leaf, \
>> + .hypfs.content = &_var }
>> +
>> +#else
>> +
>> +struct param_hypfs {
>> +};
>> +
>> +#define param_2_parfs(par) NULL
>
> Along the lines of the earlier comment, this looks latently dangerous.
In which way? How can the empty struct be dereferenced in a way not
resulting in build time errors, other than using a cast which would
be obviously wrong for the standard case when CONFIG_HYPFS is defined?
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-14 9:50 ` Jürgen Groß
@ 2020-05-14 11:58 ` Jan Beulich
2020-05-14 12:12 ` Jürgen Groß
2020-05-15 5:33 ` Jürgen Groß
1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 11:58 UTC (permalink / raw)
To: Jürgen Groß
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 14.05.2020 11:50, Jürgen Groß wrote:
> On 14.05.20 09:59, Jan Beulich wrote:
>> On 08.05.2020 17:34, Juergen Gross wrote:
>>> +#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
>>> + struct hypfs_entry_leaf __read_mostly var = { \
>>> + .e.type = typ, \
>>> + .e.encoding = XEN_HYPFS_ENC_PLAIN, \
>>> + .e.name = nam, \
>>> + .e.size = sizeof(contvar), \
>>> + .e.max_size = wr ? sizeof(contvar) : 0, \
>>> + .e.read = hypfs_read_leaf, \
>>> + .e.write = wr, \
>>> + .content = &contvar, \
>>> + }
>>
>> At the example of this, some of the macros look like they want
>> parentheses added around uses of some of their parameters.
>
> Hmm, which ones? As I've understood from previous patch reviews by you
> you only want those parameters with parantheses where they are really
> needed.
>
> - var is a plain variable, so no parantheses
> - typ _should_ be a XEN_HYPFS_TYPE_* define, so probably no parantheses
> (its usage in the macro doesn't call for using parantheses anyway)
> - nam might be a candidate, but I can't come up with a reason to put it
> in parantheses here
> - contvar has to be a variable (otherwise sizeof(contvar) wouldn't
> work), so no parantheses
> - wr is a function pointer or NULL, so no parantheses
You have a point for uses as initializers, as there's no lower
precedence operator than the assignment ones except comma,
which would need parenthesizing in the macro invocation already.
However, I disagree on what you say about contvar and wr -
contvar is expected, but not required to be just an identifier.
And wr in turn is expected but not required to by an identifier
or NULL. I.e. the respective two lines where I think parentheses
can't be avoided are
.e.max_size = (wr) ? sizeof(contvar) : 0,
and
.content = &(contvar),
.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-14 11:48 ` Jürgen Groß
@ 2020-05-14 12:10 ` Jan Beulich
2020-05-14 14:56 ` Jürgen Groß
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 12:10 UTC (permalink / raw)
To: Jürgen Groß
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 14.05.2020 13:48, Jürgen Groß wrote:
> On 14.05.20 12:20, Jan Beulich wrote:
>> On 08.05.2020 17:34, Juergen Gross wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -85,8 +85,43 @@ struct grant_table {
>>> struct grant_table_arch arch;
>>> };
>>> -static int parse_gnttab_limit(const char *param, const char *arg,
>>> - unsigned int *valp)
>>> +unsigned int __read_mostly opt_max_grant_frames = 64;
>>> +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
>>> +
>>> +#ifdef CONFIG_HYPFS
>>> +#define GRANT_CUSTOM_VAL_SZ 12
>>> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
>>> +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
>>> +
>>> +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
>>> + char *parval)
>>> +{
>>> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
>>> + custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
>>> +}
>>> +
>>> +static void __init gnttab_max_frames_init(struct param_hypfs *par)
>>> +{
>>> + update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
>>> +}
>>> +
>>> +static void __init max_maptrack_frames_init(struct param_hypfs *par)
>>> +{
>>> + update_gnttab_par(par, opt_max_maptrack_frames,
>>> + opt_max_maptrack_frames_val);
>>> +}
>>> +#else
>>> +#define opt_max_grant_frames_val NULL
>>> +#define opt_max_maptrack_frames_val NULL
>>
>> This looks latently dangerous to me (in case new uses of these
>> two identifiers appeared), but I guess my alternative suggestion
>> will be at best controversial, too:
>>
>> #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
>> #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, valp)
>>
>> (placed right here)
>
> Who else would use those identifiers not related to hypfs?
I can't see an obvious possible use, but people get creative, i.e.
you never know. Passing NULL into a function without it being
blindingly obvious that it won't ever get (de)referenced is an at
least theoretical risk imo.
>>> +#define custom_runtime_set_var_sz(parfs, var, sz) \
>>> + { \
>>> + (parfs)->hypfs.content = var; \
>>> + (parfs)->hypfs.e.max_size = sz; \
>>
>> var and sz want parentheses around them.
>
> Fine with me, but out of curiosity: what can go wrong without? Are
> you thinking of multi-statement arguments?
Well, as just said in the reply on the patch 4 thread, you have
a point about this being the right side of an assignment.
Nevertheless such uses would look more consistent if parenthesized.
The only cases where I see it reasonable to omit parentheses are
- parameters made the operands of # or ##,
- parameters handed on to further macros / functions unaltered,
- parameters representing struct/union field names,
- perhaps other special cases along the lines of the above that
I can't seem to be able to think of right now.
>>> +/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). */
>>> +#define custom_runtime_only_param(_name, _var, initfunc) \
>>
>> I've started noticing it here, but the issue exists further up
>> (and down) as well - please can you avoid identifiers with
>> leading underscores that are in violation of the C standard?
>> Even more so that here you're not even consistent across
>> macro parameter names.
>
> Basically I only extended the macro to take another parameter and I
> omitted the underscore exactly for the reason you mentioned.
>
> In case you like it better I can prepend a patch to the series dropping
> all the leading single underscores from the macros in param.h.
That's code churn I don't view as strictly necessary - adjusting
these can be done when they get touched anyway. But here we have
a whole new body of code.
>>> +#define param_2_parfs(par) NULL
>>
>> Along the lines of the earlier comment, this looks latently dangerous.
>
> In which way? How can the empty struct be dereferenced in a way not
> resulting in build time errors, other than using a cast which would
> be obviously wrong for the standard case when CONFIG_HYPFS is defined?
Have the result of the macro passed to a function taking void *, e.g.
memcpy().
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-14 11:58 ` Jan Beulich
@ 2020-05-14 12:12 ` Jürgen Groß
0 siblings, 0 replies; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 12:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 13:58, Jan Beulich wrote:
> On 14.05.2020 11:50, Jürgen Groß wrote:
>> On 14.05.20 09:59, Jan Beulich wrote:
>>> On 08.05.2020 17:34, Juergen Gross wrote:
>>>> +#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
>>>> + struct hypfs_entry_leaf __read_mostly var = { \
>>>> + .e.type = typ, \
>>>> + .e.encoding = XEN_HYPFS_ENC_PLAIN, \
>>>> + .e.name = nam, \
>>>> + .e.size = sizeof(contvar), \
>>>> + .e.max_size = wr ? sizeof(contvar) : 0, \
>>>> + .e.read = hypfs_read_leaf, \
>>>> + .e.write = wr, \
>>>> + .content = &contvar, \
>>>> + }
>>>
>>> At the example of this, some of the macros look like they want
>>> parentheses added around uses of some of their parameters.
>>
>> Hmm, which ones? As I've understood from previous patch reviews by you
>> you only want those parameters with parantheses where they are really
>> needed.
>>
>> - var is a plain variable, so no parantheses
>> - typ _should_ be a XEN_HYPFS_TYPE_* define, so probably no parantheses
>> (its usage in the macro doesn't call for using parantheses anyway)
>> - nam might be a candidate, but I can't come up with a reason to put it
>> in parantheses here
>> - contvar has to be a variable (otherwise sizeof(contvar) wouldn't
>> work), so no parantheses
>> - wr is a function pointer or NULL, so no parantheses
>
> You have a point for uses as initializers, as there's no lower
> precedence operator than the assignment ones except comma,
> which would need parenthesizing in the macro invocation already.
> However, I disagree on what you say about contvar and wr -
> contvar is expected, but not required to be just an identifier.
> And wr in turn is expected but not required to by an identifier
> or NULL. I.e. the respective two lines where I think parentheses
> can't be avoided are
>
> .e.max_size = (wr) ? sizeof(contvar) : 0,
>
> and
>
> .content = &(contvar),
Okay.
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support
2020-05-08 15:34 ` [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support Juergen Gross
@ 2020-05-14 13:03 ` Jan Beulich
2020-05-14 14:58 ` Jürgen Groß
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 13:03 UTC (permalink / raw)
To: Juergen Gross
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Daniel De Graaf, Volodymyr Babchuk,
Roger Pau Monné
On 08.05.2020 17:34, Juergen Gross wrote:
Besides the changes made, wouldn't it be worthwhile to change
HYPFS Kconfig help wording from "result in some features not
being available" to something mentioning runtime parameter
setting in particular, and perhaps also from "might" to "will"?
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -71,27 +71,6 @@ static bool __read_mostly opt_ept_pml = true;
> static s8 __read_mostly opt_ept_ad = -1;
> int8_t __read_mostly opt_ept_exec_sp = -1;
>
> -#ifdef CONFIG_HYPFS
> -static char opt_ept_setting[10];
> -
> -static void update_ept_param(void)
> -{
> - if ( opt_ept_exec_sp >= 0 )
> - snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
> - opt_ept_exec_sp);
> -}
> -
> -static void __init init_ept_param(struct param_hypfs *par)
> -{
> - update_ept_param();
> - custom_runtime_set_var(par, opt_ept_setting);
> -}
> -#else
> -static void update_ept_param(void)
> -{
> -}
> -#endif
> -
> static int __init parse_ept_param(const char *s)
> {
> const char *ss;
> @@ -118,6 +97,22 @@ static int __init parse_ept_param(const char *s)
> }
> custom_param("ept", parse_ept_param);
>
> +#ifdef CONFIG_HYPFS
> +static char opt_ept_setting[10];
> +
> +static void update_ept_param(void)
> +{
> + if ( opt_ept_exec_sp >= 0 )
> + snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
> + opt_ept_exec_sp);
> +}
> +
> +static void __init init_ept_param(struct param_hypfs *par)
> +{
> + update_ept_param();
> + custom_runtime_set_var(par, opt_ept_setting);
> +}
If I'm not mistaken this is pure code movement, and solely to be
able to have ...
> +
> static int parse_ept_param_runtime(const char *s);
> custom_runtime_only_param("ept", parse_ept_param_runtime, init_ept_param);
>
> @@ -172,6 +167,7 @@ static int parse_ept_param_runtime(const char *s)
>
> return 0;
> }
> +#endif
... a single #ifdef region now including a few more lines. No
strict need to change it, but couldn't the earlier patch have
inserted the code in its final place right away?
> @@ -1106,7 +1090,7 @@ struct xen_sysctl {
> #define XEN_SYSCTL_get_cpu_levelling_caps 25
> #define XEN_SYSCTL_get_cpu_featureset 26
> #define XEN_SYSCTL_livepatch_op 27
> -#define XEN_SYSCTL_set_parameter 28
> +/* #define XEN_SYSCTL_set_parameter 28 */
Nit: There are now 3 too many padding spaces. Granted that's
how it was done for XEN_SYSCTL_tmem_op ...
In any event, as before,
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-14 12:10 ` Jan Beulich
@ 2020-05-14 14:56 ` Jürgen Groß
2020-05-14 15:02 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 14:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 14:10, Jan Beulich wrote:
> On 14.05.2020 13:48, Jürgen Groß wrote:
>> On 14.05.20 12:20, Jan Beulich wrote:
>>> On 08.05.2020 17:34, Juergen Gross wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -85,8 +85,43 @@ struct grant_table {
>>>> struct grant_table_arch arch;
>>>> };
>>>> -static int parse_gnttab_limit(const char *param, const char *arg,
>>>> - unsigned int *valp)
>>>> +unsigned int __read_mostly opt_max_grant_frames = 64;
>>>> +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
>>>> +
>>>> +#ifdef CONFIG_HYPFS
>>>> +#define GRANT_CUSTOM_VAL_SZ 12
>>>> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
>>>> +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
>>>> +
>>>> +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
>>>> + char *parval)
>>>> +{
>>>> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
>>>> + custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
>>>> +}
>>>> +
>>>> +static void __init gnttab_max_frames_init(struct param_hypfs *par)
>>>> +{
>>>> + update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
>>>> +}
>>>> +
>>>> +static void __init max_maptrack_frames_init(struct param_hypfs *par)
>>>> +{
>>>> + update_gnttab_par(par, opt_max_maptrack_frames,
>>>> + opt_max_maptrack_frames_val);
>>>> +}
>>>> +#else
>>>> +#define opt_max_grant_frames_val NULL
>>>> +#define opt_max_maptrack_frames_val NULL
>>>
>>> This looks latently dangerous to me (in case new uses of these
>>> two identifiers appeared), but I guess my alternative suggestion
>>> will be at best controversial, too:
>>>
>>> #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
>>> #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, valp)
>>>
>>> (placed right here)
>>
>> Who else would use those identifiers not related to hypfs?
>
> I can't see an obvious possible use, but people get creative, i.e.
> you never know. Passing NULL into a function without it being
> blindingly obvious that it won't ever get (de)referenced is an at
> least theoretical risk imo.
Hmm, what about using a special type for those content variables?
Something like:
#ifdef CONFIG_HYPFS
#define hypfs_string_var char *
#else
#define hypfs_string_var char
#define opt_max_grant_frames_val 0
#define opt_max_maptrack_frames_val 0
#endif
And then use that as type for function parameters? This should make
dereferencing pretty hard.
Other than that I have no really good idea how to avoid this problem.
>
>>>> +#define custom_runtime_set_var_sz(parfs, var, sz) \
>>>> + { \
>>>> + (parfs)->hypfs.content = var; \
>>>> + (parfs)->hypfs.e.max_size = sz; \
>>>
>>> var and sz want parentheses around them.
>>
>> Fine with me, but out of curiosity: what can go wrong without? Are
>> you thinking of multi-statement arguments?
>
> Well, as just said in the reply on the patch 4 thread, you have
> a point about this being the right side of an assignment.
> Nevertheless such uses would look more consistent if parenthesized.
> The only cases where I see it reasonable to omit parentheses are
> - parameters made the operands of # or ##,
> - parameters handed on to further macros / functions unaltered,
> - parameters representing struct/union field names,
> - perhaps other special cases along the lines of the above that
> I can't seem to be able to think of right now.
Okay.
>
>>>> +/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). */
>>>> +#define custom_runtime_only_param(_name, _var, initfunc) \
>>>
>>> I've started noticing it here, but the issue exists further up
>>> (and down) as well - please can you avoid identifiers with
>>> leading underscores that are in violation of the C standard?
>>> Even more so that here you're not even consistent across
>>> macro parameter names.
>>
>> Basically I only extended the macro to take another parameter and I
>> omitted the underscore exactly for the reason you mentioned.
>>
>> In case you like it better I can prepend a patch to the series dropping
>> all the leading single underscores from the macros in param.h.
>
> That's code churn I don't view as strictly necessary - adjusting
> these can be done when they get touched anyway. But here we have
> a whole new body of code.
Okay.
>
>>>> +#define param_2_parfs(par) NULL
>>>
>>> Along the lines of the earlier comment, this looks latently dangerous.
>>
>> In which way? How can the empty struct be dereferenced in a way not
>> resulting in build time errors, other than using a cast which would
>> be obviously wrong for the standard case when CONFIG_HYPFS is defined?
>
> Have the result of the macro passed to a function taking void *, e.g.
> memcpy().
Any function like this needs either a guaranteed minimum size or a
specific size as parameter. I can't imagine a valid use case leading
to problems, TBH.
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support
2020-05-14 13:03 ` Jan Beulich
@ 2020-05-14 14:58 ` Jürgen Groß
0 siblings, 0 replies; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 14:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Daniel De Graaf, Volodymyr Babchuk,
Roger Pau Monné
On 14.05.20 15:03, Jan Beulich wrote:
> On 08.05.2020 17:34, Juergen Gross wrote:
>
> Besides the changes made, wouldn't it be worthwhile to change
> HYPFS Kconfig help wording from "result in some features not
> being available" to something mentioning runtime parameter
> setting in particular, and perhaps also from "might" to "will"?
Yes, good idea.
>
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -71,27 +71,6 @@ static bool __read_mostly opt_ept_pml = true;
>> static s8 __read_mostly opt_ept_ad = -1;
>> int8_t __read_mostly opt_ept_exec_sp = -1;
>>
>> -#ifdef CONFIG_HYPFS
>> -static char opt_ept_setting[10];
>> -
>> -static void update_ept_param(void)
>> -{
>> - if ( opt_ept_exec_sp >= 0 )
>> - snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
>> - opt_ept_exec_sp);
>> -}
>> -
>> -static void __init init_ept_param(struct param_hypfs *par)
>> -{
>> - update_ept_param();
>> - custom_runtime_set_var(par, opt_ept_setting);
>> -}
>> -#else
>> -static void update_ept_param(void)
>> -{
>> -}
>> -#endif
>> -
>> static int __init parse_ept_param(const char *s)
>> {
>> const char *ss;
>> @@ -118,6 +97,22 @@ static int __init parse_ept_param(const char *s)
>> }
>> custom_param("ept", parse_ept_param);
>>
>> +#ifdef CONFIG_HYPFS
>> +static char opt_ept_setting[10];
>> +
>> +static void update_ept_param(void)
>> +{
>> + if ( opt_ept_exec_sp >= 0 )
>> + snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
>> + opt_ept_exec_sp);
>> +}
>> +
>> +static void __init init_ept_param(struct param_hypfs *par)
>> +{
>> + update_ept_param();
>> + custom_runtime_set_var(par, opt_ept_setting);
>> +}
>
> If I'm not mistaken this is pure code movement, and solely to be
> able to have ...
>
>> +
>> static int parse_ept_param_runtime(const char *s);
>> custom_runtime_only_param("ept", parse_ept_param_runtime, init_ept_param);
>>
>> @@ -172,6 +167,7 @@ static int parse_ept_param_runtime(const char *s)
>>
>> return 0;
>> }
>> +#endif
>
> ... a single #ifdef region now including a few more lines. No
> strict need to change it, but couldn't the earlier patch have
> inserted the code in its final place right away?
I can do that.
>
>> @@ -1106,7 +1090,7 @@ struct xen_sysctl {
>> #define XEN_SYSCTL_get_cpu_levelling_caps 25
>> #define XEN_SYSCTL_get_cpu_featureset 26
>> #define XEN_SYSCTL_livepatch_op 27
>> -#define XEN_SYSCTL_set_parameter 28
>> +/* #define XEN_SYSCTL_set_parameter 28 */
>
> Nit: There are now 3 too many padding spaces. Granted that's
> how it was done for XEN_SYSCTL_tmem_op ...
You said: like it was done with tmem. ;-)
>
> In any event, as before,
> Acked-by: Jan Beulich <jbeulich@suse.com>
Thanks,
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-14 14:56 ` Jürgen Groß
@ 2020-05-14 15:02 ` Jan Beulich
2020-05-14 15:43 ` Jürgen Groß
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-05-14 15:02 UTC (permalink / raw)
To: Jürgen Groß
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 14.05.2020 16:56, Jürgen Groß wrote:
> On 14.05.20 14:10, Jan Beulich wrote:
>> On 14.05.2020 13:48, Jürgen Groß wrote:
>>> On 14.05.20 12:20, Jan Beulich wrote:
>>>> On 08.05.2020 17:34, Juergen Gross wrote:
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -85,8 +85,43 @@ struct grant_table {
>>>>> struct grant_table_arch arch;
>>>>> };
>>>>> -static int parse_gnttab_limit(const char *param, const char *arg,
>>>>> - unsigned int *valp)
>>>>> +unsigned int __read_mostly opt_max_grant_frames = 64;
>>>>> +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
>>>>> +
>>>>> +#ifdef CONFIG_HYPFS
>>>>> +#define GRANT_CUSTOM_VAL_SZ 12
>>>>> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
>>>>> +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
>>>>> +
>>>>> +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
>>>>> + char *parval)
>>>>> +{
>>>>> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
>>>>> + custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
>>>>> +}
>>>>> +
>>>>> +static void __init gnttab_max_frames_init(struct param_hypfs *par)
>>>>> +{
>>>>> + update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
>>>>> +}
>>>>> +
>>>>> +static void __init max_maptrack_frames_init(struct param_hypfs *par)
>>>>> +{
>>>>> + update_gnttab_par(par, opt_max_maptrack_frames,
>>>>> + opt_max_maptrack_frames_val);
>>>>> +}
>>>>> +#else
>>>>> +#define opt_max_grant_frames_val NULL
>>>>> +#define opt_max_maptrack_frames_val NULL
>>>>
>>>> This looks latently dangerous to me (in case new uses of these
>>>> two identifiers appeared), but I guess my alternative suggestion
>>>> will be at best controversial, too:
>>>>
>>>> #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
>>>> #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, valp)
>>>>
>>>> (placed right here)
>>>
>>> Who else would use those identifiers not related to hypfs?
>>
>> I can't see an obvious possible use, but people get creative, i.e.
>> you never know. Passing NULL into a function without it being
>> blindingly obvious that it won't ever get (de)referenced is an at
>> least theoretical risk imo.
>
> Hmm, what about using a special type for those content variables?
> Something like:
>
> #ifdef CONFIG_HYPFS
> #define hypfs_string_var char *
> #else
> #define hypfs_string_var char
> #define opt_max_grant_frames_val 0
> #define opt_max_maptrack_frames_val 0
> #endif
>
> And then use that as type for function parameters? This should make
> dereferencing pretty hard.
>
> Other than that I have no really good idea how to avoid this problem.
IOW (as suspected) you don't like my suggestion? Personally I think
yours, having more #define-s, is at least not better than mine.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs
2020-05-14 15:02 ` Jan Beulich
@ 2020-05-14 15:43 ` Jürgen Groß
0 siblings, 0 replies; 31+ messages in thread
From: Jürgen Groß @ 2020-05-14 15:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Andrew Cooper, Ian Jackson, George Dunlap, Jun Nakajima,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 17:02, Jan Beulich wrote:
> On 14.05.2020 16:56, Jürgen Groß wrote:
>> On 14.05.20 14:10, Jan Beulich wrote:
>>> On 14.05.2020 13:48, Jürgen Groß wrote:
>>>> On 14.05.20 12:20, Jan Beulich wrote:
>>>>> On 08.05.2020 17:34, Juergen Gross wrote:
>>>>>> --- a/xen/common/grant_table.c
>>>>>> +++ b/xen/common/grant_table.c
>>>>>> @@ -85,8 +85,43 @@ struct grant_table {
>>>>>> struct grant_table_arch arch;
>>>>>> };
>>>>>> -static int parse_gnttab_limit(const char *param, const char *arg,
>>>>>> - unsigned int *valp)
>>>>>> +unsigned int __read_mostly opt_max_grant_frames = 64;
>>>>>> +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
>>>>>> +
>>>>>> +#ifdef CONFIG_HYPFS
>>>>>> +#define GRANT_CUSTOM_VAL_SZ 12
>>>>>> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
>>>>>> +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
>>>>>> +
>>>>>> +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
>>>>>> + char *parval)
>>>>>> +{
>>>>>> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
>>>>>> + custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
>>>>>> +}
>>>>>> +
>>>>>> +static void __init gnttab_max_frames_init(struct param_hypfs *par)
>>>>>> +{
>>>>>> + update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
>>>>>> +}
>>>>>> +
>>>>>> +static void __init max_maptrack_frames_init(struct param_hypfs *par)
>>>>>> +{
>>>>>> + update_gnttab_par(par, opt_max_maptrack_frames,
>>>>>> + opt_max_maptrack_frames_val);
>>>>>> +}
>>>>>> +#else
>>>>>> +#define opt_max_grant_frames_val NULL
>>>>>> +#define opt_max_maptrack_frames_val NULL
>>>>>
>>>>> This looks latently dangerous to me (in case new uses of these
>>>>> two identifiers appeared), but I guess my alternative suggestion
>>>>> will be at best controversial, too:
>>>>>
>>>>> #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
>>>>> #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, valp)
>>>>>
>>>>> (placed right here)
>>>>
>>>> Who else would use those identifiers not related to hypfs?
>>>
>>> I can't see an obvious possible use, but people get creative, i.e.
>>> you never know. Passing NULL into a function without it being
>>> blindingly obvious that it won't ever get (de)referenced is an at
>>> least theoretical risk imo.
>>
>> Hmm, what about using a special type for those content variables?
>> Something like:
>>
>> #ifdef CONFIG_HYPFS
>> #define hypfs_string_var char *
>> #else
>> #define hypfs_string_var char
>> #define opt_max_grant_frames_val 0
>> #define opt_max_maptrack_frames_val 0
>> #endif
>>
>> And then use that as type for function parameters? This should make
>> dereferencing pretty hard.
>>
>> Other than that I have no really good idea how to avoid this problem.
>
> IOW (as suspected) you don't like my suggestion? Personally I think
> yours, having more #define-s, is at least not better than mine.
Oh, maybe I misunderstood you here. I thought you weren't happy with
your solution either and suspected others (not me) wouldn't like it.
In case others don't reject it I'd be happy to use your variant.
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-14 9:50 ` Jürgen Groß
2020-05-14 11:58 ` Jan Beulich
@ 2020-05-15 5:33 ` Jürgen Groß
2020-05-15 9:27 ` Jan Beulich
1 sibling, 1 reply; 31+ messages in thread
From: Jürgen Groß @ 2020-05-15 5:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 14.05.20 11:50, Jürgen Groß wrote:
> On 14.05.20 09:59, Jan Beulich wrote:
>> On 08.05.2020 17:34, Juergen Gross wrote:
>>> +int hypfs_read_dir(const struct hypfs_entry *entry,
>>> + XEN_GUEST_HANDLE_PARAM(void) uaddr)
>>> +{
>>> + const struct hypfs_entry_dir *d;
>>> + const struct hypfs_entry *e;
>>> + unsigned int size = entry->size;
>>> +
>>> + d = container_of(entry, const struct hypfs_entry_dir, e);
>>> +
>>> + list_for_each_entry ( e, &d->dirlist, list )
>>
>> This function, in particular because of being non-static, makes
>> me wonder how, with add_entry() taking a lock, it can be safe
>> without any locking. Initially I thought the justification might
>> be because all adding of entries is an init-time-only thing, but
>> various involved functions aren't marked __init (and it is at
>> least not implausible that down the road we might see new
>> entries getting added during certain hotplug operations).
>>
>> I do realize that do_hypfs_op() takes the necessary read lock,
>> but then you're still building on the assumption that the
>> function is reachable through only that code path, despite
>> being non-static. An ASSERT() here would be the minimum I guess,
>> but with read locks now being recursive I don't see why you
>> couldn't read-lock here again.
>
> Right, will add the read-lock.
>
>>
>> The same goes for other non-static functions, albeit things may
>> become more interesting for functions living on the
>> XEN_HYPFS_OP_write_contents path (because write locks aren't
>
> Adding an ASSERT() in this regard should be rather easy.
As the type specific read- and write-functions should only be called
through the generic read/write functions I think it is better to have
a percpu variable holding the current locking state and ASSERT() that
to match. This will be cheaper than nesting locks and I don't have to
worry about either write_lock nesting or making _is_write_locked_by_me()
an official interface.
Juergen
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support
2020-05-15 5:33 ` Jürgen Groß
@ 2020-05-15 9:27 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-05-15 9:27 UTC (permalink / raw)
To: Jürgen Groß
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Ian Jackson, George Dunlap, xen-devel, Daniel De Graaf,
Volodymyr Babchuk, Roger Pau Monné
On 15.05.2020 07:33, Jürgen Groß wrote:
> On 14.05.20 11:50, Jürgen Groß wrote:
>> On 14.05.20 09:59, Jan Beulich wrote:
>>> On 08.05.2020 17:34, Juergen Gross wrote:
>>>> +int hypfs_read_dir(const struct hypfs_entry *entry,
>>>> + XEN_GUEST_HANDLE_PARAM(void) uaddr)
>>>> +{
>>>> + const struct hypfs_entry_dir *d;
>>>> + const struct hypfs_entry *e;
>>>> + unsigned int size = entry->size;
>>>> +
>>>> + d = container_of(entry, const struct hypfs_entry_dir, e);
>>>> +
>>>> + list_for_each_entry ( e, &d->dirlist, list )
>>>
>>> This function, in particular because of being non-static, makes
>>> me wonder how, with add_entry() taking a lock, it can be safe
>>> without any locking. Initially I thought the justification might
>>> be because all adding of entries is an init-time-only thing, but
>>> various involved functions aren't marked __init (and it is at
>>> least not implausible that down the road we might see new
>>> entries getting added during certain hotplug operations).
>>>
>>> I do realize that do_hypfs_op() takes the necessary read lock,
>>> but then you're still building on the assumption that the
>>> function is reachable through only that code path, despite
>>> being non-static. An ASSERT() here would be the minimum I guess,
>>> but with read locks now being recursive I don't see why you
>>> couldn't read-lock here again.
>>
>> Right, will add the read-lock.
>>
>>>
>>> The same goes for other non-static functions, albeit things may
>>> become more interesting for functions living on the
>>> XEN_HYPFS_OP_write_contents path (because write locks aren't
>>
>> Adding an ASSERT() in this regard should be rather easy.
>
> As the type specific read- and write-functions should only be called
> through the generic read/write functions I think it is better to have
> a percpu variable holding the current locking state and ASSERT() that
> to match. This will be cheaper than nesting locks and I don't have to
> worry about either write_lock nesting or making _is_write_locked_by_me()
> an official interface.
Ah yes, this should do.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-05-15 9:28 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 15:34 [PATCH v8 00/12] Add hypervisor sysfs-like support Juergen Gross
2020-05-08 15:34 ` [PATCH v8 01/12] xen/vmx: let opt_ept_ad always reflect the current setting Juergen Gross
2020-05-08 15:34 ` [PATCH v8 02/12] xen: add a generic way to include binary files as variables Juergen Gross
2020-05-08 15:34 ` [PATCH v8 03/12] docs: add feature document for Xen hypervisor sysfs-like support Juergen Gross
2020-05-08 15:34 ` [PATCH v8 04/12] xen: add basic hypervisor filesystem support Juergen Gross
2020-05-14 7:59 ` Jan Beulich
2020-05-14 9:50 ` Jürgen Groß
2020-05-14 11:58 ` Jan Beulich
2020-05-14 12:12 ` Jürgen Groß
2020-05-15 5:33 ` Jürgen Groß
2020-05-15 9:27 ` Jan Beulich
2020-05-14 9:55 ` Jan Beulich
2020-05-14 9:58 ` Jürgen Groß
2020-05-08 15:34 ` [PATCH v8 05/12] libs: add libxenhypfs Juergen Gross
2020-05-08 15:34 ` [PATCH v8 06/12] tools: add xenfs tool Juergen Gross
2020-05-08 15:34 ` [PATCH v8 07/12] xen: provide version information in hypfs Juergen Gross
2020-05-08 15:34 ` [PATCH v8 08/12] xen: add /buildinfo/config entry to hypervisor filesystem Juergen Gross
2020-05-14 9:32 ` Jan Beulich
2020-05-14 9:52 ` Jürgen Groß
2020-05-08 15:34 ` [PATCH v8 09/12] xen: add runtime parameter access support to hypfs Juergen Gross
2020-05-14 10:20 ` Jan Beulich
2020-05-14 11:48 ` Jürgen Groß
2020-05-14 12:10 ` Jan Beulich
2020-05-14 14:56 ` Jürgen Groß
2020-05-14 15:02 ` Jan Beulich
2020-05-14 15:43 ` Jürgen Groß
2020-05-08 15:34 ` [PATCH v8 10/12] tools/libxl: use libxenhypfs for setting xen runtime parameters Juergen Gross
2020-05-08 15:34 ` [PATCH v8 11/12] tools/libxc: remove xc_set_parameters() Juergen Gross
2020-05-08 15:34 ` [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support Juergen Gross
2020-05-14 13:03 ` Jan Beulich
2020-05-14 14:58 ` 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.