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

[Jürgen Groß <jgross@xxxxxxxx>]

On 19.02.20 17:44, Jan Beulich wrote:
> 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/?
Yes, this is better.

> > --- 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?
I just followed the general pattern in this file.

> > --- 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.
Hmm, this would require a general update_value() function for each
custom runtime parameter.

I can see how this would look like.

> > +
> > +
> > +static void update_ept_param(void)
> No double blank lines please.
Oh, sorry.

> > @@ -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()?
Oh, of course.

> > @@ -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.
There is more than pure integer semantics here: the value is limited,
so I can't just use the normal integer assignment.

Its not as if those functions are performance critical, and special
casing for sparing the buffer memory will probably waste more memory
due to larger code size.

> > @@ -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.
Okay.

> > @@ -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?
I have some further patches in my pipeline which will do even more,
by removing the sysctl for setting parameters and using hypfs for
that purpose in libxl. This will remove the need for the runtime
copy of struct kernel_param completely.

But r

> > +          .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
Ah, nice idea.

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

> > @@ -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()?
Yes.


Juergen

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