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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.12.15 at 11:30, <huaitong.han@xxxxxxxxx> wrote:

I dislike having to repeat this: Please trim your Cc lists.

> --- 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)
> +{

See my comments on the previous version. Please avoid sending new
versions without having addressed all comments on the previous one
(verbally or by code changes). Having done the suggested change
just partially (by removing the #ifdef-s from the call sites) you now
do the key check universally, and things remain correct just because
of the long mode check in the middle of the function.

> +    unsigned int pkru = 0;
> +    bool_t pkru_ad, pkru_wd;
> +
> +    bool_t pf = !!(pfec & PFEC_page_present);

There's still this stray blank line above (and I continue to wonder
whether you really need all these boolean variables many of which
get used just once).

> +    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) )

Indentation (I think this also was broken already in the previous
version).

Jan


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