From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: RE: [Patch] Enable SMEP CPU feature support for XEN itself Date: Thu, 02 Jun 2011 14:29:32 +0100 Message-ID: <4DE79E4C0200007800070D60@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2017545497==" Return-path: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: wei.y.yang@intel.com, xin.li@intel.com Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org This is a MIME message. If you are reading this text, you may want to consider changing to a mail reader or gateway that understands how to properly handle MIME multipart messages. --===============2017545497== Content-Type: multipart/alternative; boundary="=__Part7E52D92C.1__=" This is a MIME message. If you are reading this text, you may want to consider changing to a mail reader or gateway that understands how to properly handle MIME multipart messages. --=__Part7E52D92C.1__= Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable >>> "Li, Xin" 06/02/11 6:20 AM >>> >> > +boolean_param("nosmep", disable_smep); >> > >> > struct cpu_dev * cpu_devs[X86_VENDOR_NUM] =3D {}; >> > >> > @@ -222,6 +225,17 @@ >> > c->x86_capability[4] =3D cap4; >> > } >> > >> > +static void __cpuinit setup_smep(struct cpuinfo_x86 *c) >> > +{ >> > + if ( cpu_has(c, X86_FEATURE_SMEP) ) { >> > + if( unlikely(disable_smep) ) { >>=20 >> ... 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); >>=20 >> 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?=20 >if ( cpu_has(c, X86_FEATURE_SMEP) ) >if( likely(!disable_smep) ) { >mmu_cr4_features |=3D 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 --=__Part7E52D92C.1__= Content-Type: text/html; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Description: HTML >>> "Li, Xin" <xin.li@intel.com> = 06/02/11 6:20 AM >>>
>> > +boolean_param("nosmep", = disable_smep);
>> >
>> > struct cpu_dev * = cpu_devs[X86_VENDOR_NUM] =3D {};
>> >
>> > @@ = -222,6 +225,17 @@
>> >     c->x86_capab= ility[4] =3D cap4;
>> > }
>> >
>> > = +static void __cpuinit setup_smep(struct cpuinfo_x86 *c)
>> > = +{
>> > +    if ( cpu_has(c, X86_FEATURE_SM= EP) ) {
>> > +        i= f( 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.

>> > +  &nbs= p;         setup_clear_cpu_cap= (X86_FEATURE_SMEP);
>> > +      &= nbsp;     clear_in_cr4(X86_CR4_SMEP);
>> = > +        } else
>> = > +           &nb= sp;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_fea= tures |=3D X86_CR4_SMEP;

Why?

>set_in_cr4(0);

set_i= n_cr4(X86_CR4_SMEP) does exactly what you need.

>} else
>se= tup_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_detec= t() 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
--=__Part7E52D92C.1__=-- --===============2017545497== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============2017545497==--