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

["Jan Beulich" <jbeulich@novell.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2642 bytes --]

>>> "Li, Xin"  06/02/11 6:20 AM >>>
>> > +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.

You would be okay, but as I wrote further down both are really
only needed on the BP.

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

Why?

>set_in_cr4(0);

set_in_cr4(X86_CR4_SMEP) does exactly what you need.

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

early_cpu_detect() would seem to be the most logical place, though
it doesn't have all the x86_capabilities[] fields set up yet. The BP-only
part at the end of identify_cpu() would also be a possible place.

trap_init() would be another possible (and reasonably logical) place.

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

Remember that we're going through this function for almost every page
fault happening in Xen, and also for the majority of those originating
from certain pv guests (when they have suppress_spurious_page_faults
set).

Also, my comment was to a large part aiming at better legibility of the
code you add.

Jan


[-- Attachment #1.2: HTML --]
[-- Type: text/html, Size: 3650 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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