Re: [Xen-devel] [PATCH v1 4/6] x86/hvm: Add SMAP support to HVM guest

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, April 15, 2014 5:26 PM
> To: Wu, Feng
> Cc: Ian.Campbell@xxxxxxxxxx; Dong, Eddie; Nakajima, Jun;
> xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v1 4/6] x86/hvm: Add SMAP support to HVM guest
> 
> >>> On 15.04.14 at 15:02, <feng.wu@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm/guest_walk.c
> > +++ b/xen/arch/x86/mm/guest_walk.c
> > @@ -29,6 +29,7 @@
> >  #include <xen/sched.h>
> >  #include <asm/page.h>
> >  #include <asm/guest_pt.h>
> > +#include <asm/hvm/vmx/vmx.h>
> >
> >
> >  /* Flags that are needed in a pagetable entry, with the sense of NX 
> > inverted
> */
> > @@ -144,14 +145,18 @@ guest_walk_tables(struct vcpu *v, struct
> p2m_domain *p2m,
> >      guest_l4e_t *l4p;
> >  #endif
> >      uint32_t gflags, mflags, iflags, rc = 0;
> > -    int smep;
> > +    int smep, smap;
> 
> These want to be bool_t I suppose.
> 
> >      bool_t pse1G = 0, pse2M = 0;
> > +    unsigned long sel = 0;
> > +    uint64_t eflags = guest_cpu_user_regs()->eflags;
> >      p2m_query_t qt = P2M_ALLOC | P2M_UNSHARE;
> >
> >      perfc_incr(guest_walk);
> >      memset(gw, 0, sizeof(*gw));
> >      gw->va = va;
> >
> > +    __vmread(GUEST_CS_SELECTOR, &sel);
> 
> You're in common code here - please use the proper HVM
> abstraction.
> 
> > @@ -165,7 +170,21 @@ guest_walk_tables(struct vcpu *v, struct
> p2m_domain *p2m,
> >       * whole walk as if it were a user-mode one and then invert the
> answer. */
> >      smep = (is_hvm_vcpu(v) && hvm_smep_enabled(v)
> >              && (pfec & PFEC_insn_fetch) && !(pfec &
> PFEC_user_mode) );
> > -    if ( smep )
> > +
> > +    /*
> > +     * SMAP: kernel-mode data accesses from user-mode mappings should
> fault
> > +     * A fault is considered as a SMAP violation if the following
> > +     * conditions come ture:
> 
> "true"
> 
> > +     *   - X86_CR4_SMAP is set in CR4
> > +     *   - An user page is accessed
> 
> "A user page ..." afaik.
> 
> > +     *   - CPL = 3 or X86_EFLAGS_AC clear set)
> 
> Stray closing parenthesis.
> 
> > +     *   - Page fault in kernel mode
> > +     */
> > +    smap = ( is_hvm_vcpu(v) && hvm_smap_enabled(v)
> > +            && !(!((sel & 3) == 3) && (eflags & X86_EFLAGS_AC))
> 
> !( == ) is better written as ( != ) or, as done elsewhere, ( < ).
> 
> > +            && !(pfec & PFEC_user_mode) );
> > +
> > +    if ( smep || smap )
> 
> Again, please fold these are far as possible (is_hvm_vcpu() and
> the PFEC_user_mode are common and hence should be done just
> once.
> 
> > @@ -363,6 +365,16 @@ static inline int hvm_event_pending(struct vcpu *v)
> >  #define HVM_CR4_HOST_MASK (mmu_cr4_features & \
> >      (X86_CR4_VMXE | X86_CR4_PAE | X86_CR4_MCE))
> >
> > +static inline bool_t hvm_cpuid_has_smap(void)
> > +{
> > +    unsigned int eax = 0, ebx = 0, ecx = 0, edx = 0;
> > +    unsigned int leaf = 0x7;
> > +
> > +    hvm_cpuid(leaf, &eax, &ebx, &ecx, &edx);
> 
> Please pass NULL for all outputs you don't need.
> 
> > @@ -371,6 +383,7 @@ static inline int hvm_event_pending(struct vcpu *v)
> >          X86_CR4_MCE | X86_CR4_PGE | X86_CR4_PCE |       \
> >          X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT |           \
> >          (cpu_has_smep ? X86_CR4_SMEP : 0) |             \
> > +        (hvm_cpuid_has_smap() ? X86_CR4_SMAP : 0) |     \
> 
> What's the reason for the asymmetry with SMEP here? Also, did you
> verify that v == current in all call paths? And even if you did, passing
> v into the function and adding a respective ASSERT() would seem
> rather desirable.

My original thought is that "cpu_has_smap" reflects whether the host cpu has
SMAP feature, however, here we need to check whether the VCPU of the hvm
has this feature, so I used hvm_cpuid_has_smap() instead. But I thought about
this logic again yesterday, seems using "cpu_has_smap" here is okay, since we
always expose the SMAP feature to HVM guest if it exists on the host cpu. So
I will change this to align with smep.

> 
> Jan

Thanks,
Feng

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