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

[George Dunlap <george.dunlap@citrix.com>]

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@intel.com>
> ---
>  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.

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.

> +
> +    if ( !cpu_has_xsave || !pkuf || is_pv_vcpu(vcpu) )
> +        return 0;
> +
> +    /* PKRU dom0 is always zero */

"dom0" has a very specific meaning in Xen.  I think this would be better
written "pkey 0 always has full access".

> +    if ( likely(!pte_pkeys) )
> +        return 0;
> +
> +    /* Update vcpu xsave area */
> +    fpu_xsave(vcpu);

Is there a reason you're calling fpu_xsave() directly here, rather than
just calling vcpu_save_fpu()?  That saves you actually doing the xsave
if the fpu hasn't been modified since the last time you read it.

> +    xsave_addr = get_xsave_addr(vcpu->arch.xsave_area, fls64(XSTATE_PKRU)-1);
> +    if ( !!xsave_addr )
> +        memcpy(&pkru, xsave_addr, sizeof(pkru));

There's no need for the !! here.  But in any case, isn't there a better
function for reading the xsave state than manually calculating the
address and doing a memcpy?

> +
> +    if ( unlikely(pkru) )
> +    {
> +        /*
> +         * PKU:  additional mechanism by which the paging controls
> +         * access to user-mode addresses based on the value in the
> +         * PKRU register. A fault is considered as a PKU violation if all
> +         * of the following conditions are ture:
> +         * 1.CR4_PKE=1.
> +         * 2.EFER_LMA=1.
> +         * 3.page is present with no reserved bit violations.
> +         * 4.the access is not an instruction fetch.
> +         * 5.the access is to a user page.
> +         * 6.PKRU.AD=1
> +         *       or The access is a data write and PKRU.WD=1
> +         *            and either CR0.WP=1 or it is a user access.
> +         */
> +        pkru_ad = read_pkru_ad(pkru, pte_pkeys);
> +        pkru_wd = read_pkru_wd(pkru, pte_pkeys);
> +        if ( hvm_pku_enabled(vcpu) && hvm_long_mode_enabled(vcpu) &&
> +            !rsvdf && !ff && (pkru_ad ||
> +            (pkru_wd && wf && (hvm_wp_enabled(vcpu) || uf))))
> +            return 1;

This statement here is really difficult to read.  Why don't you put the
checks which don't depend on the pkru up before you read it?  e.g.,
hvm_pku_enabled(), hvm_long_mode_enabled(), rsvdf, ff, &c?

 -George