Xen project Mailing List

Re: [Xen-devel] [PATCH v6 09/12] xen: add runtime parameter access support to hypfs

On 04.03.20 16:19, Jan Beulich wrote:

On 04.03.2020 16:07, Jürgen Groß wrote:

On 04.03.20 12:32, Jan Beulich wrote:

On 26.02.2020 13:47, Juergen Gross wrote:

+static void update_ept_param_append(const char *str, int val)
+{
+    char *pos = opt_ept_setting + strlen(opt_ept_setting);
+
+    snprintf(pos, sizeof(opt_ept_setting) - (pos - opt_ept_setting),
+             ",%s=%d", str, val);
+}
+
+static void update_ept_param(void)
+{
+    snprintf(opt_ept_setting, sizeof(opt_ept_setting), "pml=%d", opt_ept_pml);
+    if ( opt_ept_ad >= 0 )
+        update_ept_param_append("ad", opt_ept_ad);


This won't correctly reflect reality: If you look at
vmx_init_vmcs_config(), even a negative value means "true" here,
unless on a specific Atom model. I think init_ept_param() wants
to have that erratum workaround logic moved there, such that
you can then assme the value to be non-negative here.


But isn't not mentioning it in the -1 case correct? -1 means: do the
correct thing on the current hardware.


Well, I think the output here should represent effective settings,

The minimum requirement is to reflect the effective parameters, like cmdline is doing for boot-time only parameters. With runtime parameters we had no way of telling what was set, and this is now possible.

and a sub-item should be suppressed only if a setting has no effect
at all in the current setup, like ...

+    if ( opt_ept_exec_sp >= 0 )
+        update_ept_param_append("exec-sp", opt_ept_exec_sp);


I agree for this one - if the value is still -1, it has neither
been set nor is its value of any interest.


... here.

I think we should not mix up specified parameters and effective settings. In case an effective setting is of common interest it should be reported via a specific node (like e.g. specific mitigation settings where the cmdline is not providing enough details).

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -85,8 +85,10 @@ struct grant_table {
       struct grant_table_arch arch;
   };

-static int parse_gnttab_limit(const char *param, const char *arg,

-                              unsigned int *valp)
+#define GRANT_CUSTOM_VAL_SZ  12
+
+static int parse_gnttab_limit(const char *arg, unsigned int *valp,
+                              char *parval)
   {
       const char *e;
       unsigned long val;
@@ -99,28 +101,47 @@ static int parse_gnttab_limit(const char *param, const 
char *arg,
           return -ERANGE;

*valp = val;

+    snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%lu", val);

return 0;

unsigned int __read_mostly opt_max_grant_frames = 64;

+static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
+
+static void __init gnttab_max_frames_init(struct param_hypfs *par)
+{
+    custom_runtime_set_var(par, opt_max_grant_frames_val);


You still use a custom string buffer here. Can this "set-var"
operation record that the variable (for presentation purposes)
is simply of UINT type, handing a pointer to the actual
variable?


No, this would result in the need to set a custom parameter via a
binary value passed in from user land. So I'd need to convert this
binary into a string to be parseable by the custom function.


Hmm, not very fortunate, but I can see what you're saying.

--- 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,33 @@ 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) )


Once again " != buf + ulen - 1"? (EDOM also looks like an odd
error code to use in this case, but I guess there's no really
good one.)


" != buf + ulen - 1" is a logical choice with the change of patch 4.


I'm afraid I don't understand. You want to parse a string here.
The caller should tell you what the string length is (including
the nul again), not what its buffer size may be.

I agreed that changing to " != buf + ulen - 1" makes sense as I agreed already to do so in patch 4.

@@ -79,41 +88,94 @@ extern const struct kernel_param __param_start[], 
__param_end[];
             .type = OPT_IGNORE }

#define __rtparam __param(__dataparam)

+#define __paramfs         static __paramhypfs \
+    __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs

-#define custom_runtime_only_param(_name, _var) \

+#define custom_runtime_set_var(parfs, var) \
+    { \
+        (parfs)->hypfs.write_ptr = &(var); \
+        (parfs)->hypfs.e.size = sizeof(var); \


All users of this use char[]. Why sizeof() rather than strlen(),


That is the maximum string length. Otherwise I wouldn't know I am
allowed to replace e.g. "on" by "noxpti".


As said elsewhere - if e.size is the buffer size, then the
reading function wants adjusting, and it needs to be clarified
how buffer size and payload size can be told apart for BLOBs.

Okay, I'll adjust the reading size to copy only strlen() + 1 bytes and add a comment that BLOBs need blob-specific write and read functions in the common case. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.