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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.12.15 at 10:16, <feng.wu@xxxxxxxxx> wrote:
>> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
>> bounces@xxxxxxxxxxxxx] On Behalf Of George Dunlap
>> Sent: Friday, December 11, 2015 2:20 AM
>> 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.

If shadow code isn't to support pkeys, how would an error code ever
have the respective bit set?

Jan


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