[PATCH v2 0/4] Support for reading hypervisor parameters at runtime

[PATCH v2 0/4] Support for reading hypervisor parameters at runtime

[Vasilis Liaskovitis]

Currently parameters of the hypervisor cannot be inspected at runtime
through an xl command. Such a command would be a useful diagnostic
tool e.g. used in conjunction with the "xl set-parameters" command.

This patch series implements a new xl command "xl get-parameters"
which takes a string of input parameters and returns their current
values in the hypervisor settings.

Changes v1->v2:

- fixed snprintf issues, fixed memory leaks and error handling
- removed unnecessary wrapper function
- OPT_SIZE is handled

Limitations:

- Custom runtime parameters (OPT_CUSTOM) are not supported yet. I'd like
  to do this in a follow-up series. See also discussion at
  https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg01383.html
- For integer parameters (OPT_UINT), only unsigned parameters are printed
  correctly at the moment.

Examples:

xl get-parameters "gnttab_max_frames gnttab_max_maptrack_frames"
gnttab_max_frames gnttab_max_maptrack_frames : 64 1024

xl set-parameters gnttab_max_frames=128

xl get-parameters gnttab_max_frames
gnttab_max_frames : 128

xl get-parameters "gnttab_max_frames gnttab_max_maptrack_frames"
gnttab_max_frames gnttab_max_maptrack_frames : 128 1024

Vasilis Liaskovitis (4):
  xen: add hypercall for getting parameters at runtime
  libxc: add function to get hypervisor parameters
  libxl: add libxl_get_parameters() function
  xl: add new xl command get-parameters

 docs/man/xl.1.pod.in                |   5 ++
 tools/flask/policy/modules/dom0.te  |   2 +-
 tools/libxc/include/xenctrl.h       |   1 +
 tools/libxc/xc_misc.c               |  26 +++++++
 tools/libxl/libxl.c                 |  15 ++++
 tools/libxl/libxl.h                 |   1 +
 tools/xl/xl.h                       |   1 +
 tools/xl/xl_cmdtable.c              |   5 ++
 tools/xl/xl_misc.c                  |  25 +++++++
 xen/common/kernel.c                 | 102 ++++++++++++++++++++++++++++
 xen/common/sysctl.c                 |  55 ++++++++++++++-
 xen/include/public/sysctl.h         |  18 +++++
 xen/include/xen/lib.h               |   1 +
 xen/xsm/flask/hooks.c               |   3 +
 xen/xsm/flask/policy/access_vectors |   2 +
 15 files changed, 259 insertions(+), 3 deletions(-)

-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Vasilis Liaskovitis]

Add a sysctl hypercall to support getting hypervisor parameters
at runtime.

Limitations:
- Custom runtime parameters (OPT_CUSTOM) are not supported yet.
- For integer parameters (OPT_UINT), only unsigned parameters are printed
correctly.

Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
---
 tools/flask/policy/modules/dom0.te  |   2 +-
 xen/common/kernel.c                 | 110 ++++++++++++++++++++++++++++
 xen/common/sysctl.c                 |  55 +++++++++++++-
 xen/include/public/sysctl.h         |  18 +++++
 xen/include/xen/lib.h               |   1 +
 xen/xsm/flask/hooks.c               |   3 +
 xen/xsm/flask/policy/access_vectors |   2 +
 7 files changed, 188 insertions(+), 3 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index a347d664f8..681d1a101b 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 set_parameter get_parameter
 };
 
 # Allow dom0 to use all XENVER_ subops that have checks.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 612575430f..2d12a5bcf6 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -12,6 +12,7 @@
 #include <xen/paging.h>
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
+#include <xen/ctype.h>
 #include <xsm/xsm.h>
 #include <asm/current.h>
 #include <public/version.h>
@@ -52,6 +53,115 @@ static int assign_integer_param(const struct kernel_param *param, uint64_t val)
     return 0;
 }
 
+static int get_integer_param(const struct kernel_param *param, uint64_t *val)
+{
+    switch ( param->len )
+    {
+    case sizeof(uint8_t):
+        *val = *(uint8_t *)param->par.var;
+        break;
+    case sizeof(uint16_t):
+        *val = *(uint16_t *)param->par.var;
+        break;
+    case sizeof(uint32_t):
+        *val = *(uint32_t *)param->par.var;
+        break;
+    case sizeof(uint64_t):
+        *val = *(uint64_t *)param->par.var;
+        break;
+    default:
+        BUG();
+    }
+
+    return 0;
+}
+
+int runtime_get_params(const char *cmdline, char *values,
+                      size_t maxlen)
+{
+    char opt[128], *optkey, *q, *val = values;
+    const char *p = cmdline;
+    const struct kernel_param *param;
+    int rc = 0, len = 0;
+    size_t bufpos = 0;
+    uint64_t param_int;
+
+    if (!values)
+        return -EFAULT;
+
+    for ( ; ; )
+    {
+        /* Skip whitespace. */
+        while ( isspace(*p) )
+            p++;
+        if ( *p == '\0' )
+            break;
+
+        /* Grab the next whitespace-delimited option. */
+        q = optkey = opt;
+        while ( (*p != ' ') && (*p != '\0') )
+        {
+            if ( (q - opt) < (sizeof(opt)-1) ) /* avoid overflow */
+                *q++ = *p;
+            p++;
+        }
+        *q = '\0';
+
+        for ( param = __param_start; param < __param_end; param++ )
+        {
+            if ( strcmp(param->name, optkey) )
+                continue;
+
+            switch ( param->type )
+            {
+            case OPT_STR:
+                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
+                               (char*)param->par.var);
+                break;
+            case OPT_UINT:
+            case OPT_SIZE:
+                get_integer_param(param, &param_int);
+                len = snprintf(val + bufpos, maxlen - bufpos,
+                               "%"PRIu64" ", param_int);
+                break;
+            case OPT_BOOL:
+                get_integer_param(param, &param_int);
+                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
+                               param_int ? "true" : "false");
+                break;
+            case OPT_CUSTOM:
+                /* Custom parameters are not supported yet. */
+                rc = -EINVAL;
+                break;
+            default:
+                BUG();
+                break;
+            }
+
+            if (len < 0)
+                rc = len;
+            else if (len < maxlen - bufpos)
+            /* if output was not truncated update buffer position */
+                bufpos += (size_t) len;
+            else if (len > 0)
+                rc = -ENOMEM;
+
+            break;
+        }
+
+        /* no parameter was matched */
+        if ( param >= __param_end )
+        {
+            rc = -EINVAL;
+        }
+
+        if (rc)
+            break;
+    }
+
+    return rc;
+}
+
 static int parse_params(const char *cmdline, const struct kernel_param *start,
                         const struct kernel_param *end)
 {
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c0aa6bde4e..1b65bd617e 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -466,9 +466,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
             copyback = 1;
         break;
 
+#define XEN_PARAMETER_MAX_SIZE 1023
     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] ||
@@ -477,7 +477,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
             ret = -EINVAL;
             break;
         }
-        if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
+        if ( op->u.set_parameter.size > XEN_PARAMETER_MAX_SIZE )
         {
             ret = -E2BIG;
             break;
@@ -501,6 +501,57 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 
         break;
     }
+    case XEN_SYSCTL_get_parameter:
+    {
+        char *params;
+        char *values;
+
+        if ( op->u.get_parameter.pad[0] || op->u.get_parameter.pad[1] ||
+             op->u.get_parameter.pad[2] )
+        {
+            ret = -EINVAL;
+            break;
+        }
+        if ( op->u.get_parameter.size > XEN_PARAMETER_MAX_SIZE )
+        {
+            ret = -E2BIG;
+            break;
+        }
+        params = xmalloc_bytes(op->u.get_parameter.size + 1);
+        if ( !params )
+        {
+            ret = -ENOMEM;
+            break;
+        }
+
+        values = xmalloc_bytes(XEN_PARAMETER_MAX_SIZE);
+        if ( !values )
+        {
+            xfree(params);
+            ret = -ENOMEM;
+            break;
+        }
+
+        else if ( copy_from_guest(params, op->u.get_parameter.params,
+                             op->u.get_parameter.size) )
+            ret = -EFAULT;
+        else
+        {
+            params[op->u.set_parameter.size] = 0;
+            ret = runtime_get_params(params, values, XEN_PARAMETER_MAX_SIZE);
+
+            if ( !ret && copy_to_guest(op->u.get_parameter.values, values,
+                               strlen(values)) )
+            {
+                ret = -EFAULT;
+            }
+        }
+
+        xfree(params);
+        xfree(values);
+
+        break;
+    }
 
     default:
         ret = arch_do_sysctl(op, u_sysctl);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index c49b4dcc99..7d77d57115 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1100,6 +1100,22 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
 #endif
 
+/*
+ * XEN_SYSCTL_get_parameter
+ *
+ * Read hypervisor parameters at runtime.
+ * 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_get_parameter {
+    XEN_GUEST_HANDLE_64(char) params;       /* IN: pointer to parameters. */
+    XEN_GUEST_HANDLE_64(char) values;       /* OUT: pointer to output values. */
+    uint16_t size;                          /* IN: size of parameters. */
+    uint16_t pad[3];                        /* IN: MUST be zero. */
+};
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -1130,6 +1146,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_livepatch_op                  27
 #define XEN_SYSCTL_set_parameter                 28
 #define XEN_SYSCTL_get_cpu_policy                29
+#define XEN_SYSCTL_get_parameter                 30
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -1162,6 +1179,7 @@ struct xen_sysctl {
 #if defined(__i386__) || defined(__x86_64__)
         struct xen_sysctl_cpu_policy        cpu_policy;
 #endif
+        struct xen_sysctl_get_parameter     get_parameter;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 89939f43c8..96440a6041 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -71,6 +71,7 @@ struct domain;
 void cmdline_parse(const char *cmdline);
 int runtime_parse(const char *line);
 int parse_bool(const char *s, const char *e);
+int runtime_get_params(const char *cmdline, char *values, size_t maxlen);
 
 /**
  * Given a specific name, parses a string of the form:
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3d00c747f6..1b832e9a4c 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -830,6 +830,9 @@ static int flask_sysctl(int cmd)
     case XEN_SYSCTL_set_parameter:
         return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
                                     XEN2__SET_PARAMETER, NULL);
+    case XEN_SYSCTL_get_parameter:
+        return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
+                                    XEN2__GET_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 e00448b776..c5ee21d852 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -103,6 +103,8 @@ class xen2
     coverage_op
 # XEN_SYSCTL_set_parameter
     set_parameter
+# XEN_SYSCTL_get_parameter
+    get_parameter
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/4] libxc: add function to get hypervisor parameters

[Vasilis Liaskovitis]

Add a new libxc function to get hypervisor parameters at runtime.

Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
---
 tools/libxc/include/xenctrl.h |  1 +
 tools/libxc/xc_misc.c         | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index a3628e56bb..3482ca1a91 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1228,6 +1228,7 @@ int xc_readconsolering(xc_interface *xch,
 
 int xc_send_debug_keys(xc_interface *xch, char *keys);
 int xc_set_parameters(xc_interface *xch, char *params);
+int xc_get_parameters(xc_interface *xch, char *params, char *values);
 
 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 5e6714ae2b..439ad91194 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -208,6 +208,32 @@ int xc_set_parameters(xc_interface *xch, char *params)
     return ret;
 }
 
+int xc_get_parameters(xc_interface *xch, char *params, char *values)
+{
+    int ret, len = strlen(params);
+    DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BOUNCE(params, len, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(values, 1023, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    if ( xc_hypercall_bounce_pre(xch, params) )
+        return -1;
+    if ( xc_hypercall_bounce_pre(xch, values) )
+        return -1;
+
+    sysctl.cmd = XEN_SYSCTL_get_parameter;
+    set_xen_guest_handle(sysctl.u.get_parameter.params, params);
+    set_xen_guest_handle(sysctl.u.get_parameter.values, values);
+    sysctl.u.get_parameter.size = len;
+    memset(sysctl.u.get_parameter.pad, 0, sizeof(sysctl.u.get_parameter.pad));
+
+    ret = do_sysctl(xch, &sysctl);
+
+    xc_hypercall_bounce_post(xch, params);
+    xc_hypercall_bounce_post(xch, values);
+
+    return ret;
+}
+
 int xc_physinfo(xc_interface *xch,
                 xc_physinfo_t *put_info)
 {
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/4] libxl: add libxl_get_parameters() function

[Vasilis Liaskovitis]

Add a new libxl function to get hypervisor parameters at runtime.

Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
---
 tools/libxl/libxl.c | 15 +++++++++++++++
 tools/libxl/libxl.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ec71574e99..124033e5a3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -669,6 +669,21 @@ int libxl_set_parameters(libxl_ctx *ctx, char *params)
     return 0;
 }
 
+int libxl_get_parameters(libxl_ctx *ctx, char *params, char *values)
+{
+    int ret;
+    GC_INIT(ctx);
+
+    ret = xc_get_parameters(ctx->xch, params, values);
+    if (ret < 0) {
+        LOGEV(ERROR, ret, "getting parameters");
+        GC_FREE;
+        return ret;//ERROR_FAIL;
+    }
+    GC_FREE;
+    return 0;
+}
+
 static int fd_set_flags(libxl_ctx *ctx, int fd,
                         int fcntlgetop, int fcntlsetop, const char *fl,
                         int flagmask, int set_p)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index a38e5cdba2..360a757a06 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2307,6 +2307,7 @@ int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
 int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq);
 int libxl_send_debug_keys(libxl_ctx *ctx, char *keys);
 int libxl_set_parameters(libxl_ctx *ctx, char *params);
+int libxl_get_parameters(libxl_ctx *ctx, char *params, char *values);
 
 typedef struct libxl__xen_console_reader libxl_xen_console_reader;
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 4/4] xl: add new xl command get-parameters

[Vasilis Liaskovitis]

Add a new xl command "get-parameters" to get hypervisor parameters at
runtime.

Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
---
 docs/man/xl.1.pod.in   |  5 +++++
 tools/xl/xl.h          |  1 +
 tools/xl/xl_cmdtable.c |  5 +++++
 tools/xl/xl_misc.c     | 25 +++++++++++++++++++++++++
 4 files changed, 36 insertions(+)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 4310fcd818..a1fff4d382 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -827,6 +827,11 @@ Send debug I<keys> to Xen. It is the same as pressing the Xen
 Set hypervisor parameters as specified in I<params>. This allows for some
 boot parameters of the hypervisor to be modified in the running systems.
 
+=item B<get-parameters> I<params>
+
+Get hypervisor parameters as specified in I<params>. This allows for some
+boot parameters of the hypervisor to be read in the running systems.
+
 =item B<dmesg> [I<OPTIONS>]
 
 Reads the Xen message buffer, similar to dmesg on a Linux system.  The
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index cf4202bc89..af3843e5b0 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -219,6 +219,7 @@ int main_psr_mba_set(int argc, char **argv);
 int main_psr_mba_show(int argc, char **argv);
 #endif
 int main_qemu_monitor_command(int argc, char **argv);
+int main_get_parameters(int argc, char **argv);
 
 void help(const char *command);
 
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 89716badcb..a18481619b 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -662,6 +662,11 @@ struct cmd_spec cmd_table[] = {
       "Issue a qemu monitor command to the device model of a domain",
       "<Domain> <Command>",
     },
+    { "get-parameters",
+      &main_get_parameters, 0, 1,
+      "Get hypervisor parameters",
+      "<Params>",
+    },
 };
 
 int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
diff --git a/tools/xl/xl_misc.c b/tools/xl/xl_misc.c
index dcf940a6d4..811f231b78 100644
--- a/tools/xl/xl_misc.c
+++ b/tools/xl/xl_misc.c
@@ -364,6 +364,31 @@ int main_config_update(int argc, char **argv)
     return 0;
 }
 
+int main_get_parameters(int argc, char **argv)
+{
+    int opt, ret;
+    char *params;
+    char values[1023];
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "get-parameters", 1) {
+        /* No options */
+    }
+
+    params = argv[optind];
+
+    if (!params) {
+	fprintf(stderr, "no parameter specified\n");
+	return EXIT_FAILURE;
+    }
+    else if ((ret = libxl_get_parameters(ctx, params, values))) {
+        fprintf(stderr, "cannot get parameters: %s : %d\n", params, ret);
+        fprintf(stderr, "Use \"xl dmesg\" to look for possible reason.\n");
+        return EXIT_FAILURE;
+    }
+    fprintf(stderr, "%s : %s\n", params, values);
+
+    return EXIT_SUCCESS;
+}
 /*
  * Local variables:
  * mode: C
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Jan Beulich]

>>> On 22.03.19 at 20:28, <vliaskovitis@suse.com> wrote:
> Limitations:
> - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
> - For integer parameters (OPT_UINT), only unsigned parameters are printed
> correctly.

For this latter case I wonder whether it wouldn't be better to
return back the raw binary value, allowing the caller to format
it in suitable ways (e.g. both signed and unsigned, or dec and
hex).

> +int runtime_get_params(const char *cmdline, char *values,

I don't think "cmdline" is the correct term here ("opts" would
be a possible better name), but then again this keeps it
nicely in line with the actual cmdline parsing function.

> +                      size_t maxlen)
> +{
> +    char opt[128], *optkey, *q, *val = values;
> +    const char *p = cmdline;
> +    const struct kernel_param *param;
> +    int rc = 0, len = 0;
> +    size_t bufpos = 0;
> +    uint64_t param_int;
> +
> +    if (!values)

Style (missing blanks). But the question is whether this conditional
is useful at all. The only caller passes a non-NULL value anyway.
Otherwise, why wouldn't you check cmdline as well?

> +        return -EFAULT;
> +
> +    for ( ; ; )
> +    {
> +        /* Skip whitespace. */
> +        while ( isspace(*p) )
> +            p++;
> +        if ( *p == '\0' )
> +            break;
> +
> +        /* Grab the next whitespace-delimited option. */
> +        q = optkey = opt;
> +        while ( (*p != ' ') && (*p != '\0') )

You've used isspace() above - why not here?

> +        {
> +            if ( (q - opt) < (sizeof(opt)-1) ) /* avoid overflow */

Missing blanks (around binary operator).

I also think that you should return an error upon buffer
overflow, instead of potentially returning the wrong option's
value.

> +                *q++ = *p;
> +            p++;
> +        }
> +        *q = '\0';
> +
> +        for ( param = __param_start; param < __param_end; param++ )
> +        {
> +            if ( strcmp(param->name, optkey) )
> +                continue;
> +
> +            switch ( param->type )
> +            {
> +            case OPT_STR:
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               (char*)param->par.var);

What you return here is the value as set into the buffer when the
option was parsed, and before that string was actually consumed.
Is this really what's interesting to the user? I'd rather expect them
to be after what is actually in effect.

Since we've decided against a "get" per-option hook, I consider it
acceptable this way as long as the behavior is spelled out amongst
the limitations.

> +                break;
> +            case OPT_UINT:

I think (hope) I've said so on v1 already: Please have blank lines
between individual case blocks.

> +            case OPT_SIZE:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos,
> +                               "%"PRIu64" ", param_int);
> +                break;
> +            case OPT_BOOL:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               param_int ? "true" : "false");
> +                break;
> +            case OPT_CUSTOM:
> +                /* Custom parameters are not supported yet. */
> +                rc = -EINVAL;
> +                break;
> +            default:
> +                BUG();
> +                break;

Please be consistent as to whether you add "break;" in a default
case positioned last in a switch(). Personally I'd prefer them to be
there, but I won't insist. All I'd like to see is no mixed style within
a single patch at least.

> +            }
> +
> +            if (len < 0)
> +                rc = len;
> +            else if (len < maxlen - bufpos)
> +            /* if output was not truncated update buffer position */
> +                bufpos += (size_t) len;
> +            else if (len > 0)
> +                rc = -ENOMEM;

Various style issues here: Missing blanks inside if(), malformed and
improperly indented comment. I also don't see the need for the
cast, and if it was needed there would be a stray blank there.

> +            break;
> +        }
> +
> +        /* no parameter was matched */
> +        if ( param >= __param_end )
> +        {
> +            rc = -EINVAL;
> +        }

Stray braces.

> +        if (rc)
> +            break;

Missing blanks again. But perhaps better make the loop
"while ( !rc )" anyway, or "do { ... } while ( !rc );".

> @@ -501,6 +501,57 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  
>          break;
>      }
> +    case XEN_SYSCTL_get_parameter:

As per above, blank line above here please.

> +    {
> +        char *params;
> +        char *values;

Put both on the same line?

> +        if ( op->u.get_parameter.pad[0] || op->u.get_parameter.pad[1] ||
> +             op->u.get_parameter.pad[2] )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +        if ( op->u.get_parameter.size > XEN_PARAMETER_MAX_SIZE )
> +        {
> +            ret = -E2BIG;
> +            break;
> +        }
> +        params = xmalloc_bytes(op->u.get_parameter.size + 1);
> +        if ( !params )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        values = xmalloc_bytes(XEN_PARAMETER_MAX_SIZE);
> +        if ( !values )
> +        {
> +            xfree(params);
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        else if ( copy_from_guest(params, op->u.get_parameter.params,
> +                             op->u.get_parameter.size) )

The "else" is pointless here with the "break;" above, but if you
were to keep it, then there should be no blank line ahead of it.
The second line is also insufficiently indented.

> +            ret = -EFAULT;
> +        else
> +        {
> +            params[op->u.set_parameter.size] = 0;
> +            ret = runtime_get_params(params, values, XEN_PARAMETER_MAX_SIZE);
> +
> +            if ( !ret && copy_to_guest(op->u.get_parameter.values, values,
> +                               strlen(values)) )

Indentation again.

> +            {
> +                ret = -EFAULT;
> +            }

Stray braces again.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Jan Beulich]

>>> On 22.03.19 at 20:28, <vliaskovitis@suse.com> wrote:
> Limitations:
> - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
> - For integer parameters (OPT_UINT), only unsigned parameters are printed
> correctly.

For this latter case I wonder whether it wouldn't be better to
return back the raw binary value, allowing the caller to format
it in suitable ways (e.g. both signed and unsigned, or dec and
hex).

> +int runtime_get_params(const char *cmdline, char *values,

I don't think "cmdline" is the correct term here ("opts" would
be a possible better name), but then again this keeps it
nicely in line with the actual cmdline parsing function.

> +                      size_t maxlen)
> +{
> +    char opt[128], *optkey, *q, *val = values;
> +    const char *p = cmdline;
> +    const struct kernel_param *param;
> +    int rc = 0, len = 0;
> +    size_t bufpos = 0;
> +    uint64_t param_int;
> +
> +    if (!values)

Style (missing blanks). But the question is whether this conditional
is useful at all. The only caller passes a non-NULL value anyway.
Otherwise, why wouldn't you check cmdline as well?

> +        return -EFAULT;
> +
> +    for ( ; ; )
> +    {
> +        /* Skip whitespace. */
> +        while ( isspace(*p) )
> +            p++;
> +        if ( *p == '\0' )
> +            break;
> +
> +        /* Grab the next whitespace-delimited option. */
> +        q = optkey = opt;
> +        while ( (*p != ' ') && (*p != '\0') )

You've used isspace() above - why not here?

> +        {
> +            if ( (q - opt) < (sizeof(opt)-1) ) /* avoid overflow */

Missing blanks (around binary operator).

I also think that you should return an error upon buffer
overflow, instead of potentially returning the wrong option's
value.

> +                *q++ = *p;
> +            p++;
> +        }
> +        *q = '\0';
> +
> +        for ( param = __param_start; param < __param_end; param++ )
> +        {
> +            if ( strcmp(param->name, optkey) )
> +                continue;
> +
> +            switch ( param->type )
> +            {
> +            case OPT_STR:
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               (char*)param->par.var);

What you return here is the value as set into the buffer when the
option was parsed, and before that string was actually consumed.
Is this really what's interesting to the user? I'd rather expect them
to be after what is actually in effect.

Since we've decided against a "get" per-option hook, I consider it
acceptable this way as long as the behavior is spelled out amongst
the limitations.

> +                break;
> +            case OPT_UINT:

I think (hope) I've said so on v1 already: Please have blank lines
between individual case blocks.

> +            case OPT_SIZE:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos,
> +                               "%"PRIu64" ", param_int);
> +                break;
> +            case OPT_BOOL:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               param_int ? "true" : "false");
> +                break;
> +            case OPT_CUSTOM:
> +                /* Custom parameters are not supported yet. */
> +                rc = -EINVAL;
> +                break;
> +            default:
> +                BUG();
> +                break;

Please be consistent as to whether you add "break;" in a default
case positioned last in a switch(). Personally I'd prefer them to be
there, but I won't insist. All I'd like to see is no mixed style within
a single patch at least.

> +            }
> +
> +            if (len < 0)
> +                rc = len;
> +            else if (len < maxlen - bufpos)
> +            /* if output was not truncated update buffer position */
> +                bufpos += (size_t) len;
> +            else if (len > 0)
> +                rc = -ENOMEM;

Various style issues here: Missing blanks inside if(), malformed and
improperly indented comment. I also don't see the need for the
cast, and if it was needed there would be a stray blank there.

> +            break;
> +        }
> +
> +        /* no parameter was matched */
> +        if ( param >= __param_end )
> +        {
> +            rc = -EINVAL;
> +        }

Stray braces.

> +        if (rc)
> +            break;

Missing blanks again. But perhaps better make the loop
"while ( !rc )" anyway, or "do { ... } while ( !rc );".

> @@ -501,6 +501,57 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  
>          break;
>      }
> +    case XEN_SYSCTL_get_parameter:

As per above, blank line above here please.

> +    {
> +        char *params;
> +        char *values;

Put both on the same line?

> +        if ( op->u.get_parameter.pad[0] || op->u.get_parameter.pad[1] ||
> +             op->u.get_parameter.pad[2] )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +        if ( op->u.get_parameter.size > XEN_PARAMETER_MAX_SIZE )
> +        {
> +            ret = -E2BIG;
> +            break;
> +        }
> +        params = xmalloc_bytes(op->u.get_parameter.size + 1);
> +        if ( !params )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        values = xmalloc_bytes(XEN_PARAMETER_MAX_SIZE);
> +        if ( !values )
> +        {
> +            xfree(params);
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        else if ( copy_from_guest(params, op->u.get_parameter.params,
> +                             op->u.get_parameter.size) )

The "else" is pointless here with the "break;" above, but if you
were to keep it, then there should be no blank line ahead of it.
The second line is also insufficiently indented.

> +            ret = -EFAULT;
> +        else
> +        {
> +            params[op->u.set_parameter.size] = 0;
> +            ret = runtime_get_params(params, values, XEN_PARAMETER_MAX_SIZE);
> +
> +            if ( !ret && copy_to_guest(op->u.get_parameter.values, values,
> +                               strlen(values)) )

Indentation again.

> +            {
> +                ret = -EFAULT;
> +            }

Stray braces again.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Vasilis Liaskovitis]

Sorry for the delay, I was on a long vacation.

On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at
20:28, <vliaskovitis@suse.com> wrote:
> > Limitations:
> > - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
> > - For integer parameters (OPT_UINT), only unsigned parameters are
> > printed
> > correctly.
> 
> For this latter case I wonder whether it wouldn't be better to
> return back the raw binary value, allowing the caller to format
> it in suitable ways (e.g. both signed and unsigned, or dec and
> hex).

Currently the caller is oblivious to the parameters and their types,
and all retrieved values are printed together in a single string (see
patch 4/4). I'd like to keep it like that for simplicity. Otherwise I
think the caller has to find the types of requested parameters to
produce the right formatting. Actually, since OPT_UINT is used for both
signed and unsigned integer parameters, case-by-case parameter
formatting would be required to do this, and the libx* callers do not
have that knowledge. A "get_" per-parameter function pointer, which
would handle formatting for each parameter, be it int, uint, string or
custom, seems cleaner to me than leaving it to the caller.

By the way, the current implementation searches in [__param_start
__param_end), which are only the runtime parameters, not all
parameters, correct? So the series should be renamed to "Support for
reading runtime-only hypervisor parameters". The command is useful for
checking parameters that can be changed at runtime after all. 

I believe there are currently no signed integer runtime parameters. So
alternatively we could add a warning to the commit message and/or to
the code stating that signed integer runtime parameters, if added,
would not be printed correctly at the moment. This would gloss over
rather than solve the issue.

If correct formatting for all possible types is a requirement, the
get_function may be needed. Or we could separate integer from unsigned
integer parameters with an additional OPT_INT parameter type. I don't
know if either of these is desirable or simply overkill to implement
just for this command.

I 'll send a v3 when there is agreement on the above.

[...]

> > +            {
> > +            case OPT_STR:
> > +                len = snprintf(val + bufpos, maxlen - bufpos, "%s
> > ",
> > +                               (char*)param->par.var);
> 
> What you return here is the value as set into the buffer when the
> option was parsed, and before that string was actually consumed.
> Is this really what's interesting to the user? I'd rather expect them
> to be after what is actually in effect.
> 
> Since we've decided against a "get" per-option hook, I consider it
> acceptable this way as long as the behavior is spelled out amongst
> the limitations.
> 

I 'll add this limitation to the commit message in the next version.

thanks,
- Vasilis


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Vasilis Liaskovitis]

Sorry for the delay, I was on a long vacation.

On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at
20:28, <vliaskovitis@suse.com> wrote:
> > Limitations:
> > - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
> > - For integer parameters (OPT_UINT), only unsigned parameters are
> > printed
> > correctly.
> 
> For this latter case I wonder whether it wouldn't be better to
> return back the raw binary value, allowing the caller to format
> it in suitable ways (e.g. both signed and unsigned, or dec and
> hex).

Currently the caller is oblivious to the parameters and their types,
and all retrieved values are printed together in a single string (see
patch 4/4). I'd like to keep it like that for simplicity. Otherwise I
think the caller has to find the types of requested parameters to
produce the right formatting. Actually, since OPT_UINT is used for both
signed and unsigned integer parameters, case-by-case parameter
formatting would be required to do this, and the libx* callers do not
have that knowledge. A "get_" per-parameter function pointer, which
would handle formatting for each parameter, be it int, uint, string or
custom, seems cleaner to me than leaving it to the caller.

By the way, the current implementation searches in [__param_start
__param_end), which are only the runtime parameters, not all
parameters, correct? So the series should be renamed to "Support for
reading runtime-only hypervisor parameters". The command is useful for
checking parameters that can be changed at runtime after all. 

I believe there are currently no signed integer runtime parameters. So
alternatively we could add a warning to the commit message and/or to
the code stating that signed integer runtime parameters, if added,
would not be printed correctly at the moment. This would gloss over
rather than solve the issue.

If correct formatting for all possible types is a requirement, the
get_function may be needed. Or we could separate integer from unsigned
integer parameters with an additional OPT_INT parameter type. I don't
know if either of these is desirable or simply overkill to implement
just for this command.

I 'll send a v3 when there is agreement on the above.

[...]

> > +            {
> > +            case OPT_STR:
> > +                len = snprintf(val + bufpos, maxlen - bufpos, "%s
> > ",
> > +                               (char*)param->par.var);
> 
> What you return here is the value as set into the buffer when the
> option was parsed, and before that string was actually consumed.
> Is this really what's interesting to the user? I'd rather expect them
> to be after what is actually in effect.
> 
> Since we've decided against a "get" per-option hook, I consider it
> acceptable this way as long as the behavior is spelled out amongst
> the limitations.
> 

I 'll add this limitation to the commit message in the next version.

thanks,
- Vasilis


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Jan Beulich]

>>> On 30.04.19 at 21:05, <vliaskovitis@suse.com> wrote:
> On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at
> 20:28, <vliaskovitis@suse.com> wrote:
>> > Limitations:
>> > - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
>> > - For integer parameters (OPT_UINT), only unsigned parameters are
>> > printed
>> > correctly.
>> 
>> For this latter case I wonder whether it wouldn't be better to
>> return back the raw binary value, allowing the caller to format
>> it in suitable ways (e.g. both signed and unsigned, or dec and
>> hex).
> 
> Currently the caller is oblivious to the parameters and their types,
> and all retrieved values are printed together in a single string (see
> patch 4/4). I'd like to keep it like that for simplicity. Otherwise I
> think the caller has to find the types of requested parameters to
> produce the right formatting. Actually, since OPT_UINT is used for both
> signed and unsigned integer parameters, case-by-case parameter
> formatting would be required to do this, and the libx* callers do not
> have that knowledge. A "get_" per-parameter function pointer, which
> would handle formatting for each parameter, be it int, uint, string or
> custom, seems cleaner to me than leaving it to the caller.

I disagree. The caller requires no knowledge of the actual format.
It could easily format the string twice (once assuming it might be a
signed quantity, and another time assuming it might be an unsigned
one). All it would need to know is the width of the binary
representation.

> By the way, the current implementation searches in [__param_start
> __param_end), which are only the runtime parameters, not all
> parameters, correct? So the series should be renamed to "Support for
> reading runtime-only hypervisor parameters". The command is useful for
> checking parameters that can be changed at runtime after all. 

Yes, such renaming would seem desirable.

> I believe there are currently no signed integer runtime parameters. So
> alternatively we could add a warning to the commit message and/or to
> the code stating that signed integer runtime parameters, if added,
> would not be printed correctly at the moment. This would gloss over
> rather than solve the issue.

That's an option. As for other aspects, I don't heavily mind cases
not getting dealt with right away which have no practical use right
now, as long as the restrictions are clearly spelled out, such that
no-one will be surprised when trying to add a runtime option beyond
what the code is able to deal with.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] xen: add hypercall for getting parameters at runtime

[Jan Beulich]

>>> On 30.04.19 at 21:05, <vliaskovitis@suse.com> wrote:
> On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at
> 20:28, <vliaskovitis@suse.com> wrote:
>> > Limitations:
>> > - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
>> > - For integer parameters (OPT_UINT), only unsigned parameters are
>> > printed
>> > correctly.
>> 
>> For this latter case I wonder whether it wouldn't be better to
>> return back the raw binary value, allowing the caller to format
>> it in suitable ways (e.g. both signed and unsigned, or dec and
>> hex).
> 
> Currently the caller is oblivious to the parameters and their types,
> and all retrieved values are printed together in a single string (see
> patch 4/4). I'd like to keep it like that for simplicity. Otherwise I
> think the caller has to find the types of requested parameters to
> produce the right formatting. Actually, since OPT_UINT is used for both
> signed and unsigned integer parameters, case-by-case parameter
> formatting would be required to do this, and the libx* callers do not
> have that knowledge. A "get_" per-parameter function pointer, which
> would handle formatting for each parameter, be it int, uint, string or
> custom, seems cleaner to me than leaving it to the caller.

I disagree. The caller requires no knowledge of the actual format.
It could easily format the string twice (once assuming it might be a
signed quantity, and another time assuming it might be an unsigned
one). All it would need to know is the width of the binary
representation.

> By the way, the current implementation searches in [__param_start
> __param_end), which are only the runtime parameters, not all
> parameters, correct? So the series should be renamed to "Support for
> reading runtime-only hypervisor parameters". The command is useful for
> checking parameters that can be changed at runtime after all. 

Yes, such renaming would seem desirable.

> I believe there are currently no signed integer runtime parameters. So
> alternatively we could add a warning to the commit message and/or to
> the code stating that signed integer runtime parameters, if added,
> would not be printed correctly at the moment. This would gloss over
> rather than solve the issue.

That's an option. As for other aspects, I don't heavily mind cases
not getting dealt with right away which have no practical use right
now, as long as the restrictions are clearly spelled out, such that
no-one will be surprised when trying to add a runtime option beyond
what the code is able to deal with.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel