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

[He Chen <he.chen@xxxxxxxxxxxxxxx>]

On Wed, Aug 24, 2016 at 04:01:53AM -0600, Jan Beulich wrote:
> >>> 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.
> 
Let me try to address them well in next patch...

> > @@ -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.
> 
I am sorry that I am not so clear about this. As what I see in the code,
`mmu_cr4_features` get changed in `set_in_cr4` and `clear_in_cr4` only.
May I ask why the sm{e,a}p is also in `mmu_cr4_features` and where else
it will be set?

> > @@ -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.
> 
Here, do you suggest to add a artificial SMAP feature (not from hardware
but according to the `smap` option) bit in `x86_capability` and to patch
SMAP instruction according to this new bit rather than actual hardware
SMAP bit?

Regarding SMEP, even if there are not instructions need to be patched,
but for symmetry reasons we should also add **another** new SMEP bit in
`x86_capability`, right?

> > @@ -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."
> 
My carelessness, sorry for this.

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

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