Re: [Xen-devel] [V3 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

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

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxx] On Behalf Of George Dunlap
> Sent: Friday, December 11, 2015 2:20 AM
> To: Han, Huaitong <huaitong.han@xxxxxxxxx>; jbeulich@xxxxxxxx;
> andrew.cooper3@xxxxxxxxxx; Nakajima, Jun <jun.nakajima@xxxxxxxxx>; Dong,
> Eddie <eddie.dong@xxxxxxxxx>; Tian, Kevin <kevin.tian@xxxxxxxxx>;
> george.dunlap@xxxxxxxxxxxxx; ian.jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; ian.campbell@xxxxxxxxxx; 
> wei.liu2@xxxxxxxxxx;
> keir@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [V3 PATCH 7/9] x86/hvm: pkeys, add pkeys support for
> guest_walk_tables
> 
> On 07/12/15 09:16, Huaitong Han wrote:
> > This patch adds pkeys support for guest_walk_tables.
> >
> > Signed-off-by: Huaitong Han <huaitong.han@xxxxxxxxx>
> > ---
> >  xen/arch/x86/i387.c           |  2 +-
> >  xen/arch/x86/mm/guest_walk.c  | 73
> +++++++++++++++++++++++++++++++++++++++++++
> >  xen/include/asm-x86/hvm/hvm.h |  2 ++
> >  xen/include/asm-x86/i387.h    |  1 +
> >  4 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
> > index b661d39..83c8465 100644
> > --- a/xen/arch/x86/i387.c
> > +++ b/xen/arch/x86/i387.c
> > @@ -132,7 +132,7 @@ static inline uint64_t vcpu_xsave_mask(const struct
> vcpu *v)
> >  }
> >
> >  /* Save x87 extended state */
> > -static inline void fpu_xsave(struct vcpu *v)
> > +void fpu_xsave(struct vcpu *v)
> >  {
> >      bool_t ok;
> >      uint64_t mask = vcpu_xsave_mask(v);
> > diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> > index 18d1acf..e79f72f 100644
> > --- a/xen/arch/x86/mm/guest_walk.c
> > +++ b/xen/arch/x86/mm/guest_walk.c
> > @@ -31,6 +31,8 @@ asm(".file \"" __OBJECT_FILE__ "\"");
> >  #include <xen/sched.h>
> >  #include <asm/page.h>
> >  #include <asm/guest_pt.h>
> > +#include <asm/xstate.h>
> > +#include <asm/i387.h>
> >
> >  extern const uint32_t gw_page_flags[];
> >  #if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
> > @@ -90,6 +92,61 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p,
> int set_dirty)
> >      return 0;
> >  }
> >
> > +#if GUEST_PAGING_LEVELS >= CONFIG_PAGING_LEVELS
> > +bool_t leaf_pte_pkeys_check(struct vcpu *vcpu, uint32_t pfec,
> > +                uint32_t pte_access, uint32_t pte_pkeys)
> 
> pte_access doesn't seem to be used at all.
> 
> > +{
> > +    void *xsave_addr;
> > +    unsigned int pkru = 0;
> > +    bool_t pkru_ad, pkru_wd;
> > +
> > +    bool_t uf = !!(pfec & PFEC_user_mode);
> > +    bool_t wf = !!(pfec & PFEC_write_access);
> > +    bool_t ff = !!(pfec & PFEC_insn_fetch);
> > +    bool_t rsvdf = !!(pfec & PFEC_reserved_bit);
> > +    bool_t pkuf  = !!(pfec & PFEC_prot_key);
> 
> So I'm just wondering out loud here -- is there actually any situation
> in which we would want guest_walk_tables to act differently than the
> real hardware?
> 
> That is, is there actually any situation where, pku is enabled, the vcpu
> is in long mode, PFEC_write_access and/or PFEC_page_present is set, and
> the pkey is non-zero, that we want guest_walk_tables() to only check the
> write-protect bit for the pte, and not also check the pkru?
> 
> Because if not, it seems like it would be much more robust to simply
> *always* check for pkru_ad if PFEC_page_present is set, and for pkru_wd
> if PFEC_write_access is set.

guest_walk_tables() is also used by shadow code, though we don't
plan to support pkeys for shadow now, however, in that case, the 'pfec'
is generated by hardware, and the pkuf bit may be 0 or 1 depending
on the real page fault. If we unconditionally check pkeys in
guest_walk_tables(), it is not a good ideas for someone who may
want to implement the pkeys for shadow in future, since we only
need to check pkeys when the pkuf is set in pfec.

> 
> Then in patch 8, you wouldn't need to go around all the __hvm_copy
> functions adding in PFEC_prot; instead, you'd just need to add
> PFEC_insn_fetch to the "fetch" (as is already done for SMEP and NX), and
> you'd be done.

Pkeys has no business with instructions fetch, why do we need to add
PFEC_insn_fetch?

Thanks,
Feng

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