RE: [Xen-devel] [PATCH v2] add SMEP support to HVM guest

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

> > -    if ( hvm_nx_enabled(current) )
> > +    if ( hvm_nx_enabled(current) ||
> > +         (!(pfec & PFEC_user_mode) && hvm_smep_enabled(current)) )
> 
> Shouldn't that be
> "if ( hvm_nx_enabled(current) || hvm_smep_enabled(current) )"?

A smep fault happens when
1) current privilege level < 3

> > +         (!(pfec & PFEC_user_mode) && hvm_smep_enabled(current)) )
> 
> Likewise.
> 
> >      l4p = (guest_l4e_t *) top_map;
> >      gw->l4e = l4p[guest_l4_table_offset(va)];
> >      gflags = guest_l4e_get_flags(gw->l4e) ^ iflags;
> > +    user_flag &= gflags;
> 
> There's no need to add SMEP-specific code at every level.  The existing
> code already checks for flags that must be clear, so just arrange for
> _PAGE_USER to be in both mflags and iflags whenever SMEP is enabled and
> PFEC_user is clear.

2) user flags in all page table level entries are set instead of clear.

In current code, what I read is that it assumes some permission(s) is missing
in guest_walk_tables(). NX is special, but your code wisely used inverted logic
to seamlessly cover it. I did want to reuse the existing logic, but seems it 
can't
accommodate SMEP logic easily.

Please advise!


> > @@ -277,6 +281,11 @@ guest_walk_tables(struct vcpu *v, struct
> >       * walkers behave this way. */
> >      if ( rc == 0 )
> >      {
> > +        if ( guest_supports_smep(v) && user_flag &&
> > +             ((pfec & (PFEC_insn_fetch|PFEC_user_mode)) ==
> PFEC_insn_fetch) ) {
> > +            rc = _PAGE_SMEP;
> > +            goto out;
> > +        }
> 
> I think this hunk will probably go away entirely, but if not, please
> don't put it between the code below and the comment above that describes
> it.

I didn't figure out a better way than this, will add comments if we have to do
this way.  Or do you have any other suggestion?

> > +#define _PAGE_SMEP    0x8000U
> 
> What does this new code mean?  You added code that returns it but not
> any that checks for it.

Actually the current logic doesn't check what is missing returned from
guest_walk_tables unless it needs to change error code, and the callers
mostly return INVALID_GFN.
I introduced _PAGE_SMEP just to let the caller know it is not 0.  Probably
I can reuse one of the _PAGE_ macros, but the memory virtualization code
is subtle, I'm afraid I'll introduce bugs in other parts. That's to say, I do 
want
to hear from you :)
Thanks!
-Xin

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