Re: [Xen-devel] [PATCH V6 2/5] x86/hvm: pkeys, add pkeys support for guest_walk_tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.16 at 08:30, <huaitong.han@xxxxxxxxx> wrote:
> Signed-off-by: Huaitong Han <huaitong.han@xxxxxxxxx>
> ---

Please get used to put here per-patch info on what changed from the
previous revision.

> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -90,6 +90,53 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, 
> int set_dirty)
>      return 0;
>  }
>  
> +#if GUEST_PAGING_LEVELS >= 4
> +bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec,

static, especially now that you use >= in the #if.

> +        uint32_t pte_flags, uint32_t pte_pkey)
> +{
> +    unsigned int pkru = 0;
> +    bool_t pkru_ad, pkru_wd;
> +
> +    /* When page isn't present,  PKEY isn't checked. */
> +    if ( !(pfec & PFEC_page_present) || 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 true:
> +     * 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) ||
> +            (pfec & PFEC_reserved_bit) ||

This is only one half of 3 - please add a comment saying that the
other half is already being guaranteed by the caller.

> +            (pfec & PFEC_insn_fetch) ||
> +            !(pte_flags & _PAGE_USER) )

Also for the hole if() - indentation.

> +        return 0;
> +
> +    pkru = read_pkru();
> +    if ( unlikely(pkru) )
> +    {
> +        pkru_ad = read_pkru_ad(pkru, pte_pkey);
> +        pkru_wd = read_pkru_wd(pkru, pte_pkey);

Please make these the declarations (with initializers) of those
variables.

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -93,6 +93,11 @@
>  #define l3e_get_flags(x)           (get_pte_flags((x).l3))
>  #define l4e_get_flags(x)           (get_pte_flags((x).l4))
>  
> +/* Get pte pkeys (unsigned int). */
> +#define l1e_get_pkey(x)           (get_pte_pkey((x).l1))
> +#define l2e_get_pkey(x)           (get_pte_pkey((x).l2))
> +#define l3e_get_pkey(x)           (get_pte_pkey((x).l3))

Despite realizing that other code around here does so too, please
don't make things worse and omit unnecessary parentheses (like
the outer ones here).

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -374,6 +374,46 @@ static always_inline void clear_in_cr4 (unsigned long 
> mask)
>      write_cr4(read_cr4() & ~mask);
>  }
>  
> +static inline unsigned int read_pkru(void)
> +{
> +    unsigned int pkru;
> +    unsigned long cr4 = read_cr4();
> +
> +    /*
> +     * _PAGE_PKEY_BITS have a conflict with _PAGE_GNTTAB used by PV guests,
> +     * so that X86_CR4_PKE is disable on hypervisor, RDPKRU instruction can
> +     * be used with temporarily setting CR4.PKE.
> +     */

... is disabled on hypervisor. To use RDPKRU, CR4.PKE gets
temporarily enabled.


> +    write_cr4(cr4 | X86_CR4_PKE);
> +    asm volatile (".byte 0x0f,0x01,0xee"
> +        : "=a" (pkru) : "c" (0) : "dx");
> +    write_cr4(cr4);

I think you will want to abstract out the actual writing of CR4 from
write_cr4(), as updating this_cpu(cr4) back and forth is quite
pointless here.

> +/*
> + * PKRU defines 32 bits, there are 16 domains and 2 attribute bits per
> + * domain in pkru, pkeys is index to a defined domain, so the value of
> + * pte_pkeys * PKRU_ATTRS + R/W is offset of a defined domain attribute.
> + */
> +static inline bool_t read_pkru_ad(unsigned int pkru, unsigned int pkey)
> +{
> +    ASSERT(pkey < 16);
> +    return (pkru >> (pkey * PKRU_ATTRS + PKRU_READ)) & 1;
> +}
> +
> +static inline bool_t read_pkru_wd(unsigned int pkru, unsigned int pkey)
> +{
> +    ASSERT(pkey < 16);
> +    return (pkru >> (pkey * PKRU_ATTRS + PKRU_WRITE)) & 1;
> +}

I think in both cases the first parameter should be uint32_t, even if
this is a benign change (serving documentation purposes though).

Jan

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