Re: [Xen-devel] [PATCH V5 4/6] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 22/12/15 10:30, Huaitong Han wrote:
> Protection keys define a new 4-bit protection key field(PKEY) in bits 62:59 of
> leaf entries of the page tables.
> 
> PKRU register defines 32 bits, there are 16 domains and 2 attribute bits per
> domain in pkru, for each i (0 â i â 15), PKRU[2i] is the access-disable bit 
> for
> protection key i (ADi); PKRU[2i+1] is the write-disable bit for protection key
> i (WDi). PKEY is index to a defined domain.
> 
> 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.

Thanks, I think this architecture is much better.  More comments inline.

> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index 18d1acf..9cdd607 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -90,6 +90,57 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, 
> int set_dirty)
>      return 0;
>  }
>  
> +extern bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec,
> +        uint32_t pte_flags, uint32_t pte_pkey);
> +#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
> +bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec,
> +        uint32_t pte_flags, uint32_t pte_pkey)
> +{
> +    unsigned int pkru = 0;
> +    bool_t pkru_ad, pkru_wd;
> +
> +    bool_t pf = !!(pfec & PFEC_page_present);
> +    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);
> +
> +    /* When page isn't present,  PKEY isn't checked. */
> +    if ( !pf || is_pv_vcpu(vcpu) )
> +        return 0;
> +
> +    /*
> +     * 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.
> +     */
> +    if ( !hvm_pku_enabled(vcpu) || !hvm_long_mode_enabled(vcpu) ||
> +            rsvdf || ff || !(pte_flags & _PAGE_USER) )
> +        return 0;

See my comment in response to Jan's comment.

> @@ -199,6 +252,9 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>      
>      pse1G = (gflags & _PAGE_PSE) && guest_supports_1G_superpages(v); 
>  
> +    if ( pse1G && pkey_fault(v, pfec, gflags, pkey) )
> +        rc |= _PAGE_PKEY_BITS;
> +
>      if ( pse1G )

So it looks like you're only doing a pkey check on the 'leaf' ptes, is
that right?

In which case we have a lot of duplication here -- duplicate pse1G /
pse2M checks, and exact copies of the call to pkey_fault().  Would it
make sense to move the pkey_fault() check down below the set_ad label?

 -George


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