Re: [Xen-devel] [PATCH v4] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.08.16 at 11:49, <he.chen@xxxxxxxxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1433,6 +1433,7 @@ Set the serial transmit buffer size.
>  > Default: `true`
>  
>  Flag to enable Supervisor Mode Execution Protection
> +Use `smep=hvm` to enable SMEP for HVM guests only.

I'm sorry for noticing this only now, but this wording is potentially
confusing: Xen doesn't enable SMEP (or SMAP) for HVM guests. It
only _allows_ such guests to enable it for themselves. I.e. how
about

Use `smep=hvm` to allow SMEP use by HVM guests only.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -62,12 +62,12 @@ static unsigned int __initdata max_cpus;
>  integer_param("maxcpus", max_cpus);
>  
>  /* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
> -static bool_t __initdata opt_smep = 1;
> -boolean_param("smep", opt_smep);
> +static void parse_smep_param(char *s);
> +custom_param("smep", parse_smep_param);
>  
>  /* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
> -static bool_t __initdata opt_smap = 1;
> -boolean_param("smap", opt_smap);
> +static void parse_smap_param(char *s);
> +custom_param("smap", parse_smap_param);

Already on v3 I did tell you that you hadn't reacted on certain of the
the v2 review comments. And now I see that e.g. you still use forward
declarations here when you don't really need to.

> @@ -111,6 +111,48 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 
> 0, 0, -1 };
>  
>  unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
>  
> +#define SMEP_HVM_ONLY (-1)
> +static int __initdata opt_smep = 1;

s8 would do.

> +static void __init parse_smep_param(char *s)
> +{
> +    if ( !strcmp(s, "hvm") )
> +    {
> +        opt_smep = SMEP_HVM_ONLY;
> +    }
> +    else if ( !parse_bool(s) )
> +    {
> +        opt_smep = 0;
> +    }
> +    else if ( parse_bool(s) && opt_smep != SMEP_HVM_ONLY )
> +    {
> +        opt_smep = 1;
> +    }

All the braces are pointless here. And I still don't agree with the
flow, especially with the double invocation of parse_bool(). Why
don't you invoke it once first thing, and then properly handle its
tristate return value? That'll then also hopefully (and finally)
result in you no longer making not work certain multiple
specifications of "smep=" on the command line (right now it looks
like "smep=hvm smep=1" would not have the intended effect).

> @@ -67,7 +69,8 @@ XEN_CPUFEATURE(MFENCE_RDTSC,    (FSCAPINTS+0)*32+ 9) /* 
> MFENCE synchronizes RDTS
>  #define cpu_has_fsgsbase     boot_cpu_has(X86_FEATURE_FSGSBASE)
>  #define cpu_has_aperfmperf   boot_cpu_has(X86_FEATURE_APERFMPERF)
>  #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
> -#define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
> +#define cpu_has_xen_smep        boot_cpu_has(X86_FEATURE_XEN_SMEP)
> +#define cpu_has_xen_smap        boot_cpu_has(X86_FEATURE_XEN_SMAP)

Why do you remove cpu_has_smap but not also cpu_has_smep?

And the newly added shorthands are misnamed: You really mean
e.g. cpu_using_smep. But then I think not having these short hands
would be even better, the more that specifically the smep one is
being used in exactly one place.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel