Re: [PATCH v8 12/12] xen: remove XEN_SYSCTL_set_parameter support

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

On 14.05.20 15:03, Jan Beulich wrote:
> On 08.05.2020 17:34, Juergen Gross wrote:
>
> Besides the changes made, wouldn't it be worthwhile to change
> HYPFS Kconfig help wording from "result in some features not
> being available" to something mentioning runtime parameter
> setting in particular, and perhaps also from "might" to "will"?
Yes, good idea.

> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -71,27 +71,6 @@ 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;
> >   
> > -#ifdef CONFIG_HYPFS
> > -static char opt_ept_setting[10];
> > -
> > -static void update_ept_param(void)
> > -{
> > -    if ( opt_ept_exec_sp >= 0 )
> > -        snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
> > -                 opt_ept_exec_sp);
> > -}
> > -
> > -static void __init init_ept_param(struct param_hypfs *par)
> > -{
> > -    update_ept_param();
> > -    custom_runtime_set_var(par, opt_ept_setting);
> > -}
> > -#else
> > -static void update_ept_param(void)
> > -{
> > -}
> > -#endif
> > -
> >   static int __init parse_ept_param(const char *s)
> >   {
> >       const char *ss;
> > @@ -118,6 +97,22 @@ static int __init parse_ept_param(const char *s)
> >   }
> >   custom_param("ept", parse_ept_param);
> >   
> > +#ifdef CONFIG_HYPFS
> > +static char opt_ept_setting[10];
> > +
> > +static void update_ept_param(void)
> > +{
> > +    if ( opt_ept_exec_sp >= 0 )
> > +        snprintf(opt_ept_setting, sizeof(opt_ept_setting), "exec-sp=%d",
> > +                 opt_ept_exec_sp);
> > +}
> > +
> > +static void __init init_ept_param(struct param_hypfs *par)
> > +{
> > +    update_ept_param();
> > +    custom_runtime_set_var(par, opt_ept_setting);
> > +}
> If I'm not mistaken this is pure code movement, and solely to be
> able to have ...
>
> > +
> >   static int parse_ept_param_runtime(const char *s);
> >   custom_runtime_only_param("ept", parse_ept_param_runtime, init_ept_param);
> >   
> > @@ -172,6 +167,7 @@ static int parse_ept_param_runtime(const char *s)
> >   
> >       return 0;
> >   }
> > +#endif
> ... a single #ifdef region now including a few more lines. No
> strict need to change it, but couldn't the earlier patch have
> inserted the code in its final place right away?
I can do that.

> > @@ -1106,7 +1090,7 @@ struct xen_sysctl {
> >   #define XEN_SYSCTL_get_cpu_levelling_caps        25
> >   #define XEN_SYSCTL_get_cpu_featureset            26
> >   #define XEN_SYSCTL_livepatch_op                  27
> > -#define XEN_SYSCTL_set_parameter                 28
> > +/* #define XEN_SYSCTL_set_parameter                 28 */
> Nit: There are now 3 too many padding spaces. Granted that's
> how it was done for XEN_SYSCTL_tmem_op ...
You said: like it was done with tmem. ;-)

> In any event, as before,
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks,


Juergen