Re: [Xen-devel] [PATCH v3] xen: support enabling SMEP/SMAP for HVM only

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.08.16 at 12:20, <he.chen@xxxxxxxxxxxxxxx> wrote:
> Changes in v3:
> * Fix boot options.
> * Fix CR4 & mmu_cr4_features operations.
> * Disable SMEP/SMAP for Dom0.
> * Commit message refinement.

Several of my comments on v3 did not get taken care of (neither in
code nor verbally). I'm not going to repeat them here.

> @@ -1403,12 +1437,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( !opt_smep )
>          setup_clear_cpu_cap(X86_FEATURE_SMEP);
> -    if ( cpu_has_smep )
> +    if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
>          set_in_cr4(X86_CR4_SMEP);
>  
>      if ( !opt_smap )
>          setup_clear_cpu_cap(X86_FEATURE_SMAP);
> -    if ( cpu_has_smap )
> +    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
>          set_in_cr4(X86_CR4_SMAP);

So this avoids setting the flags in CR4, but also in mmu_cr4_features.

> @@ -1430,8 +1464,19 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      arch_init_memory();
>  
> +    /*
> +     * Temporarily clear SMAP in internal feature bitmap to avoid
> +     * patching unnecessary SMAP instructions when SMAP is disabled in
> +     * Xen hypervisor.
> +     */
> +    if ( opt_smap == SMAP_HVM_ONLY )
> +        __clear_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
> +
>      alternative_instructions();
>  
> +    if ( opt_smap == SMAP_HVM_ONLY )
> +        __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);

I think the better approach would be to introduce a synthetic
feature, which gets set only when SMAP gets used by Xen for
itself. Even if not needed for alternative patching, I think for
symmetry reasons the same should then also be done for SMEP.

> @@ -1098,6 +1099,12 @@ void pv_cpuid(struct cpu_user_regs *regs)
>              b |= (host_featureset[FEATURESET_7b0] &
>                    special_features[FEATURESET_7b0]);
>  
> +            if ( opt_smep == SMEP_HVM_ONLY )
> +                b &= ~cpufeat_mask(X86_FEATURE_SMEP);
> +
> +            if ( opt_smap == SMAP_HVM_ONLY )
> +                b &= ~cpufeat_mask(X86_FEATURE_SMAP);

While you changed the place where you do the adjustment, my
previous comment holds: "These flags already can't be set in
pv_featureset, so the change is pointless."

> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -51,6 +51,12 @@ void microcode_grab_module(
>  
>  extern uint8_t kbd_shift_flags;
>  
> +#define SMEP_HVM_ONLY -1
> +extern int opt_smep;
> +
> +#define SMAP_HVM_ONLY -1
> +extern int opt_smap;

Which then means that these still don't need to become non-static.
The #define-s, if you mean to retain them (in setup.c) would of
course need proper parenthesization.

Jan


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