Re: [Xen-devel] [PATCH v5 8/8] xen: add runtime parameter access support to hypfs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2020 09:11, Juergen Gross wrote:
> --- a/docs/misc/hypfs-paths.pandoc
> +++ b/docs/misc/hypfs-paths.pandoc
> @@ -152,3 +152,12 @@ The major version of Xen.
>  #### /buildinfo/version/minor = INTEGER
>  
>  The minor version of Xen.
> +
> +#### /params/
> +
> +A directory of runtime parameters.
> +
> +#### /params/*
> +
> +The single parameters. The description of the different parameters can be

s/single/individual/?

> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -89,6 +89,11 @@ SECTIONS
>         __start_schedulers_array = .;
>         *(.data.schedulers)
>         __end_schedulers_array = .;
> +
> +       . = ALIGN(8);
> +       __paramhypfs_start = .;
> +       *(.data.paramhypfs)
> +       __paramhypfs_end = .;

Do you really need 8-byte alignment even on 32-bit Arm?

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -70,6 +70,17 @@ 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] = "pml=1";

This is dangerous imo - such strings would better be populated
during boot by invoking the same function that also does so
after updating. Otherwise it won't take long until reported
and actual settings will be out of sync, until first modified
via this new interface.

> +
> +
> +static void update_ept_param(void)

No double blank lines please.

> @@ -31,10 +32,12 @@ static int parse_pcid(const char *s)
>      {
>      case 0:
>          opt_pcid = PCID_OFF;
> +        snprintf(opt_pcid_val, sizeof(opt_pcid_val), "off");
>          break;
>  
>      case 1:
>          opt_pcid = PCID_ALL;
> +        snprintf(opt_pcid_val, sizeof(opt_pcid_val), "on");
>          break;
>  
>      default:
> @@ -42,10 +45,12 @@ static int parse_pcid(const char *s)
>          {
>          case 0:
>              opt_pcid = PCID_NOXPTI;
> +            snprintf(opt_pcid_val, sizeof(opt_pcid_val), "noxpti");
>              break;
>  
>          case 1:
>              opt_pcid = PCID_XPTI;
> +            snprintf(opt_pcid_val, sizeof(opt_pcid_val), "xpti");
>              break;

Pretty expensive to use snprintf() here - how about strlcpy()?

> @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const 
> char *arg,
>          return -ERANGE;
>  
>      *valp = val;
> +    snprintf(par_val, PAR_VAL_SZ, "%lu", val);
>  
>      return 0;
>  }
>  
>  unsigned int __read_mostly opt_max_grant_frames = 64;
> +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64";

This and the other option are plain integer ones from a presentation
pov, so it would be nice to get away here without the extra buffers.

> @@ -289,6 +290,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 ( buf[ulen - 1] )

Perhaps memchr() again.

> @@ -23,10 +24,17 @@ struct kernel_param {
>      } par;
>  };
>  
> +struct param_hypfs {
> +    const struct kernel_param *param;

As long as this is here, I don't think ...

> @@ -76,40 +84,87 @@ 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

... you need the alignment attribute here. But see below.

> -#define custom_runtime_only_param(_name, _var) \
> +#define custom_runtime_only_param(_name, _var, contvar) \
>      __rtparam __rtpar_##_var = \
>        { .name = _name, \
>            .type = OPT_CUSTOM, \
> -          .par.func = _var }
> +          .par.func = _var }; \
> +    __paramfs __parfs_##_var = \
> +        { .param = &__rtpar_##_var, \

Instead of a pointer, can't the param struct be part of this
bigger structure?

> +          .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
> +          .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
> +          .hypfs.e.name = _name, \
> +          .hypfs.e.size = sizeof(contvar), \

This will go wrong if contvar is not an array. I guess you want
ARRAY_SIZE(contvar) * sizeof(*(convar)) here, and perhaps also
...

> +          .hypfs.e.read = hypfs_read_leaf, \
> +          .hypfs.e.write = hypfs_write_custom, \
> +          .hypfs.content = &contvar }

... omit the & here.

> @@ -123,4 +178,7 @@ extern const struct kernel_param __param_start[], 
> __param_end[];
>      string_param(_name, _var); \
>      string_runtime_only_param(_name, _var)
>  
> +#define param_append_str(var, fmt, val) \
> +    snprintf(var + strlen(var), sizeof(var) - strlen(var), fmt, val)

The sizeof() here again isn't safe against var not being of array
type. Also again perhaps cheaper to use strlcat()?

Jan

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