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
