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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.02.2020 13:47, Juergen Gross wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -70,6 +70,30 @@ integer_param("ple_window", ple_window);
>  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;
> +static char opt_ept_setting[16];

I don't think this is quite big enough.

> +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.

> +    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.

> +static void __init init_ept_param(struct param_hypfs *par)
> +{
> +    custom_runtime_set_var(par, opt_ept_setting);
> +    update_ept_param();
> +}
>  
>  static int __init parse_ept_param(const char *s)
>  {
> @@ -93,6 +117,8 @@ static int __init parse_ept_param(const char *s)
>          s = ss + 1;
>      } while ( *ss );
>  
> +    update_ept_param();

Isn't this redundant with the use in init_ept_param() (or the
other way around - there should be clear ordering between the
command line parsing functions and the param-init ones, I would
suppose)?

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -20,8 +20,27 @@ static __read_mostly enum {
>      PCID_OFF,
>      PCID_ALL,
>      PCID_XPTI,
> -    PCID_NOXPTI
> +    PCID_NOXPTI,
> +    PCID_END
>  } opt_pcid = PCID_XPTI;
> +static const char *opt_pcid_2_string[PCID_END] = {

You either want another const here, or (more space efficient) you
want to use const char[PCID_END][7].

> +    [PCID_OFF] = "off",
> +    [PCID_ALL] = "on",
> +    [PCID_XPTI] = "xpti",
> +    [PCID_NOXPTI] = "noxpti"
> +};
> +static char opt_pcid_val[7];
> +
> +static void update_opt_pcid(void)
> +{
> +    strlcpy(opt_pcid_val, opt_pcid_2_string[opt_pcid], sizeof(opt_pcid_val));

Instead of copying, couldn't you make the hypfs entry point
into the array above, by using custom_runtime_set_var() here?

> @@ -55,9 +74,12 @@ static int parse_pcid(const char *s)
>          break;
>      }
>  
> +    if ( !rc )
> +        update_opt_pcid();

Personally I'd avoid the if() here - there's no harm updating
the hypfs entry anyway.

> --- 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?

> --- 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.)

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -75,12 +75,36 @@ enum con_timestamp_mode
>      TSM_DATE_MS,       /* [YYYY-MM-DD HH:MM:SS.mmm] */
>      TSM_BOOT,          /* [SSSSSS.uuuuuu] */
>      TSM_RAW,           /* [XXXXXXXXXXXXXXXX] */
> +    TSM_END
> +};
> +
> +static const char *con_timestamp_mode_2_string[TSM_END] = {
> +    [TSM_NONE] = "none",
> +    [TSM_DATE] = "date",
> +    [TSM_DATE_MS] = "datems",
> +    [TSM_BOOT] = "boot",
> +    [TSM_RAW] = "raw"
>  };
>  
>  static enum con_timestamp_mode __read_mostly opt_con_timestamp_mode = 
> TSM_NONE;
> +static char con_timestamp_mode_val[7];
> +
> +static void update_con_timestamp_mode(void)
> +{
> +    strlcpy(con_timestamp_mode_val,
> +            con_timestamp_mode_2_string[opt_con_timestamp_mode],
> +            sizeof(con_timestamp_mode_val));
> +}
> +
> +static void __init con_timestamp_mode_init(struct param_hypfs *par)
> +{
> +    custom_runtime_set_var(par, con_timestamp_mode_val);
> +    update_con_timestamp_mode();
> +}
>  
>  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_init);

Same remark as for the PCID option, and then also for the log level
ones further down. My main concern with how things are currently is
that the amount of logic needed for custom params seems overly
large.

> @@ -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(),
and why taking the address instead of enforcing this to be of
(at least) array (potentially also "of char") type? Do you
envision this to be needed for anything where the value isn't
in string form, but still needs dynamically calculating? (As per
above there may already be cases where non-string variables may
want passing into here.)

Jan

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