KVM: PKS Virtualization support

From: Chenyi Qiang <[email protected]>

PKS(Protection Keys for Supervisor Pages) is a feature that extends the Protection Key architecture to support thread-specific permission restrictions on supervisor pages.

A new PKS MSR(PKRS) is defined in kernel to support PKS, which holds a set of permissions associated with each protection domain.

Two VMCS fields {HOST,GUEST}_IA32_PKRS are introduced in {host,guest}-state area to store the respective values of PKRS.

Every VM exit saves PKRS into guest-state area.

Uber nit, PKRS isn't saved if VMX doesn't support the entry control.
Every VM exit saves PKRS into guest-state area if VM_ENTRY_LOAD_IA32_PKRS is supported by the CPU.

With that tweak,

From: Chenyi Qiang <[email protected]>

Add PKRS caching into the standard register caching mechanism in order to take advantage of the availability checks provided by regs_avail.

This is because vcpu->arch.pkrs will be rarely acceesed by KVM, only in the case of host userspace MSR reads and GVA->GPA translation in following patches. It is unnecessary to keep it up-to-date at all times.

It also should be noted that the potential benefits of this caching are tenuous because the MSR read is not a hot path. it's nice-to-have so that we don't hesitate to rip it out in the future if there's a strong reason to drop the caching.

The patch looks fine, but this needs to be moved to the end of the series. Definitely after "KVM: VMX: Expose PKS to guest", and maybe even after "KVM: VMX:
Enable PKS for nested VM".

(quoted diff) +1 -0

@@ -1111,6 +1113,7 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
 #endif
 	unsigned long fs_base, gs_base;
 	u16 fs_sel, gs_sel;
+	u32 host_pkrs;
 	int i;
 
 	vmx->req_immediate_exit = false;

(quoted diff) +3 -0

+		vmx_set_host_pkrs(host_state, host_pkrs);
+	}
+
 #ifdef CONFIG_X86_64
 	savesegment(ds, host_state->ds_sel);
 	savesegment(es, host_state->es_sel);

(quoted diff) +1 -0

+		break;
 	default:
 	find_uret_msr:
 		msr = vmx_find_uret_msr(vmx, msr_index);

(quoted diff) +7 -0

+		} else {
+			vmx_enable_intercept_for_msr(vcpu, MSR_IA32_PKRS, MSR_TYPE_RW);
+
+			vm_entry_controls_clearbit(vmx, VM_ENTRY_LOAD_IA32_PKRS);
+			vm_exit_controls_clearbit(vmx, VM_EXIT_LOAD_IA32_PKRS);
+		}
+	}
 }
 

(quoted diff) +0 -0

 	vcpu->arch.cr3 = 0;
 	kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
 

Nit, something like:

KVM: X86: Virtualize and pass-through IA32_PKRS MSR when supported

because "expose" doesn't precisely cover the pass-through behavior

Eh, I don't think this comment adds anything. And practically speaking, whether or not X86_FEATURE_PKS is reported by kvm_cpu_cap_has() and/or guest_cpuid_has() is irrelevant. If KVM is loading PKRS on exit, then the VMCS needs to hold an up-to-date value. E.g. if for some reason KVM enables the control even if PKS isn't exposed to the guest (see below), this code still needs to refresh the value.

No need for an intermediate host_pkrs. I.e. this can simply be:

	if (vm_exit_controls_get(vmx) & VM_EXIT_LOAD_IA32_PKRS)
		vmx_set_host_pkrs(host_state, get_current_pkrs());

Nit, please align the lines that are inside a pair of parantheses, i.e.

A comment on the caching patch, please use kvm_pkrs_read() to follow the GPR, RIP, PDPTR, etc... terminology (CR0/3/4 got grandfathered in).

Nit, please move this to after the capability checks. Does not affect functionality at all, but logically it doesn't make sense to check for a valid value of a register that doesn't exist.

The caching patch should add kvm_write_pkrs(). And in there, I think I'd vote for a WARN_ON_ONCE() that the incoming value doesn't set bits 63:32. It's redundant with kvm_pkrs_valid()

Ugh, toggling the entry/exit controls here won't do the correct thing if L2 is active. The MSR intercept logic works because it always operates on vmcs01's bitmap.

Let's keep this as simple as possible and set the controls if they're supported. KVM will waste a few cycles on entry/exit if PKS is supported in the host but not exposed to the guest, but that's not the end of the world. If we want to optimize that, then we can do that in the future and in a more generic way.

So this becomes:

	if (kvm_cpu_cap_has(X86_FEATURE_PKS))
		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PKRS, MSR_TYPE_RW,
					  !guest_cpuid_has(vcpu, X86_FEATURE_PKS));

And please hoist that up to be next to the handling of X86_FEATURE_XFD to try and bunch together code that does similar things.

The last edge case to deal with is if only one of the controls is supported, e.g. if an L0 hypervisor is being evil. Big surprise, BNDCFGS doesn't get this right and needs a bug fix patch, e.g. it could retain the guest's value after exit, or host's value after entry. It's a silly case because it basically requires broken host "hardware", but it'd be nice to get it right in KVM. And that'd also be a good opportunity to handle all of the pairs, i.e. clear the bits during setup_vmcs_config() instead of checking both the entry and exit flags in cpu_has_load_*().

So for this series, optimistically assume my idea will pan out and a adjust_vm_entry_exit_pair() helper will exit by the time this is fully baked.

arch/x86/kvm/vmx/vmx.c +13 -0

index 6927f6e8ec31..53e12e6006af 100644
@@ -2434,6 +2434,16 @@ static bool cpu_has_sgx(void)
         return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0));
  }
 
+static __init void adjust_vm_entry_exit_pair(u32 *entry_controls, u32 entry_bit,
+                                            u32 *exit_controls, u32 exit_bit)
+{
+       if ((*entry_controls & entry_bit) && (*exit_controls & exit_bit))
+               return;
+
+       *entry_controls &= ~entry_bit;
+       *exit_controls &= ~exit_bit;
+}
+
  static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
                                       u32 msr, u32 *result)
  {

@@ -2614,6 +2624,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
                                 &_vmentry_control) < 0)
                 return -EIO;
 
+       adjust_vm_entry_exit_pair(&_vmentry_control, VM_ENTRY_LOAD_IA32_PKRS,
+                                 &_vmexit_control, VM_EXIT_LOAD_IA32_PKRS);
+
         /*
          * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they
          * can't be used due to an errata where VM Exit may incorrectly clear

Please put this with the other !init_event stuff, e.g. look for MSR_IA32_XSS.

Extra the PKR stuff to a separate, non-inline helper, which is a

preparation to introduce pks support.

Signed-off-by Lei Wang <[email protected]>

---
 arch/x86/kvm/mmu.h     | 20 +++++---------------
 arch/x86/kvm/mmu/mmu.c | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 15 deletions(-)

arch/x86/kvm/mmu.h +0 -0

index cb3f07e63778..cea03053a153 100644

arch/x86/kvm/mmu.h +2 -7

+		     unsigned pte_access, unsigned pte_pkey, unsigned int pfec);
+
 /*
  * Check if a given access (described through the I/D, W/R and U/S bits of a
  * page fault error code pfec) causes a permission fault with the given PTE

@@ -240,21 +243,8 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 
 	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
 	if (unlikely(mmu->pkr_mask)) {
-		u32 pkr_bits, offset;
-
-		/*
-		* PKRU defines 32 bits, there are 16 domains and 2
-		* attribute bits per domain in pkru.  pte_pkey is the
-		* index of the protection domain, so pte_pkey * 2 is
-		* is the index of the first bit for the domain.

arch/x86/kvm/mmu.h +0 -0

 		errcode |= -pkr_bits & PFERR_PK_MASK;
 		fault |= (pkr_bits != 0);
 	}

arch/x86/kvm/mmu/mmu.c +0 -0

index de665361548d..6d3276986102 100644
@@ -6477,3 +6477,24 @@ void kvm_mmu_pre_destroy_vm(struct kvm *kvm)
 	if (kvm->arch.nx_lpage_recovery_thread)

arch/x86/kvm/mmu/mmu.c +13 -0

+	* attribute bits per domain in pkru.  pte_pkey is the
+	* index of the protection domain, so pte_pkey * 2 is
+	* is the index of the first bit for the domain.
+	*/
+	pkr_bits = (vcpu->arch.pkru >> (pte_pkey * 2)) & 3;
+
+	/* clear present bit, replace PFEC.RSVD with ACC_USER_MASK. */
+	offset = (pfec & ~1) + ((pte_access & PT_USER_MASK)
+				<< (PFERR_RSVD_BIT - PT_USER_SHIFT));
+
+	pkr_bits &= mmu->pkr_mask >> offset;
+	return pkr_bits;
+}

Signature · 1 line

2.25.1

Quoted 1 line · Lei Wang

Extra the PKR stuff to a separate, non-inline helper, which is a

s/Extra/Extract

Quoted 1 line

preparation to introduce pks support.

Please provide more justification. The change is justified, by random readers of this patch/commit will be clueless.

Extract getting the effective PKR bits to a helper that lives in mmu.c in order to keep the is_cr4_*() helpers contained to mmu.c. Support for PKRS (versus just PKRU) will require querying MMU state to see if the relevant CR4 bit is enabled because pkr_mask will be non-zero if _either_ bit is enabled).

PKR{U,S} are exposed to the guest if and only if TDP is enabled, and while permission_fault() is performance critical for ia32 shadow paging, it's a rarely used path with TDP is enabled. I.e. moving the PKR code out-of-line is not a performance concern.

Quoted 14 lines

Signed-off-by Lei Wang <[email protected]>

---
  arch/x86/kvm/mmu.h     | 20 +++++---------------
  arch/x86/kvm/mmu/mmu.c | 21 +++++++++++++++++++++
  2 files changed, 26 insertions(+), 15 deletions(-)

arch/x86/kvm/mmu.h +0 -0

index cb3f07e63778..cea03053a153 100644
@@ -204,6 +204,9 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
  	return vcpu->arch.mmu->page_fault(vcpu, &fault);
  }
 

arch/x86/kvm/mmu.h static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, · line 207

Quoted 1 line

arch/x86/kvm/mmu.h +1 -0

+u32 kvm_mmu_pkr_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,

kvm_mmu_get_pkr_bits() so that there's a verb in there.

Quoted 16 lines

arch/x86/kvm/mmu.h +2 -7

+		     unsigned pte_access, unsigned pte_pkey, unsigned int pfec);
+
  /*
   * Check if a given access (described through the I/D, W/R and U/S bits of a
   * page fault error code pfec) causes a permission fault with the given PTE

@@ -240,21 +243,8 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 
  	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
  	if (unlikely(mmu->pkr_mask)) {
-		u32 pkr_bits, offset;
-
-		/*
-		* PKRU defines 32 bits, there are 16 domains and 2
-		* attribute bits per domain in pkru.  pte_pkey is the
-		* index of the protection domain, so pte_pkey * 2 is
-		* is the index of the first bit for the domain.

arch/x86/kvm/mmu.h static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, · line 247

Quoted 10 lines

arch/x86/kvm/mmu.h +2 -8

-		*/
-		pkr_bits = (vcpu->arch.pkru >> (pte_pkey * 2)) & 3;
-
-		/* clear present bit, replace PFEC.RSVD with ACC_USER_MASK. */
-		offset = (pfec & ~1) +
-			((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT));
-
-		pkr_bits &= mmu->pkr_mask >> offset;
+		u32 pkr_bits =
+			kvm_mmu_pkr_bits(vcpu, mmu, pte_access, pte_pkey, pfec);

Nit, I prefer wrapping in the params, that way the first line shows the most important information, e.g. what variable is being set and how (by a function call). And then there won't be overflow with the longer helper name:

		u32 pkr_bits = kvm_mmu_get_pkr_bits(vcpu, mmu, pte_access,
						    pte_pkey, pfec);

Comment needs to be aligned, and it can be adjust to wrap at 80 chars (its indentation has changed).

/*
* PKRU and PKRS both define 32 bits. There are 16 domains and 2
* attribute bits per domain in them. pte_key is the index of the
* protection domain, so pte_pkey * 2 is the index of the first bit for
* the domain. The use of PKRU versus PKRS is selected by the address
* type, as determined by the U/S bit in the paging-structure entries. */

arch/x86/include/asm/kvm_host.h +0 -0

 	*/
 	u32 pkr_mask;
 

arch/x86/kvm/mmu.h +2 -1

index cea03053a153..6963c641e6ce 100644
@@ -45,7 +45,8 @@
 #define PT32E_ROOT_LEVEL 3
 
 #define KVM_MMU_CR4_ROLE_BITS (X86_CR4_PSE | X86_CR4_PAE | X86_CR4_LA57 | \
-			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
+			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE | \
+			       X86_CR4_PKS)
 
 #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
 #define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)

arch/x86/kvm/mmu/mmu.c +2 -0

index 6d3276986102..a6cbc22d3312 100644
@@ -209,6 +209,7 @@ BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, smep, X86_CR4_SMEP);
 BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, smap, X86_CR4_SMAP);
 BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, pke, X86_CR4_PKE);
 BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, la57, X86_CR4_LA57);
+BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, pks, X86_CR4_PKS);
 BUILD_MMU_ROLE_REGS_ACCESSOR(efer, nx, EFER_NX);
 BUILD_MMU_ROLE_REGS_ACCESSOR(efer, lma, EFER_LMA);
 

@@ -231,6 +232,7 @@ BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, smep);
 BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, smap);
 BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, pke);
 BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, la57);
+BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, pks);
 BUILD_MMU_ROLE_ACCESSOR(base, efer, nx);
 
 static struct kvm_mmu_role_regs vcpu_to_role_regs(struct kvm_vcpu *vcpu)

@@ -4608,37 +4610,58 @@ static void update_permission_bitmask(struct kvm_mmu *mmu, bool ept)
 }
 
 /*

+ * Protection Key Rights (PKR) is an additional mechanism by which data accesses
+ * with 4-level or 5-level paging (EFER.LMA=1) may be disabled based on the
+ * Protection Key Rights Userspace (PRKU) or Protection Key Rights Supervisor
+ * (PKRS) registers. The Protection Key (PK) used for an access is a 4-bit
+ * value specified in bits 62:59 of the leaf PTE used to translate the address.
+ *
+ * PKRU and PKRS are 32-bit registers, with 16 2-bit entries consisting of an
+ * access-disable (AD) and write-disable (WD) bit. The PK from the leaf PTE is
+ * used to index the approriate PKR (see below), e.g. PK=1 would consume bits

+ * 3:2 (bit 3 == write-disable, bit 2 == access-disable).
+ *
+ * The PK register (PKRU vs. PKRS) indexed by the PK depends on the type of
+ * _address_ (not access type!). For a user-mode address, PKRU is used; for a
+ * supervisor-mode address, PKRS is used. An address is supervisor-mode if the
+ * U/S flag (bit 2) is 0 in at least one of the paging-structure entries, i.e.
+ * an address is user-mode if the U/S flag is 1 in _all_ entries. Again, this
+ * is the address type, not the the access type, e.g. a supervisor-mode _access_

+ * will consume PKRU if the _address_ is a user-mode address.
+ *
+ * As alluded to above, PKR checks are only performed for data accesses; code
+ * fetches are not subject to PKR checks. Terminal page faults (!PRESENT or
+ * PFEC.RSVD=1) are also not subject to PKR checks.
+ *
+ * PKR write-disable checks for superivsor-mode _accesses_ are performed if and
+ * only if CR0.WP=1 (though access-disable checks still apply).
+ *
+ * In summary, PKR checks are based on (a) EFER.LMA, (b) CR4.PKE or CR4.PKS,
+ * (c) CR0.WP, (d) the PK in the leaf PTE, (e) two bits from the corresponding
+ * PKR{S,U} entry, (f) the access type (derived from the other PFEC bits), and
+ * (g) the address type (retrieved from the paging-structure entries).
+ *
+ * To avoid conditional branches in permission_fault(), the PKR bitmask caches
+ * the above inputs, except for (e) the PKR{S,U} entry. The FETCH, USER, and
+ * WRITE bits of the PFEC and the effective value of the paging-structures' U/S
+ * bit (slotted into the PFEC.RSVD position, bit 3) are used to index into the
+ * PKR bitmask (similar to the 4-bit Protection Key itself). The two bits of
+ * the PKR bitmask "entry" are then extracted and ANDed with the two bits of
+ * the PKR{S,U} register corresponding to the address type and protection key.
+ *
+ * E.g. for all values where PFEC.FETCH=1, the corresponding pkr_bitmask bits
+ * will be 00b, thus masking away the AD and WD bits from the PKR{S,U} register
+ * to suppress PKR checks on code fetches.
+ */
static void update_pkr_bitmask(struct kvm_mmu *mmu)
{

arch/x86/kvm/mmu/mmu.c +3 -1

+	bool cr4_pke = is_cr4_pke(mmu);
+	bool cr4_pks = is_cr4_pks(mmu);
 	mmu->pkr_mask = 0;
 
-	if (!is_cr4_pke(mmu))
+	if (!cr4_pke && !cr4_pks)
 		return;
 
 	wp = is_cr0_wp(mmu);

arch/x86/kvm/mmu/mmu.c +1 -0

@@ -6482,14 +6509,22 @@ u32 kvm_mmu_pkr_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 		     unsigned pte_access, unsigned pte_pkey, unsigned int pfec)
 {
 	u32 pkr_bits, offset;
+	u32 pkr;
 
 	/*

arch/x86/kvm/mmu/mmu.c +6 -1

 	*/
-	pkr_bits = (vcpu->arch.pkru >> (pte_pkey * 2)) & 3;
+	if (pte_access & PT_USER_MASK)
+		pkr = is_cr4_pke(mmu) ? vcpu->arch.pkru : 0;
+	else
+		pkr = is_cr4_pks(mmu) ? kvm_read_pkrs(vcpu) : 0;
+
+	pkr_bits = (pkr >> pte_pkey * 2) & 3;
 
 	/* clear present bit, replace PFEC.RSVD with ACC_USER_MASK. */
 	offset = (pfec & ~1) + ((pte_access & PT_USER_MASK)

Signature · 1 line

2.25.1

Quoted 3 lines · Lei Wang

arch/x86/include/asm/kvm_host.h +0 -0

@@ -454,10 +455,11 @@ struct kvm_mmu {
  	u8 permissions[16];
 

arch/x86/include/asm/kvm_host.h struct kvm_mmu { · line 462

Quoted 10 lines

arch/x86/include/asm/kvm_host.h +5 -4

  	/*
-	* The pkru_mask indicates if protection key checks are needed.  It
-	* consists of 16 domains indexed by page fault error code bits [4:1],
-	* with PFEC.RSVD replaced by ACC_USER_MASK from the page tables.
-	* Each domain has 2 bits which are ANDed with AD and WD from PKRU.
+	* The pkr_mask indicates if protection key checks are needed.
+	* It consists of 16 domains indexed by page fault error code
+	* bits[4:1] with PFEC.RSVD replaced by ACC_USER_MASK from the
+	* page tables. Each domain has 2 bits which are ANDed with AD
+	* and WD from PKRU/PKRS.

Same comments, align and wrap closer to 80 please.

Quoted 41 lines

arch/x86/include/asm/kvm_host.h +0 -0

  	*/
  	u32 pkr_mask;
 

arch/x86/kvm/mmu.h +2 -1

index cea03053a153..6963c641e6ce 100644
@@ -45,7 +45,8 @@
  #define PT32E_ROOT_LEVEL 3
 
  #define KVM_MMU_CR4_ROLE_BITS (X86_CR4_PSE | X86_CR4_PAE | X86_CR4_LA57 | \
-			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
+			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE | \
+			       X86_CR4_PKS)
 
  #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
  #define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)

arch/x86/kvm/mmu/mmu.c +2 -0

index 6d3276986102..a6cbc22d3312 100644
@@ -209,6 +209,7 @@ BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, smep, X86_CR4_SMEP);
  BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, smap, X86_CR4_SMAP);
  BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, pke, X86_CR4_PKE);
  BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, la57, X86_CR4_LA57);
+BUILD_MMU_ROLE_REGS_ACCESSOR(cr4, pks, X86_CR4_PKS);
  BUILD_MMU_ROLE_REGS_ACCESSOR(efer, nx, EFER_NX);
  BUILD_MMU_ROLE_REGS_ACCESSOR(efer, lma, EFER_LMA);
 

@@ -231,6 +232,7 @@ BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, smep);
  BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, smap);
  BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, pke);
  BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, la57);
+BUILD_MMU_ROLE_ACCESSOR(ext,  cr4, pks);
  BUILD_MMU_ROLE_ACCESSOR(base, efer, nx);
 
  static struct kvm_mmu_role_regs vcpu_to_role_regs(struct kvm_vcpu *vcpu)

@@ -4608,37 +4610,58 @@ static void update_permission_bitmask(struct kvm_mmu *mmu, bool ept)
  }
 
  /*

...

Quoted 9 lines

+ * Protection Key Rights (PKR) is an additional mechanism by which data accesses
+ * with 4-level or 5-level paging (EFER.LMA=1) may be disabled based on the
+ * Protection Key Rights Userspace (PRKU) or Protection Key Rights Supervisor
+ * (PKRS) registers. The Protection Key (PK) used for an access is a 4-bit
+ * value specified in bits 62:59 of the leaf PTE used to translate the address.
+ *
+ * PKRU and PKRS are 32-bit registers, with 16 2-bit entries consisting of an
+ * access-disable (AD) and write-disable (WD) bit. The PK from the leaf PTE is
+ * used to index the approriate PKR (see below), e.g. PK=1 would consume bits

s/approriate/appropriate

Quoted 8 lines

+ * 3:2 (bit 3 == write-disable, bit 2 == access-disable).
+ *
+ * The PK register (PKRU vs. PKRS) indexed by the PK depends on the type of
+ * _address_ (not access type!). For a user-mode address, PKRU is used; for a
+ * supervisor-mode address, PKRS is used. An address is supervisor-mode if the
+ * U/S flag (bit 2) is 0 in at least one of the paging-structure entries, i.e.
+ * an address is user-mode if the U/S flag is 1 in _all_ entries. Again, this
+ * is the address type, not the the access type, e.g. a supervisor-mode _access_

Double "the the" can be a single "the".

Quoted 23 lines

+ * will consume PKRU if the _address_ is a user-mode address.
+ *
+ * As alluded to above, PKR checks are only performed for data accesses; code
+ * fetches are not subject to PKR checks. Terminal page faults (!PRESENT or
+ * PFEC.RSVD=1) are also not subject to PKR checks.
+ *
+ * PKR write-disable checks for superivsor-mode _accesses_ are performed if and
+ * only if CR0.WP=1 (though access-disable checks still apply).
+ *
+ * In summary, PKR checks are based on (a) EFER.LMA, (b) CR4.PKE or CR4.PKS,
+ * (c) CR0.WP, (d) the PK in the leaf PTE, (e) two bits from the corresponding
+ * PKR{S,U} entry, (f) the access type (derived from the other PFEC bits), and
+ * (g) the address type (retrieved from the paging-structure entries).
+ *
+ * To avoid conditional branches in permission_fault(), the PKR bitmask caches
+ * the above inputs, except for (e) the PKR{S,U} entry. The FETCH, USER, and
+ * WRITE bits of the PFEC and the effective value of the paging-structures' U/S
+ * bit (slotted into the PFEC.RSVD position, bit 3) are used to index into the
+ * PKR bitmask (similar to the 4-bit Protection Key itself). The two bits of
+ * the PKR bitmask "entry" are then extracted and ANDed with the two bits of
+ * the PKR{S,U} register corresponding to the address type and protection key.
+ *
+ * E.g. for all values where PFEC.FETCH=1, the corresponding pkr_bitmask bits

Quoted 8 lines

+ * will be 00b, thus masking away the AD and WD bits from the PKR{S,U} register
+ * to suppress PKR checks on code fetches.
+ */
static void update_pkr_bitmask(struct kvm_mmu *mmu)
{
unsigned bit;
bool wp;
-

Please keep this newline, i.e. after the declaration of the cr4 booleans. That helps isolate the clearing of mmu->pkr_mask, which makes the functional affect of the earlier return more obvious.

Ah, and use reverse fir tree for the variable declarations, i.e.

	bool cr4_pke = is_cr4_pke(mmu);
	bool cr4_pks = is_cr4_pks(mmu);
	unsigned bit;
	bool wp;

	mmu->pkr_mask = 0;

	if (!cr4_pke && !cr4_pks)
		return;

arch/x86/kvm/mmu/mmu.c static void update_permission_bitmask(struct kvm_mmu *mmu, bool ept) · line 4667

Quoted 9 lines

arch/x86/kvm/mmu/mmu.c +3 -1

+	bool cr4_pke = is_cr4_pke(mmu);
+	bool cr4_pks = is_cr4_pks(mmu);
  	mmu->pkr_mask = 0;
 
-	if (!is_cr4_pke(mmu))
+	if (!cr4_pke && !cr4_pks)
  		return;
 
  	wp = is_cr0_wp(mmu);

   ...

Quoted 6 lines

arch/x86/kvm/mmu/mmu.c +1 -0

@@ -6482,14 +6509,22 @@ u32 kvm_mmu_pkr_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
  		     unsigned pte_access, unsigned pte_pkey, unsigned int pfec)
  {
  	u32 pkr_bits, offset;
+	u32 pkr;
 

arch/x86/kvm/mmu/mmu.c u32 kvm_mmu_pkr_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, · line 6520

Quoted 11 lines

arch/x86/kvm/mmu/mmu.c +6 -4

  	/*
-	* PKRU defines 32 bits, there are 16 domains and 2
-	* attribute bits per domain in pkru.  pte_pkey is the
-	* index of the protection domain, so pte_pkey * 2 is
-	* is the index of the first bit for the domain.
+	* PKRU and PKRS both define 32 bits. There are 16 domains
+	* and 2 attribute bits per domain in them. pte_key is the
+	* index of the protection domain, so pte_pkey * 2 is the
+	* index of the first bit for the domain. The use of PKRU
+	* versus PKRS is selected by the address type, as determined
+	* by the U/S bit in the paging-structure entries.

Align and wrap closer to 80 please.

arch/x86/kvm/vmx/vmx.c +0 -0

 		 * to be manually disabled when guest switches to non-paging
 		 * mode.

arch/x86/kvm/vmx/vmx.c void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) · line 3254

Quoted 11 lines · Lei Wang

arch/x86/kvm/vmx/vmx.c +1 -1

index 9d0588e85410..cbcb0d7b47a4 100644
@@ -3250,7 +3250,7 @@ void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
  		}
 
  		/*
-		 * SMEP/SMAP/PKU is disabled if CPU is in non-paging mode in
+		 * SMEP/SMAP/PKU/PKS is disabled if CPU is in non-paging mode in
  		 * hardware.  To emulate this behavior, SMEP/SMAP/PKU needs

Heh, missed one ;-) Let's reduce future pain and reword this whole comment:

/*
* SMEP/SMAP/PKU/PKS are effectively disabled if the CPU is in
* non-paging mode in hardware. To emulate this behavior,
* clear them in the hardware CR4 when the guest switches to
* non-paging mode and unrestricted guest is disabled, as KVM
* must run the guest with hardware CR0.PG=1.
*/

arch/x86/kvm/vmx/nested.c +12 -0

+	    (!vmx->nested.nested_run_pending ||
+	     !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS)))
+		vmcs_write64(GUEST_IA32_PKRS, vmx->nested.vmcs01_guest_pkrs);
+
 	vmx_set_rflags(vcpu, vmcs12->guest_rflags);
 
 	/* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the

@@ -2897,6 +2910,10 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 					   vmcs12->host_ia32_perf_global_ctrl)))
 		return -EINVAL;
 
+	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PKRS) &&
+	    CC(!kvm_pkrs_valid(vmcs12->host_ia32_pkrs)))
+		return -EINVAL;
+
 #ifdef CONFIG_X86_64
 	ia32e = !!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE);
 #else

@@ -3049,6 +3066,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 	if (nested_check_guest_non_reg_state(vmcs12))
 		return -EINVAL;
 
+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS) &&
+	    CC(!kvm_pkrs_valid(vmcs12->guest_ia32_pkrs)))
+		return -EINVAL;
+
 	return 0;
 }
 

arch/x86/kvm/vmx/nested.c +2 -0

+	     !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS)))
+		vmx->nested.vmcs01_guest_pkrs = vmcs_read64(GUEST_IA32_PKRS);
 
 	/*
 	 * Overwrite vmcs01.GUEST_CR3 with L1's CR3 if EPT is disabled *and*

arch/x86/kvm/vmx/vmx.h +2 -0

+	u64 vmcs01_guest_pkrs;
+
 	/* to migrate it to L1 if L2 writes to L1's CR8 directly */
 	int l1_tpr_threshold;
 

Signature · 1 line

2.25.1

Nit, use "KVM: nVMX:" for the shortlog scope.

arch/x86/kvm/vmx/nested.c static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12) · line 2443

Quoted 8 lines

arch/x86/kvm/vmx/nested.c +4 -0

@@ -2433,6 +2437,10 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
  		if (kvm_mpx_supported() && vmx->nested.nested_run_pending &&
  		    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS))
  			vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
+
+		if (vmx->nested.nested_run_pending &&
+		    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS))
+			vmcs_write64(GUEST_IA32_PKRS, vmcs12->guest_ia32_pkrs);

As mentioned in the BNDCFGS thread, this does the wrong thing for SMM. But, after a lot of thought, handling this in nested_vmx_enter_non_root_mode() would be little more than a band-aid, and a messy one at that, because KVM's SMM emulation is horrifically broken with respect to nVMX.

Entry does to SMM does not modify _any_ state that is not saved in SMRAM. That we're having to deal with this crap is a symptom of KVM doing the complete wrong thing by piggybacking nested_vmx_vmexit() and nested_vmx_enter_non_root_mode().

The SDM's description of CET spells this out very, very clearly:

On processors that support CET shadow stacks, when the processor enters SMM, the processor saves the SSP register to the SMRAM state save area (see Table 31-3) and clears CR4.CET to 0. Thus, the initial execution environment of the SMI handler has CET disabled and all of the CET state of the interrupted program is still in the machine. An SMM that uses CET is required to save the interrupted program’s CET state and restore the CET state prior to exiting SMM.

It mostly works because no guest SMM handler does anything with most of the MSRs, but it's all wildy wrong. A concrete example of a lurking bug is if vmcs12 uses the VM-Exit MSR load list, in which case the forced nested_vmx_vmexit() will load state that is never undone.

So, my very strong vote is to ignore SMM and let someone who actually cares about SMM fix that mess properly by adding custom flows for exiting/re-entering L2 on SMI/RSM.

arch/x86/kvm/vmx/nested.c static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, · line 2532

Quoted 8 lines

arch/x86/kvm/vmx/nested.c +1 -0

  	}
 
  	if (nested_cpu_has_xsaves(vmcs12))

@@ -2521,6 +2529,11 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
  	if (kvm_mpx_supported() && (!vmx->nested.nested_run_pending ||
  	    !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)))
  		vmcs_write64(GUEST_BNDCFGS, vmx->nested.vmcs01_guest_bndcfgs);
+	if (kvm_cpu_cap_has(X86_FEATURE_PKS) &&

ERROR: trailing whitespace
#85: FILE: arch/x86/kvm/vmx/nested.c:3407:
+^Iif (kvm_cpu_cap_has(X86_FEATURE_PKS) && $

Quoted 29 lines

arch/x86/kvm/vmx/nested.c +12 -0

+	    (!vmx->nested.nested_run_pending ||
+	     !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS)))
+		vmcs_write64(GUEST_IA32_PKRS, vmx->nested.vmcs01_guest_pkrs);
+
  	vmx_set_rflags(vcpu, vmcs12->guest_rflags);
 
  	/* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the

@@ -2897,6 +2910,10 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
  					   vmcs12->host_ia32_perf_global_ctrl)))
  		return -EINVAL;
 
+	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PKRS) &&
+	    CC(!kvm_pkrs_valid(vmcs12->host_ia32_pkrs)))
+		return -EINVAL;
+
  #ifdef CONFIG_X86_64
  	ia32e = !!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE);
  #else

@@ -3049,6 +3066,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
  	if (nested_check_guest_non_reg_state(vmcs12))
  		return -EINVAL;
 
+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS) &&
+	    CC(!kvm_pkrs_valid(vmcs12->guest_ia32_pkrs)))
+		return -EINVAL;
+
  	return 0;
  }
 

arch/x86/kvm/vmx/nested.c enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, · line 3409

Quoted 6 lines

arch/x86/kvm/vmx/nested.c +2 -0

@@ -3384,6 +3405,10 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
  	    (!from_vmentry ||
  	     !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)))
  		vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS);
+	if (kvm_cpu_cap_has(X86_FEATURE_PKS) &&
+	    (!from_vmentry ||

This should be "!vmx->nested.nested_run_pending" instead of "!from_vmentry" to avoid the unnecessary VMREAD when restoring L2 with a pending VM-Enter.

Quoted 5 lines

arch/x86/kvm/vmx/nested.c +2 -0

+	     !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PKRS)))
+		vmx->nested.vmcs01_guest_pkrs = vmcs_read64(GUEST_IA32_PKRS);
 
  	/*
  	 * Overwrite vmcs01.GUEST_CR3 with L1's CR3 if EPT is disabled *and*

...

arch/x86/kvm/vmx/vmx.h struct nested_vmx { · line 224

Quoted 8 lines

arch/x86/kvm/vmx/vmx.h +0 -0

index 91723a226bf3..82f79ac46d7b 100644
@@ -222,6 +222,8 @@ struct nested_vmx {
  	u64 vmcs01_debugctl;
  	u64 vmcs01_guest_bndcfgs;
 

Please pack these together, i.e. don't have a blank line between the various vmcs01_* fields.

This patch series is based on top of v10 PKS core support kernel patchset:
https://lore.kernel.org/lkml/[email protected]/

---

Protection Keys for Supervisor Pages(PKS) is a feature that extends the Protection Keys architecture to support thread-specific permission restrictions on supervisor pages.

PKS works similar to an existing feature named PKU(protecting user pages). They both perform an additional check after normal paging permission checks are done. Access or Writes can be disabled via a MSR update without TLB flushes when permissions changes. If violating this addional check, #PF occurs and PFEC.PK bit will be set.

PKS introduces MSR IA32_PKRS to manage supervisor protection key rights. The MSR contains 16 pairs of ADi and WDi bits. Each pair advertises on a group of pages with the same key which is set in the leaf paging-structure entries(bits[62:59]). Currently, IA32_PKRS is not supported by XSAVES architecture.

This patchset aims to add the virtualization of PKS in KVM. It implemented PKS CPUID enumeration, vmentry/vmexit configuration, MSR exposure, nested supported etc. Currently, PKS is not yet supported for shadow paging.

Detailed information about PKS can be found in the latest Intel 64 and IA-32 Architectures Software Developer's Manual.

---

Changelogs:

v6->v7
- Add documentation to note that it's nice-to-have cache tracking for PKRS, and we also needn't hesitate to rip it out in the future if there's a strong reason to drop the caching. (Sean)
- Blindly reading PKRU/PKRS is wrong, fixed. (Sean)
- Add a non-inline helper kvm_mmu_pkr_bits() to read PKR bits. (Sean)
- Delete the comment for exposing the PKS because the pattern is common and the behavior is self-explanatory. (Sean)
- Add a helper vmx_set_host_pkrs() for setting host pkrs and rewrite the related code for concise. (Sean)
- Align an indentation in arch/x86/kvm/vmx/nested.c. (Sean)
- Read the current PKRS if from_vmentry == false under the nested condition. (Sean)
- v6: https://lore.kernel.org/lkml/[email protected]/

v5->v6
- PKRS is preserved on INIT. Add the PKRS reset operation in kvm_vcpu_reset. (Sean)
- Track the pkrs as u32. Add the code WARN on bits 64:32 being set in VMCS field. (Sean)
- Adjust the MSR intercept and entry/exit control in VMCS according to guest CPUID. This resolve the issue when userspace re-enable this feature. (Sean)
- Split VMX restriction on PKS support(entry/exit load controls) out of common x86. And put tdp restriction together with PKU in common x86. (Sean)
- Thanks for Sean to revise the comments in mmu.c related to update_pkr_bitmap, which make it more clear for pkr bitmask cache usage.
- v5: https://lore.kernel.org/lkml/[email protected]/

v4->v5
- Make setting of MSR intercept/vmcs control bits not dependent on guest.CR4.PKS. And set them if PKS is exposed to guest. (Suggested by Sean)
- Add pkrs to standard register caching mechanism to help update vcpu->arch.pkrs on demand. Add related helper functions. (Suggested by Sean)
- Do the real pkrs update in VMCS field in vmx_vcpu_reset and vmx_sync_vmcs_host_state(). (Sean)
- Add a new mmu_role cr4_pks instead of smushing PKU and PKS together. (Sean & Paolo)
- v4: https://lore.kernel.org/lkml/[email protected]/

v3->v4
- Make the MSR intercept and load-controls setting depend on CR4.PKS value
- shadow the guest pkrs and make it usable in PKS emultion
- add the cr4_pke and cr4_pks check in pkr_mask update
- squash PATCH 2 and PATCH 5 to make the dependencies read more clear
- v3: https://lore.kernel.org/lkml/[email protected]/

v2->v3:
- No function changes since last submit
- rebase on the latest PKS kernel support:

   https://lore.kernel.org/lkml/[email protected]/

- add MSR_IA32_PKRS to the vmx_possible_passthrough_msrs[]
- RFC v2: https://lore.kernel.org/lkml/[email protected]/

v1->v2:
- rebase on the latest PKS kernel support:
https://github.com/weiny2/linux-kernel/tree/pks-rfc-v3
- add a kvm-unit-tests for PKS
- add the check in kvm_init_msr_list for PKRS
- place the X86_CR4_PKS in mmu_role_bits in kvm_set_cr4
- add the support to expose VM_{ENTRY, EXIT}_LOAD_IA32_PKRS in nested VMX MSR
- RFC v1: https://lore.kernel.org/lkml/[email protected]/

---

Chenyi Qiang (7):
KVM: VMX: Introduce PKS VMCS fields
KVM: VMX: Add proper cache tracking for PKRS
KVM: X86: Expose IA32_PKRS MSR
KVM: MMU: Rename the pkru to pkr
KVM: MMU: Add support for PKS emulation
KVM: VMX: Expose PKS to guest
KVM: VMX: Enable PKS for nested VM

Lei Wang (1):
KVM: MMU: Add helper function to get pkr bits

  arch/x86/include/asm/kvm_host.h |  17 +++--
  arch/x86/include/asm/vmx.h      |   6 ++
  arch/x86/kvm/cpuid.c            |  13 +++-
  arch/x86/kvm/kvm_cache_regs.h   |   7 ++
  arch/x86/kvm/mmu.h              |  29 +++----
  arch/x86/kvm/mmu/mmu.c          | 130 +++++++++++++++++++++++---------
  arch/x86/kvm/vmx/capabilities.h |   6 ++
  arch/x86/kvm/vmx/nested.c       |  36 ++++++++-
  arch/x86/kvm/vmx/vmcs.h         |   1 +
  arch/x86/kvm/vmx/vmcs12.c       |   2 +
  arch/x86/kvm/vmx/vmcs12.h       |   4 +
  arch/x86/kvm/vmx/vmx.c          |  85 +++++++++++++++++++--
  arch/x86/kvm/vmx/vmx.h          |  14 +++-
  arch/x86/kvm/x86.c              |   9 ++-
  arch/x86/kvm/x86.h              |   8 ++
  arch/x86/mm/pkeys.c             |   6 ++
  include/linux/pks.h             |   7 ++
  17 files changed, 301 insertions(+), 79 deletions(-)

Hi,

I'm sorry to bother you by replying to the email from years ago. I would like to learn about the current status of PKS virtualization.

In short, I tried to rebase this patch series on the latest kernel. The result was a working kernel that supports PKS virtualization, which would be useful for my purposes. Would PKS virtualization be accepted even if the kernel itself does not use PKS?

Fundamentally, I don't think this patch series
has to be built on top of basic PKS support. But I am unsure whether there is a policy or convention that states virtualization support can only be added after basic support.

One problem is that if the Linux kernel does not use PKS, we will be unable to test PKS virtualization with a guest Linux kernel. However, given that we have KVM unit test infrastructure, I believe we can find a way to properly test PKS virtualization for its correctness?

I'd like to hear from you to know whether I understand things correctly. Thank you in advance for any feedback.

Thanks,
Ruihan Li

No, there is none. In fact, the only dependency of the original series on host PKS was for functions to read/write the host PKRS MSR. Without host PKS support it could be loaded with all-ones, or technically it could even be left with the guest value. Since the host clears CR4.PKS, the actual value won't matter.

Quoted 5 lines · Paolo Bonzini

No, there is none. In fact, the only dependency of the original series on host PKS was for functions to read/write the host PKRS MSR. Without host PKS support it could be loaded with all-ones, or technically it could even be left with the guest value. Since the host clears CR4.PKS, the actual value won't matter.

Thanks a lot for your quick and detailed reply! That's good news for me. Then I plan to spend some time tidying up my rebased version to see if I can get PKS virtualization upstreamed.

As a side note, I can no longer contact the original author of this patch series. At least, my previous email was returned by Intel's server. However, since more than three years have passed, I assume it's okay for me to post a new version after I have the code ready.

Thanks,
Ruihan Li

Lei has left Intel so the mail address in unreachable.

And as you found, we dropped the PKS KVM upstream along with the base PKS support due to no valid use case in Linux. You can feel free to continue the upstream work.

But I'm not sure if your use case is compelling enough to be accepted by maintainers.

Quoted 4 lines · Chenyi Qiang

Lei has left Intel so the mail address in unreachable.

And as you found, we dropped the PKS KVM upstream along with the base PKS support due to no valid use case in Linux. You can feel free to continue the upstream work.

Thanks for the reply and for the information!

By the way, I'm just curious (feel free to ignore this question): Is there an on-list discussion that rejects the originally proposed PKS use cases?

I found that pmem stray write protection was rejected [1], but there is no reason given nor any reference provided. After searching the list, I found the latest patch series that attempts to add pmem stray protection [2]. However, I didn't find any discussion rejecting the use case. Maybe the discussion happened off the list? Or did I miss something?