From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 07/10] x86/hvm: pkeys,
	add functions to support PKRU access/write
Date: Mon, 16 Nov 2015 15:09:08 +0000
Message-ID: <5649F194.2020803@citrix.com>
References: <1447669917-17939-1-git-send-email-huaitong.han@intel.com>
	<1447669917-17939-8-git-send-email-huaitong.han@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1447669917-17939-8-git-send-email-huaitong.han@intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Huaitong Han <huaitong.han@intel.com>, jbeulich@suse.com, jun.nakajima@intel.com, eddie.dong@intel.com, kevin.tian@intel.com, george.dunlap@eu.citrix.com, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, wei.liu2@citrix.com, keir@xen.org
Cc: xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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