RE: [Xen-devel] [Patch] Enable SMEP CPU feature support for XEN itself

["Li, Xin" <xin.li@xxxxxxxxx>]

> > --- a/xen/arch/x86/cpu/common.c     Wed Jun 01 11:11:43 2011 +0100
> > +++ b/xen/arch/x86/cpu/common.c     Wed Jun 01 19:53:52 2011 +0800
> > @@ -28,6 +28,9 @@
> >  integer_param("cpuid_mask_ext_ecx", opt_cpuid_mask_ext_ecx);
> >  unsigned int __devinitdata opt_cpuid_mask_ext_edx = ~0u;
> >  integer_param("cpuid_mask_ext_edx", opt_cpuid_mask_ext_edx);
> > +/* nosmep: if true, Intel SMEP is disabled. */
> > +static bool_t __initdata disable_smep;
> 
> An __initdata variable used in ...

a mistake copied from native patch :) we'll change it to __cpuinitdata

> 
> > +boolean_param("nosmep", disable_smep);
> >
> >  struct cpu_dev * cpu_devs[X86_VENDOR_NUM] = {};
> >
> > @@ -222,6 +225,17 @@
> >     c->x86_capability[4] = cap4;
> >  }
> >
> > +static void __cpuinit setup_smep(struct cpuinfo_x86 *c)
> > +{
> > +   if ( cpu_has(c, X86_FEATURE_SMEP) ) {
> > +           if( unlikely(disable_smep) ) {
> 
> ... a __cpuinit function?

If change disable_smep to __cpuinitdata, this should be ok.


> 
> > +                   setup_clear_cpu_cap(X86_FEATURE_SMEP);
> > +                   clear_in_cr4(X86_CR4_SMEP);
> > +           } else
> > +                   set_in_cr4(X86_CR4_SMEP);
> 
> Anyway, the whole thing is overkill - {set,clear}_in_cr4() write
> the updated bits to mmu_cr4_features, and these get loaded
> on secondary CPUs *before* you have any chance of looking
> at the CPUID bits. As with everything else, it's assumed that
> APs don't have less features than the BP, and hence you only
> need to set_in_cr4() once (on the BP). And then the function
> can be __init.
>

Do you mean? 
        if ( cpu_has(c, X86_FEATURE_SMEP) )
                if( likely(!disable_smep) ) {
                        mmu_cr4_features |= X86_CR4_SMEP;
                        set_in_cr4(0);
                } else
                        setup_clear_cpu_cap(X86_FEATURE_SMEP);

Sounds good ... but the code will be harder to read, as it implicitly set smep?
Also where to put setup_smep thus it's only called in BP?

> > +   }
> > +}
> > +
> >  void __cpuinit generic_identify(struct cpuinfo_x86 * c)
> >  {
> >     u32 tfms, xlvl, capability, excap, ebx;
> > @@ -268,6 +282,8 @@
> 
> Would also be really helpful if you patch was generated with -p.
> 
> >             c->x86_capability[X86_FEATURE_FSGSBASE / 32] = ebx;
> >     }
> >
> > +   setup_smep(c);
> > +
> >     early_intel_workaround(c);
> >
> >  #ifdef CONFIG_X86_HT
> > diff -r d4f6310f1ef5 xen/arch/x86/traps.c
> > --- a/xen/arch/x86/traps.c  Wed Jun 01 11:11:43 2011 +0100
> > +++ b/xen/arch/x86/traps.c  Wed Jun 01 19:53:52 2011 +0800
> > @@ -1195,8 +1195,16 @@
> >      if ( ((l3e_get_flags(l3e) & required_flags) != required_flags) ||
> >           (l3e_get_flags(l3e) & disallowed_flags) )
> >          return 0;
> > -    if ( l3e_get_flags(l3e) & _PAGE_PSE )
> > +    if ( l3e_get_flags(l3e) & _PAGE_PSE ) {
> > +        /* SMEP fault error code 10001b */
> > +        if ( (error_code & PFEC_insn_fetch) &&
> > +             !(error_code & PFEC_user_mode) &&
> > +             cpu_has_smep &&
> > +             (_PAGE_USER & l4e_get_flags(l4e) & l3e_get_flags(l3e)) )
> > +            return 2;
> > +
> >          return 1;
> > +    }
> >  #endif
> >  #endif
> >
> > @@ -1207,8 +1215,21 @@
> >      if ( ((l2e_get_flags(l2e) & required_flags) != required_flags) ||
> >           (l2e_get_flags(l2e) & disallowed_flags) )
> >          return 0;
> > -    if ( l2e_get_flags(l2e) & _PAGE_PSE )
> > +    if ( l2e_get_flags(l2e) & _PAGE_PSE ) {
> > +        /* SMEP fault error code 10001b */
> > +        if ( (error_code & PFEC_insn_fetch) &&
> > +             !(error_code & PFEC_user_mode) &&
> > +             cpu_has_smep &&
> > +             (_PAGE_USER &
> > +#if CONFIG_PAGING_LEVELS >= 4
> > +              l4e_get_flags(l4e) &
> > +              l3e_get_flags(l3e) &
> > +#endif
> > +              l2e_get_flags(l2e)) )
> > +            return 2;
> > +
> >          return 1;
> > +    }
> >
> >      l1t = map_domain_page(mfn);
> >      l1e = l1e_read_atomic(&l1t[l1_table_offset(addr)]);
> > @@ -1218,6 +1239,18 @@
> >           (l1e_get_flags(l1e) & disallowed_flags) )
> >          return 0;
> >
> > +    /* SMEP fault error code 10001b */
> > +    if ( (error_code & PFEC_insn_fetch) &&
> > +         !(error_code & PFEC_user_mode) &&
> > +         cpu_has_smep &&
> > +         (_PAGE_USER &
> > +#if CONFIG_PAGING_LEVELS >= 4
> > +          l4e_get_flags(l4e) &
> > +          l3e_get_flags(l3e) &
> > +#endif
> > +          l2e_get_flags(l2e) & l1e_get_flags(l1e)) )
> > +        return 2;
> 
> The further down I get the uglier this looks. Can't you simply
> accumulate the user bit into a separate variable? That way the
> compiler also doesn't need to keep around all the l[1234]e
> variables.

At the beginning we did accumulate the user bit into a separate variable. 
However
SMEP faults hardly happen while we keep accumulating user bit no matter it's a
spurious fault or not, and even spurious faults are rare I guess.
Thanks!
-Xin


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel