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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

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

Jan