Messages in this thread Patch in this message | | | Date | Tue, 25 Apr 2023 11:10:16 +0800 | From | Chao Gao <> | Subject | Re: [PATCH 2/6] KVM: VMX: Add new ops in kvm_x86_ops for LASS violation check |
| |
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 >
| |