[V2 PATCH 0/9] x86/hvm: pkeys, add memory protection-key support

[V2 PATCH 0/9] x86/hvm: pkeys, add memory protection-key support

[Huaitong Han]

Changes in v2:
*Rebase all patches in staging branch
*Disable X86_CR4_PKE on hypervisor, and delete pkru_read/write functions, and
use xsave state read to get pkru value.
*Delete the patch that adds pkeys support for do_page_fault.
*Add pkeys support for gva2gfn so that setting _PAGE_PK_BIT in the return
value can get propagated to the guest correctly.

The protection-key feature provides an additional mechanism by which IA-32e
paging controls access to usermode addresses.

Hardware support for protection keys for user pages is enumerated with CPUID
feature flag CPUID.7.0.ECX[3]:PKU. Software support is CPUID.7.0.ECX[4]:OSPKE
with the setting of CR4.PKE(bit 22).

When CR4.PKE = 1, every linear address is associated with the 4-bit protection
key located in bits 62:59 of the paging-structure entry that mapped the page
containing the linear address. The PKRU register determines, for each
protection key, whether user-mode addresses with that protection key may be
read or written.

The PKRU register (protection key rights for user pages) is a 32-bit register
with the following format: for each i (0 ≤ i ≤ 15), PKRU[2i] is the
access-disable bit for protection key i (ADi); PKRU[2i+1] is the write-disable
bit for protection key i (WDi).

Software can use the RDPKRU and WRPKRU instructions with ECX = 0 to read and
write PKRU. In addition, the PKRU register is XSAVE-managed state and can thus
be read and written by instructions in the XSAVE feature set.

PFEC.PK (bit 5) is defined as protection key violations.

The specification of Protection Keys can be found at SDM (4.6.2, volume 3)
http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf.

Huaitong Han (9):
  x86/hvm: pkeys, add pkeys support for cpuid handling
  x86/hvm: pkeys, add the flag to enable Memory Protection Keys
  x86/hvm: pkeys, add pkeys support when setting CR4
  x86/hvm: pkeys, disable pkeys for guests in non-paging mode
  x86/hvm: pkeys, add functions to get pkeys value from PTE
  x86/hvm: pkeys, add functions to support PKRU access
  x86/hvm: pkeys, add pkeys support for guest_walk_tables
  x86/hvm: pkeys, add xstate support for pkeys
  x86/hvm: pkeys, add pkeys support for gva2gfn funcitons

 docs/misc/xen-command-line.markdown | 21 ++++++++++++
 tools/libxc/xc_cpufeature.h         |  2 ++
 tools/libxc/xc_cpuid_x86.c          |  6 ++--
 xen/arch/x86/cpu/common.c           |  5 +--
 xen/arch/x86/hvm/hvm.c              | 32 ++++++++++++++----
 xen/arch/x86/hvm/vmx/vmx.c          | 11 ++++---
 xen/arch/x86/mm/guest_walk.c        | 65 +++++++++++++++++++++++++++++++++++++
 xen/arch/x86/setup.c                |  7 ++++
 xen/arch/x86/xstate.c               | 18 ++++++++++
 xen/include/asm-x86/cpufeature.h    |  7 +++-
 xen/include/asm-x86/guest_pt.h      |  7 ++++
 xen/include/asm-x86/hvm/hvm.h       |  2 ++
 xen/include/asm-x86/page.h          |  5 +++
 xen/include/asm-x86/processor.h     | 15 +++++++++
 xen/include/asm-x86/x86_64/page.h   | 19 +++++++++++
 xen/include/asm-x86/xstate.h        |  5 ++-
 16 files changed, 210 insertions(+), 17 deletions(-)

-- 
2.4.3


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

[V2 PATCH 1/9] x86/hvm: pkeys, add pkeys support for cpuid handling

[Huaitong Han]

This patch adds pkeys support for cpuid handing.

Pkeys hardware support is CPUID.7.0.ECX[3]:PKU. software support is
CPUID.7.0.ECX[4]:OSPKE and it reflects the support setting of CR4.PKE.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 tools/libxc/xc_cpufeature.h      |  2 ++
 tools/libxc/xc_cpuid_x86.c       |  6 ++++--
 xen/arch/x86/cpu/common.c        |  5 +++--
 xen/arch/x86/hvm/hvm.c           | 12 ++++++++++++
 xen/include/asm-x86/cpufeature.h |  7 ++++++-
 5 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_cpufeature.h b/tools/libxc/xc_cpufeature.h
index c3ddc80..f6a9778 100644
--- a/tools/libxc/xc_cpufeature.h
+++ b/tools/libxc/xc_cpufeature.h
@@ -141,5 +141,7 @@
 #define X86_FEATURE_ADX         19 /* ADCX, ADOX instructions */
 #define X86_FEATURE_SMAP        20 /* Supervisor Mode Access Protection */
 
+/* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx) */
+#define X86_FEATURE_PKU     3
 
 #endif /* __LIBXC_CPUFEATURE_H */
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 031c848..5c1d076 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -421,9 +421,11 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
                         bitmaskof(X86_FEATURE_ADX)  |
                         bitmaskof(X86_FEATURE_SMAP) |
                         bitmaskof(X86_FEATURE_FSGSBASE));
+            regs[2] &= bitmaskof(X86_FEATURE_PKU);
         } else
-            regs[1] = 0;
-        regs[0] = regs[2] = regs[3] = 0;
+            regs[1] = regs[2] = 0;
+
+        regs[0] = regs[3] = 0;
         break;
 
     case 0x0000000d:
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e60929d..84d3a10 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -264,8 +264,9 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 	/* Intel-defined flags: level 0x00000007 */
 	if ( c->cpuid_level >= 0x00000007 )
 		cpuid_count(0x00000007, 0, &tmp,
-			    &c->x86_capability[cpufeat_word(X86_FEATURE_FSGSBASE)],
-			    &tmp, &tmp);
+                &c->x86_capability[cpufeat_word(X86_FEATURE_FSGSBASE)],
+                &c->x86_capability[cpufeat_word(X86_FEATURE_PKU)],
+                &tmp);
 }
 
 /*
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ea982e2..0adafe9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4582,6 +4582,18 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
         /* Don't expose INVPCID to non-hap hvm. */
         if ( (count == 0) && !hap_enabled(d) )
             *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
+
+        /* X86_FEATURE_PKU is not yet implemented for shadow paging
+         *
+         * Hypervisor gets guest pkru value from XSAVE state, because
+         * Hypervisor CR4 without X86_CR4_PKE disables RDPKRU instruction.
+         */
+        if ( (count == 0) && (!hap_enabled(d) || !cpu_has_xsave) )
+            *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
+
+        if ( (count == 0) && cpu_has_pku )
+            *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) ?
+                     cpufeat_mask(X86_FEATURE_OSPKE) : 0;
         break;
     case 0xb:
         /* Fix the x2APIC identifier. */
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index af127cf..f041efa 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -11,7 +11,7 @@
 
 #include <xen/const.h>
 
-#define NCAPINTS	8	/* N 32-bit words worth of info */
+#define NCAPINTS	9	/* N 32-bit words worth of info */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (edx), word 0 */
 #define X86_FEATURE_FPU		(0*32+ 0) /* Onboard FPU */
@@ -163,6 +163,10 @@
 #define X86_FEATURE_ADX		(7*32+19) /* ADCX, ADOX instructions */
 #define X86_FEATURE_SMAP	(7*32+20) /* Supervisor Mode Access Prevention */
 
+/* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx), word 8 */
+#define X86_FEATURE_PKU	(8*32+ 3) /* Protection Keys for Userspace */
+#define X86_FEATURE_OSPKE	(8*32+ 4) /* OS Protection Keys Enable */
+
 #define cpufeat_word(idx)	((idx) / 32)
 #define cpufeat_bit(idx)	((idx) % 32)
 #define cpufeat_mask(idx)	(_AC(1, U) << cpufeat_bit(idx))
@@ -199,6 +203,7 @@
 
 #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
 #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
+#define cpu_has_pku             boot_cpu_has(X86_FEATURE_PKU)
 #define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
 
 #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
-- 
2.4.3

Re: [V2 PATCH 1/9] x86/hvm: pkeys, add pkeys support for cpuid handling

[Andrew Cooper]

On 27/11/15 09:51, Huaitong Han wrote:
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index e60929d..84d3a10 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -264,8 +264,9 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
>  	/* Intel-defined flags: level 0x00000007 */
>  	if ( c->cpuid_level >= 0x00000007 )
>  		cpuid_count(0x00000007, 0, &tmp,
> -			    &c->x86_capability[cpufeat_word(X86_FEATURE_FSGSBASE)],
> -			    &tmp, &tmp);
> +                &c->x86_capability[cpufeat_word(X86_FEATURE_FSGSBASE)],
> +                &c->x86_capability[cpufeat_word(X86_FEATURE_PKU)],
> +                &tmp);

You have some indentation issues here.

>  }
>  
>  /*
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ea982e2..0adafe9 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4582,6 +4582,18 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>          /* Don't expose INVPCID to non-hap hvm. */
>          if ( (count == 0) && !hap_enabled(d) )
>              *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> +
> +        /* X86_FEATURE_PKU is not yet implemented for shadow paging
> +         *
> +         * Hypervisor gets guest pkru value from XSAVE state, because
> +         * Hypervisor CR4 without X86_CR4_PKE disables RDPKRU instruction.
> +         */
> +        if ( (count == 0) && (!hap_enabled(d) || !cpu_has_xsave) )
> +            *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
> +
> +        if ( (count == 0) && cpu_has_pku )
> +            *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) ?
> +                     cpufeat_mask(X86_FEATURE_OSPKE) : 0;

This is still buggy.  cpu_has_pku has no relevance to whether OSPKE
becomes visible.

Visibility of OSPKE is determined solely by v->arch.hvm_vcpu.guest_cr[4]
& X86_CR4_PKE and nothing else.

~Andrew

Re: [V2 PATCH 1/9] x86/hvm: pkeys, add pkeys support for cpuid handling

[Jan Beulich]

>>> On 01.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote:
> On 27/11/15 09:51, Huaitong Han wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4582,6 +4582,18 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>>          /* Don't expose INVPCID to non-hap hvm. */
>>          if ( (count == 0) && !hap_enabled(d) )
>>              *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
>> +
>> +        /* X86_FEATURE_PKU is not yet implemented for shadow paging
>> +         *
>> +         * Hypervisor gets guest pkru value from XSAVE state, because
>> +         * Hypervisor CR4 without X86_CR4_PKE disables RDPKRU instruction.
>> +         */
>> +        if ( (count == 0) && (!hap_enabled(d) || !cpu_has_xsave) )
>> +            *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
>> +
>> +        if ( (count == 0) && cpu_has_pku )
>> +            *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) ?
>> +                     cpufeat_mask(X86_FEATURE_OSPKE) : 0;
> 
> This is still buggy.  cpu_has_pku has no relevance to whether OSPKE
> becomes visible.
> 
> Visibility of OSPKE is determined solely by v->arch.hvm_vcpu.guest_cr[4]
> & X86_CR4_PKE and nothing else.

Actually I wouldn't mind guarding against the case where the CR4 flag
is wrongly set for whatever reason, but that ought to check the PKU
bit in *ecx, not the host flag. Same applies to the cpu_has_xsave
check - this too should check the guest flag, not the host one.

Jan

Re: [V2 PATCH 1/9] x86/hvm: pkeys, add pkeys support for cpuid handling

[Han, Huaitong]

On Wed, 2015-12-02 at 04:06 -0700, Jan Beulich wrote:

> > > )
> > > +            *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
> > > +
> > > +        if ( (count == 0) && cpu_has_pku )
> > > +            *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE)
> > > ?
> > > +                     cpufeat_mask(X86_FEATURE_OSPKE) : 0;
> > 
> > This is still buggy.  cpu_has_pku has no relevance to whether OSPKE
> > becomes visible.
> > 
> > Visibility of OSPKE is determined solely by v
> > ->arch.hvm_vcpu.guest_cr[4]
> > & X86_CR4_PKE and nothing else.
> 
> Actually I wouldn't mind guarding against the case where the CR4 flag
> is wrongly set for whatever reason, but that ought to check the PKU
> bit in *ecx, not the host flag. Same applies to the cpu_has_xsave
> check - this too should check the guest flag, not the host one.
I will add guest_cpuid_has_xxx functions.
> 
> Jan
> 

Re: [V2 PATCH 1/9] x86/hvm: pkeys, add pkeys support for cpuid handling

[Jan Beulich]

>>> On 03.12.15 at 02:53, <huaitong.han@intel.com> wrote:
> On Wed, 2015-12-02 at 04:06 -0700, Jan Beulich wrote:
> 
>> > > )
>> > > +            *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
>> > > +
>> > > +        if ( (count == 0) && cpu_has_pku )
>> > > +            *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE)
>> > > ?
>> > > +                     cpufeat_mask(X86_FEATURE_OSPKE) : 0;
>> > 
>> > This is still buggy.  cpu_has_pku has no relevance to whether OSPKE
>> > becomes visible.
>> > 
>> > Visibility of OSPKE is determined solely by v
>> > ->arch.hvm_vcpu.guest_cr[4]
>> > & X86_CR4_PKE and nothing else.
>> 
>> Actually I wouldn't mind guarding against the case where the CR4 flag
>> is wrongly set for whatever reason, but that ought to check the PKU
>> bit in *ecx, not the host flag. Same applies to the cpu_has_xsave
>> check - this too should check the guest flag, not the host one.
> I will add guest_cpuid_has_xxx functions.

If anything, guest_has_ or guest_cpu_has_, but what's wrong with
just (recursively) invoking hvm_cpuid() where needed (as already
done in the 0x80000008 leaf case)?

Jan

Re: [V2 PATCH 1/9] x86/hvm: pkeys, add pkeys support for cpuid handling

[Jan Beulich]

>>> On 27.11.15 at 10:51, <huaitong.han@intel.com> wrote:
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -421,9 +421,11 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
>                          bitmaskof(X86_FEATURE_ADX)  |
>                          bitmaskof(X86_FEATURE_SMAP) |
>                          bitmaskof(X86_FEATURE_FSGSBASE));
> +            regs[2] &= bitmaskof(X86_FEATURE_PKU);
>          } else
> -            regs[1] = 0;
> -        regs[0] = regs[2] = regs[3] = 0;
> +            regs[1] = regs[2] = 0;
> +
> +        regs[0] = regs[3] = 0;
>          break;

This is not a valid change until the hypervisor side implementation is
complete.

Also you should Cc tools maintainers on tools changes.

Jan

[V2 PATCH 2/9] x86/hvm: pkeys, add the flag to enable Memory Protection Keys

[Huaitong Han]

This patch adds the flag to enable Memory Protection Keys.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 docs/misc/xen-command-line.markdown | 21 +++++++++++++++++++++
 xen/arch/x86/setup.c                |  7 +++++++
 2 files changed, 28 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index afb9548..c0bd84d 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1177,6 +1177,27 @@ This option can be specified more than once (up to 8 times at present).
 ### ple\_window
 > `= <integer>`
 
+### pku
+> `= <boolean>`
+
+> Default: `true`
+
+Flag to enable Memory Protection Keys.
+
+The protection-key feature provides an additional mechanism by which IA-32e
+paging controls access to usermode addresses.
+
+When CR4.PKE = 1, every linear address is associated with the 4-bit protection
+key located in bits 62:59 of the paging-structure entry that mapped the page
+containing the linear address. The PKRU register determines, for each
+protection key, whether user-mode addresses with that protection key may be
+read or written.
+
+The PKRU register (protection key rights for user pages) is a 32-bit register
+with the following format: for each i (0 ≤ i ≤ 15), PKRU[2i] is the
+access-disable bit for protection key i (ADi); PKRU[2i+1] is the write-disable
+bit for protection key i (WDi).
+
 ### psr (Intel)
 > `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 6714473..2aa2f83 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -67,6 +67,10 @@ invbool_param("smep", disable_smep);
 static bool_t __initdata disable_smap;
 invbool_param("smap", disable_smap);
 
+/* pku: Flag to enable Memory Protection Keys (default on). */
+static bool_t __initdata opt_pku = 1;
+boolean_param("pku", opt_pku);
+
 /* Boot dom0 in pvh mode */
 static bool_t __initdata opt_dom0pvh;
 boolean_param("dom0pvh", opt_dom0pvh);
@@ -1307,6 +1311,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( cpu_has_smap )
         set_in_cr4(X86_CR4_SMAP);
 
+    if ( !opt_pku )
+        setup_clear_cpu_cap(X86_FEATURE_PKU);
+
     if ( cpu_has_fsgsbase )
         set_in_cr4(X86_CR4_FSGSBASE);
 
-- 
2.4.3


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

Re: [V2 PATCH 2/9] x86/hvm: pkeys, add the flag to enable Memory Protection Keys

[Andrew Cooper]

On 27/11/15 09:51, Huaitong Han wrote:
> This patch adds the flag to enable Memory Protection Keys.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [V2 PATCH 2/9] x86/hvm: pkeys, add the flag to enable Memory Protection Keys

[Jan Beulich]

>>> On 27.11.15 at 10:51, <huaitong.han@intel.com> wrote:
> @@ -1307,6 +1311,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      if ( cpu_has_smap )
>          set_in_cr4(X86_CR4_SMAP);
>  
> +    if ( !opt_pku )
> +        setup_clear_cpu_cap(X86_FEATURE_PKU);
> +
>      if ( cpu_has_fsgsbase )
>          set_in_cr4(X86_CR4_FSGSBASE);

I don't think this is a good place to do this. It really belongs in
cpu/common.c, e.g. next to the xsave disabling. In no case
does it belong in the middle of a series of CR4 updates.

Jan

[V2 PATCH 3/9] x86/hvm: pkeys, add pkeys support when setting CR4

[Huaitong Han]

This patch adds pkeys support when setting CR4.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 xen/arch/x86/hvm/hvm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0adafe9..0103bcb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1924,6 +1924,7 @@ static unsigned long hvm_cr4_guest_reserved_bits(const struct vcpu *v,
         leaf1_edx = boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_VME)];
         leaf1_ecx = boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_PCID)];
         leaf7_0_ebx = boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_FSGSBASE)];
+        leaf7_0_ecx = boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_PKU)];
     }
 
     return ~(unsigned long)
@@ -1959,7 +1960,9 @@ static unsigned long hvm_cr4_guest_reserved_bits(const struct vcpu *v,
              (leaf7_0_ebx & cpufeat_mask(X86_FEATURE_SMEP) ?
               X86_CR4_SMEP : 0) |
              (leaf7_0_ebx & cpufeat_mask(X86_FEATURE_SMAP) ?
-              X86_CR4_SMAP : 0));
+              X86_CR4_SMAP : 0) |
+              (leaf7_0_ecx & cpufeat_mask(X86_FEATURE_PKU) ?
+              X86_CR4_PKE : 0));
 }
 
 static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
-- 
2.4.3

Re: [V2 PATCH 3/9] x86/hvm: pkeys, add pkeys support when setting CR4

[Andrew Cooper]

On 27/11/15 09:51, Huaitong Han wrote:
> This patch adds pkeys support when setting CR4.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[V2 PATCH 4/9] x86/hvm: pkeys, disable pkeys for guests in non-paging mode

[Huaitong Han]

This patch disables pkeys for guest in non-paging mode, However XEN always uses
paging mode to emulate guest non-paging mode, To emulate this behavior, pkeys
needs to be manually disabled when guest switches to non-paging mode.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index eb6248e..4a0c95f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1361,12 +1361,13 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         if ( !hvm_paging_enabled(v) )
         {
             /*
-             * SMEP/SMAP is disabled if CPU is in non-paging mode in hardware.
-             * However Xen always uses paging mode to emulate guest non-paging
-             * mode. To emulate this behavior, SMEP/SMAP needs to be manually
-             * disabled when guest VCPU is in non-paging mode.
+             * SMEP/SMAP/PKU is disabled if CPU is in non-paging mode in
+             * hardware. However Xen always uses paging mode to emulate guest
+             * non-paging mode. To emulate this behavior, SMEP/SMAP/PKU needs
+             * to be manually disabled when guest VCPU is in non-paging mode.
              */
-            v->arch.hvm_vcpu.hw_cr[4] &= ~(X86_CR4_SMEP | X86_CR4_SMAP);
+            v->arch.hvm_vcpu.hw_cr[4] &=
+                ~(X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE);
         }
         __vmwrite(GUEST_CR4, v->arch.hvm_vcpu.hw_cr[4]);
         break;
-- 
2.4.3

[V2 PATCH 5/9] x86/hvm: pkeys, add functions to get pkeys value from PTE

[Huaitong Han]

This patch adds functions to get pkeys value from PTE.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 xen/include/asm-x86/guest_pt.h    |  7 +++++++
 xen/include/asm-x86/page.h        |  5 +++++
 xen/include/asm-x86/x86_64/page.h | 19 +++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/xen/include/asm-x86/guest_pt.h b/xen/include/asm-x86/guest_pt.h
index 3447973..6b0af70 100644
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -154,6 +154,13 @@ static inline u32 guest_l4e_get_flags(guest_l4e_t gl4e)
 { return l4e_get_flags(gl4e); }
 #endif
 
+static inline u32 guest_l1e_get_pkeys(guest_l1e_t gl1e)
+{ return l1e_get_pkeys(gl1e); }
+static inline u32 guest_l2e_get_pkeys(guest_l2e_t gl2e)
+{ return l2e_get_pkeys(gl2e); }
+static inline u32 guest_l3e_get_pkeys(guest_l3e_t gl3e)
+{ return l3e_get_pkeys(gl3e); }
+
 static inline guest_l1e_t guest_l1e_from_gfn(gfn_t gfn, u32 flags)
 { return l1e_from_pfn(gfn_x(gfn), flags); }
 static inline guest_l2e_t guest_l2e_from_gfn(gfn_t gfn, u32 flags)
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index a095a93..93a0db0 100644
--- 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_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))
+
 /* 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..49343ec 100644
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -134,6 +134,25 @@ 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.
+ *
+ * Notice: Bit 22 is used by _PAGE_GNTTAB which is visible to PV guests,
+ * so Protection keys must be disabled on PV guests.
+ */
+#define _PAGE_PKEY_BIT0 (1u<<19)       /* Protection Keys, bit 1/4 */
+#define _PAGE_PKEY_BIT1 (1u<<20)       /* Protection Keys, bit 2/4 */
+#define _PAGE_PKEY_BIT2 (1u<<21)       /* Protection Keys, bit 3/4 */
+#define _PAGE_PKEY_BIT3 (1u<<22)       /* Protection Keys, bit 4/4 */
+
+/* The order of mask _PAGE_PKEY_BIT0 is 19 */
+#define get_pte_pkeys(x) ((int)(get_pte_flags(x) >> 19) & 0xF)
+
+/* Take pkey first bit as pkey feature */
+#define _PAGE_PKEY_BIT _PAGE_PKEY_BIT0
+
 /* Bit 23 of a 24-bit flag mask. This corresponds to bit 63 of a pte.*/
 #define _PAGE_NX_BIT (1U<<23)
 
-- 
2.4.3

Re: [V2 PATCH 5/9] x86/hvm: pkeys, add functions to get pkeys value from PTE

[Andrew Cooper]

On 27/11/15 09:51, Huaitong Han wrote:
> diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
> index 19ab4d0..49343ec 100644
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -134,6 +134,25 @@ 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.
> + *
> + * Notice: Bit 22 is used by _PAGE_GNTTAB which is visible to PV guests,
> + * so Protection keys must be disabled on PV guests.
> + */
> +#define _PAGE_PKEY_BIT0 (1u<<19)       /* Protection Keys, bit 1/4 */
> +#define _PAGE_PKEY_BIT1 (1u<<20)       /* Protection Keys, bit 2/4 */
> +#define _PAGE_PKEY_BIT2 (1u<<21)       /* Protection Keys, bit 3/4 */
> +#define _PAGE_PKEY_BIT3 (1u<<22)       /* Protection Keys, bit 4/4 */
> +
> +/* The order of mask _PAGE_PKEY_BIT0 is 19 */
> +#define get_pte_pkeys(x) ((int)(get_pte_flags(x) >> 19) & 0xF)
> +
> +/* Take pkey first bit as pkey feature */
> +#define _PAGE_PKEY_BIT _PAGE_PKEY_BIT0

You have not addressed Jan's feedback from v1.

~Andrew

Re: [V2 PATCH 5/9] x86/hvm: pkeys, add functions to get pkeys value from PTE

[Jan Beulich]

>>> On 27.11.15 at 10:51, <huaitong.han@intel.com> wrote:
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -134,6 +134,25 @@ 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.
> + *
> + * Notice: Bit 22 is used by _PAGE_GNTTAB which is visible to PV guests,
> + * so Protection keys must be disabled on PV guests.
> + */
> +#define _PAGE_PKEY_BIT0 (1u<<19)       /* Protection Keys, bit 1/4 */
> +#define _PAGE_PKEY_BIT1 (1u<<20)       /* Protection Keys, bit 2/4 */
> +#define _PAGE_PKEY_BIT2 (1u<<21)       /* Protection Keys, bit 3/4 */
> +#define _PAGE_PKEY_BIT3 (1u<<22)       /* Protection Keys, bit 4/4 */
> +
> +/* The order of mask _PAGE_PKEY_BIT0 is 19 */
> +#define get_pte_pkeys(x) ((int)(get_pte_flags(x) >> 19) & 0xF)

Pointless cast.

Jan

[V2 PATCH 6/9] x86/hvm: pkeys, add functions to support PKRU access

[Huaitong Han]

This patch adds functions to support PKRU access.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 xen/include/asm-x86/processor.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 3f8411f..68d86cb 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -342,6 +342,21 @@ static inline void write_cr4(unsigned long val)
     asm volatile ( "mov %0,%%cr4" : : "r" (val) );
 }
 
+/* Macros for PKRU domain */
+#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(pkru, pkey) \
+    ((pkru >> (pkey * PKRU_ATTRS + PKRU_READ)) & 1)
+#define READ_PKRU_WD(pkru, pkey) \
+    ((pkru >> (pkey * PKRU_ATTRS + PKRU_WRITE)) & 1)
+
 /* Clear and set 'TS' bit respectively */
 static inline void clts(void) 
 {
-- 
2.4.3

Re: [V2 PATCH 6/9] x86/hvm: pkeys, add functions to support PKRU access

[Andrew Cooper]

On 27/11/15 09:52, Huaitong Han wrote:
> This patch adds functions to support PKRU access.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
> ---
>  xen/include/asm-x86/processor.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index 3f8411f..68d86cb 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -342,6 +342,21 @@ static inline void write_cr4(unsigned long val)
>      asm volatile ( "mov %0,%%cr4" : : "r" (val) );
>  }
>  
> +/* Macros for PKRU domain */
> +#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(pkru, pkey) \
> +    ((pkru >> (pkey * PKRU_ATTRS + PKRU_READ)) & 1)
> +#define READ_PKRU_WD(pkru, pkey) \
> +    ((pkru >> (pkey * PKRU_ATTRS + PKRU_WRITE)) & 1)

Both macro parameters need quoting, but these would be better still as
static inline functions.

~Andrew

[V2 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Huaitong Han]

This patch adds pkeys support for guest_walk_tables.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 xen/arch/x86/mm/guest_walk.c  | 65 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 2 files changed, 67 insertions(+)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index 18d1acf..3e443b3 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -31,6 +31,8 @@ asm(".file \"" __OBJECT_FILE__ "\"");
 #include <xen/sched.h>
 #include <asm/page.h>
 #include <asm/guest_pt.h>
+#include <asm/xstate.h>
+#include <asm/i387.h>
 
 extern const uint32_t gw_page_flags[];
 #if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
@@ -90,6 +92,53 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty)
     return 0;
 }
 
+#if GUEST_PAGING_LEVELS >= 4
+uint32_t leaf_pte_pkeys_check(struct vcpu *vcpu, uint32_t pfec,
+                uint32_t pte_access, uint32_t pte_pkeys)
+{
+    bool_t pkru_ad, pkru_wd;
+    bool_t ff, wf, uf, rsvdf, pkuf;
+    unsigned int pkru = 0;
+
+    uf = pfec & PFEC_user_mode;
+    wf = pfec & PFEC_write_access;
+    rsvdf = pfec & PFEC_reserved_bit;
+    ff = pfec & PFEC_insn_fetch;
+    pkuf = pfec & PFEC_prot_key;
+
+    if ( !cpu_has_xsave || !pkuf || is_pv_vcpu(vcpu) )
+        return 0;
+
+    vcpu_save_fpu(vcpu);
+    pkru = *(unsigned int*)get_xsave_addr(vcpu->arch.xsave_area, XSTATE_PKRU);
+    if ( unlikely(pkru) )
+    {
+        /*
+         * 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.
+         */
+        pkru_ad = READ_PKRU_AD(pkru, pte_pkeys);
+        pkru_wd = READ_PKRU_AD(pkru, pte_pkeys);
+        if ( hvm_pku_enabled(vcpu) && hvm_long_mode_enabled(vcpu) &&
+            !rsvdf && !ff && (pkru_ad ||
+            (pkru_wd && wf && (hvm_wp_enabled(vcpu) || uf))))
+            return 1;
+    }
+
+    return 0;
+}
+#endif
+
 /* Walk the guest pagetables, after the manner of a hardware walker. */
 /* Because the walk is essentially random, it can cause a deadlock 
  * warning in the p2m locking code. Highly unlikely this is an actual
@@ -106,6 +155,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
 #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
     guest_l3e_t *l3p = NULL;
     guest_l4e_t *l4p;
+    uint32_t pkeys;
 #endif
     uint32_t gflags, mflags, iflags, rc = 0;
     bool_t smep = 0, smap = 0;
@@ -190,6 +240,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
         goto out;
     /* Get the l3e and check its flags*/
     gw->l3e = l3p[guest_l3_table_offset(va)];
+    pkeys = guest_l3e_get_pkeys(gw->l3e);
     gflags = guest_l3e_get_flags(gw->l3e) ^ iflags;
     if ( !(gflags & _PAGE_PRESENT) ) {
         rc |= _PAGE_PRESENT;
@@ -199,6 +250,9 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
     
     pse1G = (gflags & _PAGE_PSE) && guest_supports_1G_superpages(v); 
 
+    if (pse1G && leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
+        rc |= _PAGE_PKEY_BIT;
+
     if ( pse1G )
     {
         /* Generate a fake l1 table entry so callers don't all 
@@ -270,6 +324,12 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
 
     pse2M = (gflags & _PAGE_PSE) && guest_supports_superpages(v); 
 
+#if GUEST_PAGING_LEVELS >= 4
+    pkeys = guest_l2e_get_pkeys(gw->l2e);
+    if (pse2M && leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
+        rc |= _PAGE_PKEY_BIT;
+#endif
+
     if ( pse2M )
     {
         /* Special case: this guest VA is in a PSE superpage, so there's
@@ -330,6 +390,11 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
             goto out;
         }
         rc |= ((gflags & mflags) ^ mflags);
+#if GUEST_PAGING_LEVELS >= 4
+        pkeys = guest_l1e_get_pkeys(gw->l1e);
+        if (leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
+            rc |= _PAGE_PKEY_BIT;
+#endif
     }
 
 #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index da799a0..cfbb1ef 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -276,6 +276,8 @@ int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode);
     (hvm_paging_enabled(v) && ((v)->arch.hvm_vcpu.guest_cr[4] & X86_CR4_SMAP))
 #define hvm_nx_enabled(v) \
     (!!((v)->arch.hvm_vcpu.guest_efer & EFER_NX))
+#define hvm_pku_enabled(v) \
+    (hvm_paging_enabled(v) && ((v)->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE))
 
 /* Can we use superpages in the HAP p2m table? */
 #define hap_has_1gb (!!(hvm_funcs.hap_capabilities & HVM_HAP_SUPERPAGE_1GB))
-- 
2.4.3

Re: [V2 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Andrew Cooper]

On 27/11/15 09:52, Huaitong Han wrote:
> This patch adds pkeys support for guest_walk_tables.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>

You must CC the appropriate maintainer for this patch, which includes
the x86 MM maintainer.

> ---
>  xen/arch/x86/mm/guest_walk.c  | 65 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/hvm.h |  2 ++
>  2 files changed, 67 insertions(+)
>
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index 18d1acf..3e443b3 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -31,6 +31,8 @@ asm(".file \"" __OBJECT_FILE__ "\"");
>  #include <xen/sched.h>
>  #include <asm/page.h>
>  #include <asm/guest_pt.h>
> +#include <asm/xstate.h>
> +#include <asm/i387.h>

I can see why you need xstate.h, but I why do you need i387.h ?

>  
>  extern const uint32_t gw_page_flags[];
>  #if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
> @@ -90,6 +92,53 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty)
>      return 0;
>  }
>  
> +#if GUEST_PAGING_LEVELS >= 4
> +uint32_t leaf_pte_pkeys_check(struct vcpu *vcpu, uint32_t pfec,
> +                uint32_t pte_access, uint32_t pte_pkeys)

This is a latent linking bug for the future when 5 levels comes along.

It will probably be best to use the same trick as gw_page_flags to
compile it once but use it multiple times.

> +{
> +    bool_t pkru_ad, pkru_wd;
> +    bool_t ff, wf, uf, rsvdf, pkuf;
> +    unsigned int pkru = 0;
> +
> +    uf = pfec & PFEC_user_mode;
> +    wf = pfec & PFEC_write_access;
> +    rsvdf = pfec & PFEC_reserved_bit;
> +    ff = pfec & PFEC_insn_fetch;
> +    pkuf = pfec & PFEC_prot_key;
> +
> +    if ( !cpu_has_xsave || !pkuf || is_pv_vcpu(vcpu) )
> +        return 0;
> +
> +    vcpu_save_fpu(vcpu);
> +    pkru = *(unsigned int*)get_xsave_addr(vcpu->arch.xsave_area, XSTATE_PKRU);

Style.

> +    if ( unlikely(pkru) )
> +    {
> +        /*
> +         * 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.
> +         */
> +        pkru_ad = READ_PKRU_AD(pkru, pte_pkeys);
> +        pkru_wd = READ_PKRU_AD(pkru, pte_pkeys);
> +        if ( hvm_pku_enabled(vcpu) && hvm_long_mode_enabled(vcpu) &&
> +            !rsvdf && !ff && (pkru_ad ||
> +            (pkru_wd && wf && (hvm_wp_enabled(vcpu) || uf))))
> +            return 1;
> +    }
> +
> +    return 0;
> +}
> +#endif
> +
>  /* Walk the guest pagetables, after the manner of a hardware walker. */
>  /* Because the walk is essentially random, it can cause a deadlock 
>   * warning in the p2m locking code. Highly unlikely this is an actual
> @@ -106,6 +155,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>  #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
>      guest_l3e_t *l3p = NULL;
>      guest_l4e_t *l4p;
> +    uint32_t pkeys;
>  #endif
>      uint32_t gflags, mflags, iflags, rc = 0;
>      bool_t smep = 0, smap = 0;
> @@ -190,6 +240,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>          goto out;
>      /* Get the l3e and check its flags*/
>      gw->l3e = l3p[guest_l3_table_offset(va)];
> +    pkeys = guest_l3e_get_pkeys(gw->l3e);
>      gflags = guest_l3e_get_flags(gw->l3e) ^ iflags;
>      if ( !(gflags & _PAGE_PRESENT) ) {
>          rc |= _PAGE_PRESENT;
> @@ -199,6 +250,9 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>      
>      pse1G = (gflags & _PAGE_PSE) && guest_supports_1G_superpages(v); 
>  
> +    if (pse1G && leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
> +        rc |= _PAGE_PKEY_BIT;
> +
>      if ( pse1G )
>      {
>          /* Generate a fake l1 table entry so callers don't all 
> @@ -270,6 +324,12 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>  
>      pse2M = (gflags & _PAGE_PSE) && guest_supports_superpages(v); 
>  
> +#if GUEST_PAGING_LEVELS >= 4
> +    pkeys = guest_l2e_get_pkeys(gw->l2e);
> +    if (pse2M && leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
> +        rc |= _PAGE_PKEY_BIT;
> +#endif
> +
>      if ( pse2M )
>      {
>          /* Special case: this guest VA is in a PSE superpage, so there's
> @@ -330,6 +390,11 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>              goto out;
>          }
>          rc |= ((gflags & mflags) ^ mflags);
> +#if GUEST_PAGING_LEVELS >= 4
> +        pkeys = guest_l1e_get_pkeys(gw->l1e);
> +        if (leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
> +            rc |= _PAGE_PKEY_BIT;
> +#endif

As I identified in v1, the fact that you do not modify the callers of
guest_walk_tables() proves that this change is buggy.  You must modify
the callers to cope with the new error of _PAGE_PKEY_BIT.

~Andrew

Re: [V2 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Han, Huaitong]

On Tue, 2015-12-01 at 20:30 +0000, Andrew Cooper wrote:
> > +#include <asm/i387.h>
> 
> I can see why you need xstate.h, but I why do you need i387.h ?
Use vcpu_save_fpu functions.

> > +
> >      if ( pse2M )
> >      {
> >          /* Special case: this guest VA is in a PSE superpage, so
> > there's
> > @@ -330,6 +390,11 @@ guest_walk_tables(struct vcpu *v, struct
> > p2m_domain *p2m,
> >              goto out;
> >          }
> >          rc |= ((gflags & mflags) ^ mflags);
> > +#if GUEST_PAGING_LEVELS >= 4
> > +        pkeys = guest_l1e_get_pkeys(gw->l1e);
> > +        if (leaf_pte_pkeys_check(v, pfec, gflags, pkeys))
> > +            rc |= _PAGE_PKEY_BIT;
> > +#endif
> 
> As I identified in v1, the fact that you do not modify the callers of
> guest_walk_tables() proves that this change is buggy.  You must
> modify
> the callers to cope with the new error of _PAGE_PKEY_BIT.
_PAGE_PKEY_BIT just a flag to tell
 hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS) there is a page fault, PK is
real physical fault which is included in pfec, the funtion of
_PAGE_PKEY_BIT is that hap_p2m_ga_to_gfn returns INVALID_GFN, you can
find the pfec modification on PATCH 9/9.

> 
> ~Andrew

Re: [V2 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Jan Beulich]

>>> On 02.12.15 at 09:23, <huaitong.han@intel.com> wrote:
> On Tue, 2015-12-01 at 20:30 +0000, Andrew Cooper wrote:
>> > +#include <asm/i387.h>
>> 
>> I can see why you need xstate.h, but I why do you need i387.h ?
> Use vcpu_save_fpu functions.

Which is bogus by itself: That function isn't meant to be used for
purposes like the one you have, e.g. due to its side effect of
clearing ->fpu_dirtied. You really ought to be using a lower level
function here (and I don't think the corresponding struct vcpu
should get altered in any way).

Jan

Re: [V2 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Jan Beulich]

>>> On 27.11.15 at 10:52, <huaitong.han@intel.com> wrote:
> @@ -90,6 +92,53 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty)
>      return 0;
>  }
>  
> +#if GUEST_PAGING_LEVELS >= 4
> +uint32_t leaf_pte_pkeys_check(struct vcpu *vcpu, uint32_t pfec,
> +                uint32_t pte_access, uint32_t pte_pkeys)
> +{
> +    bool_t pkru_ad, pkru_wd;
> +    bool_t ff, wf, uf, rsvdf, pkuf;
> +    unsigned int pkru = 0;
> +
> +    uf = pfec & PFEC_user_mode;
> +    wf = pfec & PFEC_write_access;
> +    rsvdf = pfec & PFEC_reserved_bit;
> +    ff = pfec & PFEC_insn_fetch;
> +    pkuf = pfec & PFEC_prot_key;

None of these operations yield valid bool_t values. All of them
should be using !!(), and readability would imo benefit if you
made the expressions initializers instead of assignments.

Jan

Re: [V2 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Jan Beulich]

>>> On 27.11.15 at 10:52, <huaitong.han@intel.com> wrote:
> @@ -90,6 +92,53 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty)
>      return 0;
>  }
>  
> +#if GUEST_PAGING_LEVELS >= 4
> +uint32_t leaf_pte_pkeys_check(struct vcpu *vcpu, uint32_t pfec,
> +                uint32_t pte_access, uint32_t pte_pkeys)
> +{
> +    bool_t pkru_ad, pkru_wd;
> +    bool_t ff, wf, uf, rsvdf, pkuf;
> +    unsigned int pkru = 0;

Pointless initializer.

> +    uf = pfec & PFEC_user_mode;
> +    wf = pfec & PFEC_write_access;
> +    rsvdf = pfec & PFEC_reserved_bit;
> +    ff = pfec & PFEC_insn_fetch;
> +    pkuf = pfec & PFEC_prot_key;
> +
> +    if ( !cpu_has_xsave || !pkuf || is_pv_vcpu(vcpu) )
> +        return 0;
> +
> +    vcpu_save_fpu(vcpu);
> +    pkru = *(unsigned int*)get_xsave_addr(vcpu->arch.xsave_area, XSTATE_PKRU);

I.e. you really want to _only_ save that one component.

> @@ -106,6 +155,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>  #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
>      guest_l3e_t *l3p = NULL;
>      guest_l4e_t *l4p;
> +    uint32_t pkeys;

No reason I can see to use a fixed width type here - unsigned int
ought to be fine.

> @@ -199,6 +250,9 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>      
>      pse1G = (gflags & _PAGE_PSE) && guest_supports_1G_superpages(v); 
>  
> +    if (pse1G && leaf_pte_pkeys_check(v, pfec, gflags, pkeys))

Coding style (more further down).

Jan

[V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Huaitong Han]

This patch adds xstate support for pkeys.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 xen/arch/x86/xstate.c        | 18 ++++++++++++++++++
 xen/include/asm-x86/xstate.h |  5 ++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 827e0e1..00bddb0 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -294,6 +294,24 @@ unsigned int xstate_ctxt_size(u64 xcr0)
     return _xstate_ctxt_size(xcr0);
 }
 
+/*
+ * Given the xsave area and a state inside, this function returns the
+ * address of the state.
+ *
+ * This is the API that is called to get xstate address in standard format.
+ * Just because XSAVE function does not use compacted format of xsave
+ * area.
+ */
+void *get_xsave_addr(struct xsave_struct *xsave, u32 xfeature)
+{
+    u32 xstate_offsets, xstate_sizes, ecx, edx;
+    u32 xstate_nr = fls64(xfeature) - 1;
+
+    cpuid_count(XSTATE_CPUID, xstate_nr, &xstate_sizes, &xstate_offsets, &ecx, &edx);
+
+    return (void *)xsave + xstate_offsets;
+}
+
 /* Collect the information of processor's extended state */
 void xstate_init(struct cpuinfo_x86 *c)
 {
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index b95a5b5..e9abe71 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -34,13 +34,15 @@
 #define XSTATE_OPMASK  (1ULL << 5)
 #define XSTATE_ZMM     (1ULL << 6)
 #define XSTATE_HI_ZMM  (1ULL << 7)
+#define XSTATE_UNUSED  (1ULL << 8)
+#define XSTATE_PKRU    (1ULL << 9)
 #define XSTATE_LWP     (1ULL << 62) /* AMD lightweight profiling */
 #define XSTATE_FP_SSE  (XSTATE_FP | XSTATE_SSE)
 #define XCNTXT_MASK    (XSTATE_FP | XSTATE_SSE | XSTATE_YMM | XSTATE_OPMASK | \
                         XSTATE_ZMM | XSTATE_HI_ZMM | XSTATE_NONLAZY)
 
 #define XSTATE_ALL     (~(1ULL << 63))
-#define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR)
+#define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR | XSTATE_PKRU)
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
@@ -89,6 +91,7 @@ uint64_t get_msr_xss(void);
 void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 bool_t xsave_enabled(const struct vcpu *v);
+void *get_xsave_addr(struct xsave_struct *xsave, u32 xfeature);
 int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv);
 int __must_check handle_xsetbv(u32 index, u64 new_bv);
 
-- 
2.4.3

Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Andrew Cooper]

On 27/11/15 09:52, Huaitong Han wrote:
> This patch adds xstate support for pkeys.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
> ---
>  xen/arch/x86/xstate.c        | 18 ++++++++++++++++++
>  xen/include/asm-x86/xstate.h |  5 ++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index 827e0e1..00bddb0 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -294,6 +294,24 @@ unsigned int xstate_ctxt_size(u64 xcr0)
>      return _xstate_ctxt_size(xcr0);
>  }
>  
> +/*
> + * Given the xsave area and a state inside, this function returns the
> + * address of the state.
> + *
> + * This is the API that is called to get xstate address in standard format.
> + * Just because XSAVE function does not use compacted format of xsave
> + * area.
> + */
> +void *get_xsave_addr(struct xsave_struct *xsave, u32 xfeature)
> +{
> +    u32 xstate_offsets, xstate_sizes, ecx, edx;
> +    u32 xstate_nr = fls64(xfeature) - 1;
> +
> +    cpuid_count(XSTATE_CPUID, xstate_nr, &xstate_sizes, &xstate_offsets, &ecx, &edx);
> +
> +    return (void *)xsave + xstate_offsets;
> +}
> +

Does this even compile?  There is already

static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)

higher in the same file.

That function should be augmented to take a struct xsave_struct *xsave,
look at whether the representation is compressed or not, and use the
appropriate offset array.

>  /* Collect the information of processor's extended state */
>  void xstate_init(struct cpuinfo_x86 *c)
>  {
> diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
> index b95a5b5..e9abe71 100644
> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -34,13 +34,15 @@
>  #define XSTATE_OPMASK  (1ULL << 5)
>  #define XSTATE_ZMM     (1ULL << 6)
>  #define XSTATE_HI_ZMM  (1ULL << 7)
> +#define XSTATE_UNUSED  (1ULL << 8)

No need for this.

~Andrew

Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Han, Huaitong]

> Does this even compile?  There is already
> 
> static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)
> 
> higher in the same file.
> 
> That function should be augmented to take a struct xsave_struct
> *xsave,
> look at whether the representation is compressed or not, and use the
> appropriate offset array.
> 
Just because I have pulled staging branch when "static void
*get_xsave_addr(void *xsave, unsigned int xfeature_idx)" is not added.

Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Jan Beulich]

>>> On 02.12.15 at 08:20, <huaitong.han@intel.com> wrote:
>> Does this even compile?  There is already
>> 
>> static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)
>> 
>> higher in the same file.
>> 
>> That function should be augmented to take a struct xsave_struct
>> *xsave,
>> look at whether the representation is compressed or not, and use the
>> appropriate offset array.
>> 
> Just because I have pulled staging branch when "static void
> *get_xsave_addr(void *xsave, unsigned int xfeature_idx)" is not added.

Yet you use the function on patch 7 already...

Jan

Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Han, Huaitong]

On Wed, 2015-12-02 at 04:31 -0700, Jan Beulich wrote:
> > > > On 02.12.15 at 08:20, <huaitong.han@intel.com> wrote:
> > > Does this even compile?  There is already
> > > 
> > > static void *get_xsave_addr(void *xsave, unsigned int
> > > xfeature_idx)
> > > 
> > > higher in the same file.
> > > 
> > > That function should be augmented to take a struct xsave_struct
> > > *xsave,
> > > look at whether the representation is compressed or not, and use
> > > the
> > > appropriate offset array.
> > > 
> > Just because I have pulled staging branch when "static void
> > *get_xsave_addr(void *xsave, unsigned int xfeature_idx)" is not
> > added.
> 
> Yet you use the function on patch 7 already...
The function used by patch 7 has same name but is added by me.
> 
> Jan
> 

Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Jan Beulich]

>>> On 03.12.15 at 02:58, <huaitong.han@intel.com> wrote:
> On Wed, 2015-12-02 at 04:31 -0700, Jan Beulich wrote:
>> > > > On 02.12.15 at 08:20, <huaitong.han@intel.com> wrote:
>> > > Does this even compile?  There is already
>> > > 
>> > > static void *get_xsave_addr(void *xsave, unsigned int
>> > > xfeature_idx)
>> > > 
>> > > higher in the same file.
>> > > 
>> > > That function should be augmented to take a struct xsave_struct
>> > > *xsave,
>> > > look at whether the representation is compressed or not, and use
>> > > the
>> > > appropriate offset array.
>> > > 
>> > Just because I have pulled staging branch when "static void
>> > *get_xsave_addr(void *xsave, unsigned int xfeature_idx)" is not
>> > added.
>> 
>> Yet you use the function on patch 7 already...
> The function used by patch 7 has same name but is added by me.

Seems like you don't get the point: You can't use in patch 7 a
function that you only add in patch 8.

Jan

Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Andrew Cooper]

On 03/12/15 01:58, Han, Huaitong wrote:
> On Wed, 2015-12-02 at 04:31 -0700, Jan Beulich wrote:
>>>>> On 02.12.15 at 08:20, <huaitong.han@intel.com> wrote:
>>>> Does this even compile?  There is already
>>>>
>>>> static void *get_xsave_addr(void *xsave, unsigned int
>>>> xfeature_idx)
>>>>
>>>> higher in the same file.
>>>>
>>>> That function should be augmented to take a struct xsave_struct
>>>> *xsave,
>>>> look at whether the representation is compressed or not, and use
>>>> the
>>>> appropriate offset array.
>>>>
>>> Just because I have pulled staging branch when "static void
>>> *get_xsave_addr(void *xsave, unsigned int xfeature_idx)" is not
>>> added.
>> Yet you use the function on patch 7 already...
> The function used by patch 7 has same name but is added by me.

You must ensure that your patch series compiles and functions correctly
at each patch boundary.  This is for bisectability.

~Andrew

[V2 PATCH 9/9] x86/hvm: pkeys, add pkeys support for gva2gfn funcitons

[Huaitong Han]

 This patch adds pkeys support for gva2gfn funcitons.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>
---
 xen/arch/x86/hvm/hvm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0103bcb..b28d104 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4304,7 +4304,8 @@ static enum hvm_copy_result __hvm_clear(paddr_t addr, int size)
     p2m_type_t p2mt;
     char *p;
     int count, todo = size;
-    uint32_t pfec = PFEC_page_present | PFEC_write_access;
+    uint32_t pfec = PFEC_page_present | PFEC_write_access |
+        hvm_pku_enabled(curr) ? PFEC_prot_key : 0;
 
     /*
      * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
@@ -4405,7 +4406,8 @@ enum hvm_copy_result hvm_copy_to_guest_virt(
 {
     return __hvm_copy(buf, vaddr, size,
                       HVMCOPY_to_guest | HVMCOPY_fault | HVMCOPY_virt,
-                      PFEC_page_present | PFEC_write_access | pfec);
+                      PFEC_page_present | PFEC_write_access | pfec |
+                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
 }
 
 enum hvm_copy_result hvm_copy_from_guest_virt(
@@ -4413,7 +4415,8 @@ enum hvm_copy_result hvm_copy_from_guest_virt(
 {
     return __hvm_copy(buf, vaddr, size,
                       HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_virt,
-                      PFEC_page_present | pfec);
+                      PFEC_page_present | pfec |
+                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
 }
 
 enum hvm_copy_result hvm_fetch_from_guest_virt(
@@ -4431,7 +4434,8 @@ enum hvm_copy_result hvm_copy_to_guest_virt_nofault(
 {
     return __hvm_copy(buf, vaddr, size,
                       HVMCOPY_to_guest | HVMCOPY_no_fault | HVMCOPY_virt,
-                      PFEC_page_present | PFEC_write_access | pfec);
+                      PFEC_page_present | PFEC_write_access | pfec |
+                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
 }
 
 enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
@@ -4439,7 +4443,8 @@ enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
 {
     return __hvm_copy(buf, vaddr, size,
                       HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_virt,
-                      PFEC_page_present | pfec);
+                      PFEC_page_present | pfec |
+                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
 }
 
 enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(
-- 
2.4.3

Re: [V2 PATCH 9/9] x86/hvm: pkeys, add pkeys support for gva2gfn funcitons

[Jan Beulich]

 >>> On 27.11.15 at 10:52, <huaitong.han@intel.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4304,7 +4304,8 @@ static enum hvm_copy_result __hvm_clear(paddr_t addr, int size)
>      p2m_type_t p2mt;
>      char *p;
>      int count, todo = size;
> -    uint32_t pfec = PFEC_page_present | PFEC_write_access;
> +    uint32_t pfec = PFEC_page_present | PFEC_write_access |
> +        hvm_pku_enabled(curr) ? PFEC_prot_key : 0;
>  
>      /*
>       * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
> @@ -4405,7 +4406,8 @@ enum hvm_copy_result hvm_copy_to_guest_virt(
>  {
>      return __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_to_guest | HVMCOPY_fault | HVMCOPY_virt,
> -                      PFEC_page_present | PFEC_write_access | pfec);
> +                      PFEC_page_present | PFEC_write_access | pfec |
> +                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
>  }
>  
>  enum hvm_copy_result hvm_copy_from_guest_virt(
> @@ -4413,7 +4415,8 @@ enum hvm_copy_result hvm_copy_from_guest_virt(
>  {
>      return __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | pfec |
> +                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
>  }
>  
>  enum hvm_copy_result hvm_fetch_from_guest_virt(
> @@ -4431,7 +4434,8 @@ enum hvm_copy_result hvm_copy_to_guest_virt_nofault(
>  {
>      return __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_to_guest | HVMCOPY_no_fault | HVMCOPY_virt,
> -                      PFEC_page_present | PFEC_write_access | pfec);
> +                      PFEC_page_present | PFEC_write_access | pfec |
> +                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
>  }
>  
>  enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
> @@ -4439,7 +4443,8 @@ enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
>  {
>      return __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | pfec |
> +                      hvm_pku_enabled(current) ? PFEC_prot_key : 0);
>  }
>  
>  enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(

Was this patch tested at all? The lack of parentheses in all the
changes you make result - afaict - in PFEC_prot_key to be
unconditionally passed to __hvm_copy(), which can't be right.

Jan

Re: [V2 PATCH 9/9] x86/hvm: pkeys, add pkeys support for gva2gfn funcitons

[Han, Huaitong]

On Wed, 2015-12-02 at 04:35 -0700, Jan Beulich wrote:
>  >>> On 27.11.15 at 10:52, <huaitong.han@intel.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4304,7 +4304,8 @@ static enum hvm_copy_result
> > __hvm_clear(paddr_t addr, int size)
> >      p2m_type_t p2mt;
> >      char *p;
> >      int count, todo = size;
> > -    uint32_t pfec = PFEC_page_present | PFEC_write_access;
> > +    uint32_t pfec = PFEC_page_present | PFEC_write_access |
> > +        hvm_pku_enabled(curr) ? PFEC_prot_key : 0;
> >  
> >      /*
> >       * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant
> > -table ops
> > @@ -4405,7 +4406,8 @@ enum hvm_copy_result hvm_copy_to_guest_virt(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_to_guest | HVMCOPY_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | PFEC_write_access |
> > pfec);
> > +                      PFEC_page_present | PFEC_write_access | pfec
> > |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_copy_from_guest_virt(
> > @@ -4413,7 +4415,8 @@ enum hvm_copy_result
> > hvm_copy_from_guest_virt(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_from_guest | HVMCOPY_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | pfec);
> > +                      PFEC_page_present | pfec |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_fetch_from_guest_virt(
> > @@ -4431,7 +4434,8 @@ enum hvm_copy_result
> > hvm_copy_to_guest_virt_nofault(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_to_guest | HVMCOPY_no_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | PFEC_write_access |
> > pfec);
> > +                      PFEC_page_present | PFEC_write_access | pfec
> > |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
> > @@ -4439,7 +4443,8 @@ enum hvm_copy_result
> > hvm_copy_from_guest_virt_nofault(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_from_guest | HVMCOPY_no_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | pfec);
> > +                      PFEC_page_present | pfec |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(
> 
> Was this patch tested at all? The lack of parentheses in all the
> changes you make result - afaict - in PFEC_prot_key to be
> unconditionally passed to __hvm_copy(), which can't be right.
Yes, the patch can work, I understand, if the pfec parameter of __hvm_c
opy is zero, it means that memory permission check is not to be
required, when pfec is not 0, PKRU has access disable and write
disable, so, PFEC_prot_key is unconditionally passed to the functions.

Thanks
Huaitong.

Re: [V2 PATCH 9/9] x86/hvm: pkeys, add pkeys support for gva2gfn funcitons

[Jan Beulich]

>>> On 03.12.15 at 09:50, <huaitong.han@intel.com> wrote:
> On Wed, 2015-12-02 at 04:35 -0700, Jan Beulich wrote:
>>  >>> On 27.11.15 at 10:52, <huaitong.han@intel.com> wrote:
>> > @@ -4439,7 +4443,8 @@ enum hvm_copy_result
>> > hvm_copy_from_guest_virt_nofault(
>> >  {
>> >      return __hvm_copy(buf, vaddr, size,
>> >                        HVMCOPY_from_guest | HVMCOPY_no_fault |
>> > HVMCOPY_virt,
>> > -                      PFEC_page_present | pfec);
>> > +                      PFEC_page_present | pfec |
>> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
>> > 0);
>> >  }
>> >  
>> >  enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(
>> 
>> Was this patch tested at all? The lack of parentheses in all the
>> changes you make result - afaict - in PFEC_prot_key to be
>> unconditionally passed to __hvm_copy(), which can't be right.
> Yes, the patch can work, I understand, if the pfec parameter of __hvm_c
> opy is zero, it means that memory permission check is not to be
> required, when pfec is not 0, PKRU has access disable and write
> disable, so, PFEC_prot_key is unconditionally passed to the functions.

Did you understand that what I'm trying to point out is that the
code you've supplied is equivalent to (in the above case)

    return __hvm_copy(buf, vaddr, size,
                      HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_virt,
                      PFEC_prot_key);

(which I doubt you consider correct)?

Jan

Re: [V2 PATCH 0/9] x86/hvm: pkeys, add memory protection-key support

[Andrew Cooper]

On 27/11/15 09:51, Huaitong Han wrote:
> Changes in v2:
> *Rebase all patches in staging branch
> *Disable X86_CR4_PKE on hypervisor, and delete pkru_read/write functions, and
> use xsave state read to get pkru value.
> *Delete the patch that adds pkeys support for do_page_fault.
> *Add pkeys support for gva2gfn so that setting _PAGE_PK_BIT in the return
> value can get propagated to the guest correctly.
>
> The protection-key feature provides an additional mechanism by which IA-32e
> paging controls access to usermode addresses.
>
> Hardware support for protection keys for user pages is enumerated with CPUID
> feature flag CPUID.7.0.ECX[3]:PKU. Software support is CPUID.7.0.ECX[4]:OSPKE
> with the setting of CR4.PKE(bit 22).
>
> When CR4.PKE = 1, every linear address is associated with the 4-bit protection
> key located in bits 62:59 of the paging-structure entry that mapped the page
> containing the linear address. The PKRU register determines, for each
> protection key, whether user-mode addresses with that protection key may be
> read or written.
>
> The PKRU register (protection key rights for user pages) is a 32-bit register
> with the following format: for each i (0 ≤ i ≤ 15), PKRU[2i] is the
> access-disable bit for protection key i (ADi); PKRU[2i+1] is the write-disable
> bit for protection key i (WDi).
>
> Software can use the RDPKRU and WRPKRU instructions with ECX = 0 to read and
> write PKRU. In addition, the PKRU register is XSAVE-managed state and can thus
> be read and written by instructions in the XSAVE feature set.
>
> PFEC.PK (bit 5) is defined as protection key violations.
>
> The specification of Protection Keys can be found at SDM (4.6.2, volume 3)
> http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf.

Just for my own understand, do you have a sample use-case for protection
keys?

As everything can WRPKRU, I cant see how it would actually be useful. 
Clearly there is a usecase or you (Intel) wouldn't have gone to the
effort of putting into silicon.

~Andrew

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

Re: [V2 PATCH 0/9] x86/hvm: pkeys, add memory protection-key support

[Han, Huaitong]

On Fri, 2015-11-27 at 12:59 +0000, Andrew Cooper wrote:
> Just for my own understand, do you have a sample use-case for
> protection
> keys?
> 
> As everything can WRPKRU, I cant see how it would actually be useful.
> Clearly there is a usecase or you (Intel) wouldn't have gone to the
> effort of putting into silicon.
> 
> ~Andrew

I understand PKU is a active memory protection, pkru register is thread
-private, every thread can only change its pkru value, it is a
enhancement of pte write/access, it provides two advantage:
1.During permission changes, we only need update pkru register instead 
  of pte, so it can reduce the need to perform TLB shootdowns.
2.Compared with traditional read and write permissions, it can provide
thread-private permissions.

Thanks
Huaitong

Re: [V2 PATCH 0/9] x86/hvm: pkeys, add memory protection-key support

[Andrew Cooper]

On 27/11/15 16:22, Han, Huaitong wrote:
> On Fri, 2015-11-27 at 12:59 +0000, Andrew Cooper wrote:
>> Just for my own understand, do you have a sample use-case for
>> protection
>> keys?
>>
>> As everything can WRPKRU, I cant see how it would actually be useful.
>> Clearly there is a usecase or you (Intel) wouldn't have gone to the
>> effort of putting into silicon.
>>
>> ~Andrew
> I understand PKU is a active memory protection, pkru register is thread
> -private, every thread can only change its pkru value, it is a
> enhancement of pte write/access, it provides two advantage:
> 1.During permission changes, we only need update pkru register instead 
>   of pte, so it can reduce the need to perform TLB shootdowns.
> 2.Compared with traditional read and write permissions, it can provide
> thread-private permissions.

So it is a feature which allows threads to cooperatively prevent
themselves trampling on each other.

I can see this being useful for debugging and robustness.

~Andrew