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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.08.16 at 04:47, <he.chen@xxxxxxxxxxxxxxx> wrote:
> On Wed, Aug 24, 2016 at 04:01:53AM -0600, Jan Beulich wrote:
>> >>> On 19.08.16 at 12:20, <he.chen@xxxxxxxxxxxxxxx> wrote:
>> > @@ -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?

I realize that I should have deleted that remark after having checked
VMX code: Not setting the two flags in mmu_cr4_features is actually
what we want. The guest can still set the flags in its view of the
register, independent of what's 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.
>> 
> 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?

Yes.

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

Yes.

Jan


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