Re: [PATCH 06/10] x86/hvm: pkeys, add functions to get pkeys value from PTE

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/11/15 10:31, Huaitong Han wrote:
> This patch adds functions to get pkeys value from PTE.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
>
> diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
> index 87b3341..1cdbfc8 100644
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -93,6 +93,13 @@
>  #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_pkeys(x)           (get_pte_pkeys((x).l1))
> +#define l2e_get_pkeys(x)           (get_pte_pkeys((x).l2))
> +#define l3e_get_pkeys(x)           (get_pte_pkeys((x).l3))
> +#define l4e_get_pkeys(x)           (get_pte_pkeys((x).l4))
> +
> +

Please only insert a single line here.

>  /* Construct an empty pte. */
>  #define l1e_empty()                ((l1_pgentry_t) { 0 })
>  #define l2e_empty()                ((l2_pgentry_t) { 0 })
> diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
> index 19ab4d0..03418ba 100644
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -134,6 +134,18 @@ typedef l4_pgentry_t root_pgentry_t;
>  #define get_pte_flags(x) (((int)((x) >> 40) & ~0xFFF) | ((int)(x) & 0xFFF))
>  #define put_pte_flags(x) (((intpte_t)((x) & ~0xFFF) << 40) | ((x) & 0xFFF))
>  
> +/*
> +* Protection keys define a new 4-bit protection key field
> +* (PKEY) in bits 62:59 of leaf entries of the page tables.
> +* This corresponds to bit 22:19 of a 24-bit flags.
> +*/

Please align all the *'s of the comment vertically (i.e. with a space on
the very left hand side)

> +#define _PAGE_PKEY_BIT0 19       /* Protection Keys, bit 1/4 */
> +#define _PAGE_PKEY_BIT1 20       /* Protection Keys, bit 2/4 */
> +#define _PAGE_PKEY_BIT2 21       /* Protection Keys, bit 3/4 */
> +#define _PAGE_PKEY_BIT3 22       /* Protection Keys, bit 4/4 */
> +
> +#define get_pte_pkeys(x) ((int)(get_pte_flags(x) >> _PAGE_PKEY_BIT0) & 0xF)

Please implemented these as masks, for consistency with the other
_PAGE_* entries.  I would recommend also having a _SHIFT and _MASK define.

> +
>  /* Bit 23 of a 24-bit flag mask. This corresponds to bit 63 of a pte.*/
>  #define _PAGE_NX_BIT (1U<<23)
>  

There is however another issue. Xen currently uses bit 62 for software
purposes, which is incompatible with enabling PKE.  _PAGE_GNTTAB needs
moving to a different, software-available bit.  I would recommend bit 13
of flags, adjacent to _PAGE_GUEST_KERNEL which is our other
software-used flag.

Also, the changes to _PAGE_GNTTAB must be ahead of patch 4 which enables
CR4.PKE for Xen.

~Andrew