Re: [PATCH 07/10] x86/hvm: pkeys, add functions to support PKRU access/write

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

On 16/11/15 10:31, Huaitong Han wrote:
> This patch adds functions to support PKRU access/write
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
>
> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index f507f5e..427eb84 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -336,6 +336,44 @@ static inline void write_cr4(unsigned long val)
>      asm volatile ( "mov %0,%%cr4" : : "r" (val) );
>  }
>  
> +static inline unsigned int read_pkru(void)
> +{
> +    unsigned int eax, edx;
> +    unsigned int ecx = 0;
> +    unsigned int pkru;
> +
> +    asm volatile(".byte 0x0f,0x01,0xee\n\t"
> +                 : "=a" (eax), "=d" (edx)
> +                 : "c" (ecx));
> +    pkru = eax;
> +    return pkru;

This can be far more simple.

{
unsigned int pkru;

asm volatile (".byte 0x0f,0x01,0xee"
                    : "=a" (pkru) : "c" (0) : "dx");

return pkru;
}

> +}
> +
> +static inline void write_pkru(unsigned int pkru)
> +{
> +    unsigned int eax = pkru;
> +    unsigned int ecx = 0;
> +    unsigned int edx = 0;
> +
> +    asm volatile(".byte 0x0f,0x01,0xef\n\t"
> +                 : : "a" (eax), "c" (ecx), "d" (edx));
> +}

I don't see any need for Xen to use WRPKRU, and this helper isn't used
anywhere in your series.  Please drop it (unless I have overlooked
something).

> +
> +/* macros for pkru */
> +#define PKRU_READ  0
> +#define PKRU_WRITE 1
> +#define PKRU_ATTRS 2
> +
> +/*
> +* 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.
> +*/
> +#define READ_PKRU_AD(x) ((read_pkru() >> (x * PKRU_ATTRS + PKRU_READ)) & 1)
> +#define READ_PKRU_WD(x) ((read_pkru() >> (x * PKRU_ATTRS + PKRU_WRITE)) & 1)

Sorry, but no.  This hides expensive rdpkru instructions in innocuous code.

Instead, a function manipulating the keys should cache rdpkru once, and
use the following helpers:

static inline bool_t pkru_ad(unsigned int pkru, unsigned int key);
static inline bool_t pkru_wd(unsigned int pkru, unsigned int key);

~Andrew