[PATCH 0/4] KVM: X86: Improve permission_fault() for SMAP

[PATCH 0/4] KVM: X86: Improve permission_fault() for SMAP

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

permission_fault() calls two callbacks to get CPL and RFLAGS for SMAP,
but it is unneeded for some cases, this patchset improves it.

Lai Jiangshan (4):
  KVM: X86: Fix comments in update_permission_bitmask
  KVM: X86: Rename variable smap to not_smap in permission_fault()
  KVM: X86: Handle implicit supervisor access with SMAP
  KVM: X86: Only get rflags when needed in permission_fault()

 arch/x86/include/asm/kvm_host.h |  9 +++++++
 arch/x86/kvm/mmu.h              | 45 +++++++++++++++++++++------------
 arch/x86/kvm/mmu/mmu.c          |  8 +++---
 arch/x86/kvm/x86.c              | 20 ++++++++++++---
 4 files changed, 59 insertions(+), 23 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/4] KVM: X86: Fix comments in update_permission_bitmask

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

The commit 09f037aa48f3 ("KVM: MMU: speedup update_permission_bitmask")
refactored the code of update_permission_bitmask() and change the
comments.  It added a condition into a list to match the new code,
so the number/order for conditions in the comments should be updated
too.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kvm/mmu/mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 63c04c25fd66..9d045395fe8d 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4550,8 +4550,8 @@ static void update_permission_bitmask(struct kvm_mmu *mmu, bool ept)
 			 *   - Page fault in kernel mode
 			 *   - if CPL = 3 or X86_EFLAGS_AC is clear
 			 *
-			 * Here, we cover the first three conditions.
-			 * The fourth is computed dynamically in permission_fault();
+			 * Here, we cover the first four conditions.
+			 * The fifth is computed dynamically in permission_fault();
 			 * PFERR_RSVD_MASK bit will be set in PFEC if the access is
 			 * *not* subject to SMAP restrictions.
 			 */
-- 
2.19.1.6.gb485710b

[PATCH 2/4] KVM: X86: Rename variable smap to not_smap in permission_fault()

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

Comments above the variable says the bit is set when SMAP is overridden
or the same meaning in update_permission_bitmask(): it is not subjected
to SMAP restriction.

Renaming it to reflect the negative implication and make the code better
readability.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kvm/mmu.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 018108b0113a..b70b36734bc0 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -268,9 +268,9 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 	 * but it will be one in index if SMAP checks are being overridden.
 	 * It is important to keep this branchless.
 	 */
-	unsigned long smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
+	unsigned long not_smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
 	int index = (pfec >> 1) +
-		    (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
+		    (not_smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
 	bool fault = (mmu->permissions[index] >> pte_access) & 1;
 	u32 errcode = PFERR_PRESENT_MASK;
 
-- 
2.19.1.6.gb485710b

[PATCH 3/4] KVM: X86: Handle implicit supervisor access with SMAP

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

There are two kinds of implicit supervisor access
	implicit supervisor access when CPL = 3
	implicit supervisor access when CPL < 3

Current permission_fault() handles only the first kind for SMAP.

But if the access is implicit when SMAP is on, data may not be read
nor write from any user-mode address regardless the current CPL.

So the second find should be also supported.

The first kind can be detect via CPL and access mode: if it is
supervisor access and CPL = 3, it must be implicit supervisor access.

But it is not possible to detect the second kind without extra
information, so this patch adds vcpu->arch.explicit_access for it.  And
it is always set to KVM_EXPLICIT_ACCESS unless the vcpu is doing
implicit supervisor access.  This extra information also works for
the first kind, so the logic is changed to use this information
for both cases.

The value of KVM_EXPLICIT_ACCESS is deliberately chosen to include
all bits so that the calculation in permission_fault() can be
simplified.

This patch removes the call to ->get_cpl() for it integrates both
implicit supervisor access kinds into one logic.  Not only does it
reduce a function call, but also remove confusions when the permission
is checked for nested TDP.  The nested TDP shouldn't have SMAP
checking nor even the L2's CPL have any bearing on it.  The original
code works just because it is always user walk for NPT and SMAP fault
is not set for EPT in update_permission_bitmask.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/include/asm/kvm_host.h |  9 +++++++++
 arch/x86/kvm/mmu.h              | 17 ++++++++++-------
 arch/x86/kvm/mmu/mmu.c          |  4 ++--
 arch/x86/kvm/x86.c              | 20 +++++++++++++++++---
 4 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e41ad1ead721..88ecf53f0d2b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -261,6 +261,14 @@ enum x86_intercept_stage;
 				 PFERR_WRITE_MASK |		\
 				 PFERR_PRESENT_MASK)
 
+
+/*
+ * KVM_EXPLICIT_ACCESS is set to contain all bits so that the calculation
+ * in permission_fault() can be simplified and branchless.
+ */
+#define KVM_EXPLICIT_ACCESS	((u32)~0)
+#define KVM_IMPLICIT_ACCESS	((u32)0)
+
 /* apic attention bits */
 #define KVM_APIC_CHECK_VAPIC	0
 /*
@@ -630,6 +638,7 @@ struct kvm_vcpu_arch {
 	unsigned long cr8;
 	u32 host_pkru;
 	u32 pkru;
+	u32 explicit_access;
 	u32 hflags;
 	u64 efer;
 	u64 apic_base;
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index b70b36734bc0..0cb2c52377c8 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -252,23 +252,26 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 				  unsigned pte_access, unsigned pte_pkey,
 				  unsigned pfec)
 {
-	int cpl = static_call(kvm_x86_get_cpl)(vcpu);
 	unsigned long rflags = static_call(kvm_x86_get_rflags)(vcpu);
 
 	/*
-	 * If CPL < 3, SMAP prevention are disabled if EFLAGS.AC = 1.
+	 * If explicit supervisor accesses, SMAP is disabled
+	 * if EFLAGS.AC = 1.
 	 *
-	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
-	 * (these are implicit supervisor accesses) regardless of the value
-	 * of EFLAGS.AC.
+	 * If implicit supervisor accesses, SMAP can not be disabled
+	 * regardless of the value EFLAGS.AC.
 	 *
-	 * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving
+	 * SMAP works on supervisor accesses only, and not_smap can
+	 * be set or not set when user access with neither has any bearing
+	 * on the result.
+	 *
+	 * This computes explicit_access && (rflags & X86_EFLAGS_AC), leaving
 	 * the result in X86_EFLAGS_AC. We then insert it in place of
 	 * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
 	 * but it will be one in index if SMAP checks are being overridden.
 	 * It is important to keep this branchless.
 	 */
-	unsigned long not_smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
+	u32 not_smap = (rflags & X86_EFLAGS_AC) & vcpu->arch.explicit_access;
 	int index = (pfec >> 1) +
 		    (not_smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
 	bool fault = (mmu->permissions[index] >> pte_access) & 1;
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 9d045395fe8d..11b06d536cc9 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4547,8 +4547,8 @@ static void update_permission_bitmask(struct kvm_mmu *mmu, bool ept)
 			 *   - X86_CR4_SMAP is set in CR4
 			 *   - A user page is accessed
 			 *   - The access is not a fetch
-			 *   - Page fault in kernel mode
-			 *   - if CPL = 3 or X86_EFLAGS_AC is clear
+			 *   - The access is supervisor mode
+			 *   - If implicit supervisor access or X86_EFLAGS_AC is clear
 			 *
 			 * Here, we cover the first four conditions.
 			 * The fifth is computed dynamically in permission_fault();
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 78c40ac3b197..a972e3ab98ec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6651,11 +6651,17 @@ static int emulator_read_std(struct x86_emulate_ctxt *ctxt,
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
 	u32 access = 0;
+	int ret;
 
 	if (!system && static_call(kvm_x86_get_cpl)(vcpu) == 3)
 		access |= PFERR_USER_MASK;
 
-	return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access, exception);
+	if (system)
+		vcpu->arch.explicit_access = KVM_IMPLICIT_ACCESS;
+	ret = kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access, exception);
+	if (system)
+		vcpu->arch.explicit_access = KVM_EXPLICIT_ACCESS;
+	return ret;
 }
 
 static int kvm_read_guest_phys_system(struct x86_emulate_ctxt *ctxt,
@@ -6703,12 +6709,18 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
 	u32 access = PFERR_WRITE_MASK;
+	int ret;
 
 	if (!system && static_call(kvm_x86_get_cpl)(vcpu) == 3)
 		access |= PFERR_USER_MASK;
 
-	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
-					   access, exception);
+	if (system)
+		vcpu->arch.explicit_access = KVM_IMPLICIT_ACCESS;
+	ret = kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
+					  access, exception);
+	if (system)
+		vcpu->arch.explicit_access = KVM_EXPLICIT_ACCESS;
+	return ret;
 }
 
 int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val,
@@ -11104,6 +11116,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
 
+	vcpu->arch.explicit_access = KVM_EXPLICIT_ACCESS;
+
 	if (vcpu->arch.guest_fpu.fpstate && kvm_mpx_supported()) {
 		struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
 
-- 
2.19.1.6.gb485710b

Re: [PATCH 3/4] KVM: X86: Handle implicit supervisor access with SMAP

[Sean Christopherson]

On Tue, Dec 07, 2021, Lai Jiangshan wrote:
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index b70b36734bc0..0cb2c52377c8 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -252,23 +252,26 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  				  unsigned pte_access, unsigned pte_pkey,
>  				  unsigned pfec)
>  {
> -	int cpl = static_call(kvm_x86_get_cpl)(vcpu);
>  	unsigned long rflags = static_call(kvm_x86_get_rflags)(vcpu);
>  
>  	/*
> -	 * If CPL < 3, SMAP prevention are disabled if EFLAGS.AC = 1.
> +	 * If explicit supervisor accesses, SMAP is disabled

Slight reword, and each clause can fit on one line.

	 * For explicit supervisor accesses, SMAP is disabled if EFLAGS.AC = 1.
	 *
	 * For implicit supervisor accesses, SMAP cannot be overridden.

> +	 * if EFLAGS.AC = 1.
>  	 *
> -	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
> -	 * (these are implicit supervisor accesses) regardless of the value
> -	 * of EFLAGS.AC.
> +	 * If implicit supervisor accesses, SMAP can not be disabled
> +	 * regardless of the value EFLAGS.AC.
>  	 *
> -	 * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving
> +	 * SMAP works on supervisor accesses only, and not_smap can
> +	 * be set or not set when user access with neither has any bearing
> +	 * on the result.

This is quite jumbled, I'd just drop it entirely, the interesting bits are
the rules for implicit vs. explicit and the blurb below that describes the magic.

> +	 *
> +	 * This computes explicit_access && (rflags & X86_EFLAGS_AC), leaving

Too many &&, the logic below is a bitwise &, not a logical &&.

>  	 * the result in X86_EFLAGS_AC. We then insert it in place of
>  	 * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
>  	 * but it will be one in index if SMAP checks are being overridden.
>  	 * It is important to keep this branchless.

Heh, so important that it incurs multiple branches and possible VMREADs in
vmx_get_cpl() and vmx_get_rflags().  And before static_call, multiple retpolines
to boot.  Probably a net win now as only the first permission_fault() check for
a given VM-Exit be penalized, but the comment is amusing nonetheless.

>  	 */
> -	unsigned long not_smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
> +	u32 not_smap = (rflags & X86_EFLAGS_AC) & vcpu->arch.explicit_access;

I really, really dislike shoving this into vcpu->arch.  I'd much prefer to make
this a property of the access, even if that means adding another param or doing
something gross with @access (@pfec here).

>  	int index = (pfec >> 1) +
>  		    (not_smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
>  	bool fault = (mmu->permissions[index] >> pte_access) & 1;

Re: [PATCH 3/4] KVM: X86: Handle implicit supervisor access with SMAP

[Lai Jiangshan]

On 2021/12/8 05:52, Sean Christopherson wrote:

> 
>> +	 *
>> +	 * This computes explicit_access && (rflags & X86_EFLAGS_AC), leaving
> 
> Too many &&, the logic below is a bitwise &, not a logical &&.

The intended logic is "explicit_access &&" ("logic and") and the code ensures
explicit_access has the bit X86_EFLAGS_AC and morphs it into "&" ("binary and")
below to achieve branchless.

Original comments is "(cpl < 3) &&", and it is morphed into "(cpl - 3) &" in
code to achieve branchless.

The comments is bad in this patch, it should have stated that the logic operations
on the conditions are changed to use "binary and" to achieve branchless.

> 
>>   	 * the result in X86_EFLAGS_AC. We then insert it in place of
>>   	 * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
>>   	 * but it will be one in index if SMAP checks are being overridden.
>>   	 * It is important to keep this branchless.
> 
> Heh, so important that it incurs multiple branches and possible VMREADs in
> vmx_get_cpl() and vmx_get_rflags().  And before static_call, multiple retpolines
> to boot.  Probably a net win now as only the first permission_fault() check for
> a given VM-Exit be penalized, but the comment is amusing nonetheless.
> 
>>   	 */
>> -	unsigned long not_smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
>> +	u32 not_smap = (rflags & X86_EFLAGS_AC) & vcpu->arch.explicit_access;
> 
> I really, really dislike shoving this into vcpu->arch.  I'd much prefer to make
> this a property of the access, even if that means adding another param or doing
> something gross with @access (@pfec here).

En, it taste bad to add a variable in vcpu->arch for a scope-like condition.
I will do as you suggested.

> 
>>   	int index = (pfec >> 1) +
>>   		    (not_smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
>>   	bool fault = (mmu->permissions[index] >> pte_access) & 1;

Re: [PATCH 3/4] KVM: X86: Handle implicit supervisor access with SMAP

[Paolo Bonzini]

On 12/7/21 22:52, Sean Christopherson wrote:
>> -	unsigned long not_smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
>> +	u32 not_smap = (rflags & X86_EFLAGS_AC) & vcpu->arch.explicit_access;
> I really, really dislike shoving this into vcpu->arch.  I'd much prefer to make
> this a property of the access, even if that means adding another param or doing
> something gross with @access (@pfec here).
> 

Well, we already have something gross going on with the pfec.  Maybe we 
should add separate constants for the index into mmu->permissions.

Paolo

[PATCH 4/4] KVM: X86: Only get rflags when needed in permission_fault()

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

In same cases, it doesn't need to get rflags for SMAP checks.

For example: it is user mode access, it could have contained other
permission fault, SMAP is not enabled, it is implicit supervisor
access, or it is nested TDP pagetable.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kvm/mmu.h | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 0cb2c52377c8..70ab6e392f18 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -252,8 +252,6 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 				  unsigned pte_access, unsigned pte_pkey,
 				  unsigned pfec)
 {
-	unsigned long rflags = static_call(kvm_x86_get_rflags)(vcpu);
-
 	/*
 	 * If explicit supervisor accesses, SMAP is disabled
 	 * if EFLAGS.AC = 1.
@@ -261,22 +259,34 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 	 * If implicit supervisor accesses, SMAP can not be disabled
 	 * regardless of the value EFLAGS.AC.
 	 *
-	 * SMAP works on supervisor accesses only, and not_smap can
+	 * SMAP works on supervisor accesses only, and SMAP checking bit can
 	 * be set or not set when user access with neither has any bearing
 	 * on the result.
 	 *
-	 * This computes explicit_access && (rflags & X86_EFLAGS_AC), leaving
-	 * the result in X86_EFLAGS_AC. We then insert it in place of
-	 * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
-	 * but it will be one in index if SMAP checks are being overridden.
-	 * It is important to keep this branchless.
+	 * We put the SMAP checking bit in place of the PFERR_RSVD_MASK bit;
+	 * this bit will always be zero in pfec, but it will be one in index
+	 * if SMAP checks are being disabled.
 	 */
-	u32 not_smap = (rflags & X86_EFLAGS_AC) & vcpu->arch.explicit_access;
-	int index = (pfec >> 1) +
-		    (not_smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
-	bool fault = (mmu->permissions[index] >> pte_access) & 1;
+	u32 fault = (mmu->permissions[pfec >> 1] >> pte_access) & 1;
+	int index = (pfec + PFERR_RSVD_MASK) >> 1;
+	u32 fault_not_smap = (mmu->permissions[index] >> pte_access) & 1;
 	u32 errcode = PFERR_PRESENT_MASK;
 
+	/*
+	 * fault	fault_not_smap
+	 * 0		0		not fault here
+	 * 0		1		impossible combination
+	 * 1		0		check if implicit access or EFLAGS.AC
+	 * 1		1		fault with non-SMAP permission fault
+	 *
+	 * It is common fault == fault_not_smap, and they are always
+	 * equivalent when SMAP is not enabled.
+	 */
+	if (unlikely(fault & ~fault_not_smap & vcpu->arch.explicit_access)) {
+		unsigned long rflags = static_call(kvm_x86_get_rflags)(vcpu);
+		fault = (rflags ^ X86_EFLAGS_AC) >> X86_EFLAGS_AC_BIT;
+	}
+
 	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
 	if (unlikely(mmu->pkru_mask)) {
 		u32 pkru_bits, offset;
-- 
2.19.1.6.gb485710b

Re: [PATCH 4/4] KVM: X86: Only get rflags when needed in permission_fault()

[Sean Christopherson]

On Tue, Dec 07, 2021, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> In same cases, it doesn't need to get rflags for SMAP checks.
> 
> For example: it is user mode access, it could have contained other
> permission fault, SMAP is not enabled, it is implicit supervisor
> access, or it is nested TDP pagetable.

I don't disagree that reading RFLAGS is silly and _may_ have worse performance,
but I'd prefer any change have actual numbers to justify that it's an improvement
or at least a wash / in the noise.

Too much of the MMU code (and KVM in general) has optimizations like this that
have bitrotted horribly over the years.  And in many/most cases, the original commit
didn't provide performance numbers, so it's not even clear that the "optimizations"
were _ever_ a net win.