Re: [PATCH 2/6] KVM: VMX: Add new ops in kvm_x86_ops for LASS violation check

[Chao Gao <chao.gao@intel.com>]

On Thu, Apr 20, 2023 at 09:37:20PM +0800, Zeng Guang wrote:
>+/*
>+ * Determine whether an access to the linear address causes a LASS violation.
>+ * LASS protection is only effective in long mode. As a prerequisite, caller
>+ * should make sure VM running in long mode and invoke this api to do LASS
>+ * violation check.

Could you place the comment above vmx_check_lass()?

And for __vmx_check_lass(), just add:

A variant of vmx_check_lass() without the check for long mode.

>+ */
>+bool __vmx_check_lass(struct kvm_vcpu *vcpu, u64 access, u64 la, u64 flags)
>+{
>+	bool user_mode, user_as, rflags_ac;
>+
>+	if (!!(flags & KVM_X86_EMULFLAG_SKIP_LASS) ||
>+	    !kvm_is_cr4_bit_set(vcpu, X86_CR4_LASS))
>+		return false;
>+
>+	WARN_ON_ONCE(!is_long_mode(vcpu));
>+
>+	user_as = !(la >> 63);
>+


>+	/*
>+	 * An access is a supervisor-mode access if CPL < 3 or if it implicitly
>+	 * accesses a system data structure. For implicit accesses to system
>+	 * data structure, the processor acts as if RFLAGS.AC is clear.
>+	 */
>+	if (access & PFERR_IMPLICIT_ACCESS) {
>+		user_mode = false;
>+		rflags_ac = false;
>+	} else {
>+		user_mode = vmx_get_cpl(vcpu) == 3;
>+		if (!user_mode)
>+			rflags_ac = !!(kvm_get_rflags(vcpu) & X86_EFLAGS_AC);
>+	}
>+
>+	if (user_mode != user_as) {

to reduce one level of indentation, how about:

	if (user_mode == user_as)
		return false;

	/*
	 * Supervisor-mode _data_ accesses to user address space
	 * cause LASS violations only if SMAP is enabled.
	 */
	if (!user_mode && !(access & PFERR_FETCH_MASK)) {
		return kvm_is_cr4_bit_set(vcpu, X86_CR4_SMAP) && !rflags_ac;

	return true;


>+		/*
>+		 * Supervisor-mode _data_ accesses to user address space
>+		 * cause LASS violations only if SMAP is enabled.
>+		 */
>+		if (!user_mode && !(access & PFERR_FETCH_MASK)) {
>+			return kvm_is_cr4_bit_set(vcpu, X86_CR4_SMAP) &&
>+			       !rflags_ac;
>+		} else {
>+			return true;
>+		}
>+	}
>+
>+	return false;
>+}
>+
>+static bool vmx_check_lass(struct kvm_vcpu *vcpu, u64 access, u64 la, u64 flags)
>+{
>+	return is_long_mode(vcpu) && __vmx_check_lass(vcpu, access, la, flags);

Why not request all callers to check if vcpu is in long mode?

e.g.,
	return is_long_mode(vcpu) && static_call(kvm_x86_check_lass)(...);

then you can rename __vmx_check_lass() to vmx_check_lass() and drop the
original one.

>+}
>+
> static struct kvm_x86_ops vmx_x86_ops __initdata = {
> 	.name = "kvm_intel",
> 
>@@ -8207,6 +8260,8 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
> 	.complete_emulated_msr = kvm_complete_insn_gp,
> 
> 	.vcpu_deliver_sipi_vector = kvm_vcpu_deliver_sipi_vector,
>+
>+	.check_lass = vmx_check_lass,
> };
> 
> static unsigned int vmx_handle_intel_pt_intr(void)
>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>index a3da84f4ea45..6569385a5978 100644
>--- a/arch/x86/kvm/vmx/vmx.h
>+++ b/arch/x86/kvm/vmx/vmx.h
>@@ -433,6 +433,8 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type);
> u64 vmx_get_l2_tsc_offset(struct kvm_vcpu *vcpu);
> u64 vmx_get_l2_tsc_multiplier(struct kvm_vcpu *vcpu);
> 
>+bool __vmx_check_lass(struct kvm_vcpu *vcpu, u64 access, u64 la, u64 flags);
>+

no one uses this function. You can defer exporting it to when the first
external caller is added.

> static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> 					     int type, bool value)
> {
>-- 
>2.27.0
>