Re: [PATCH v7 041/102] KVM: VMX: Introduce test mode related to EPT violation VE

[Isaku Yamahata]

On Thu, Jul 28, 2022 at 08:11:59PM +0000,
Sean Christopherson <seanjc@xxxxxxxxxx> wrote:

> On Thu, Jul 28, 2022, Kai Huang wrote:
> > On Wed, 2022-07-27 at 16:39 -0700, Isaku Yamahata wrote:
> > > On Wed, Jul 20, 2022 at 05:13:08PM +1200,
> > > Kai Huang <kai.huang@xxxxxxxxx> wrote:
> > > 
> > > > On Tue, 2022-07-19 at 07:49 -0700, Isaku Yamahata wrote:
> > > > > On Fri, Jul 08, 2022 at 02:23:43PM +1200,
> > > > > Kai Huang <kai.huang@xxxxxxxxx> wrote:
> > > > > 
> > > > > > On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@xxxxxxxxx wrote:
> > > > > > > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > > > > > > 
> > > > > > > To support TDX, KVM is enhanced to operate with #VE.  For TDX, KVM programs
> > > > > > > to inject #VE conditionally and set #VE suppress bit in EPT entry.  For VMX
> > > > > > > case, #VE isn't used.  If #VE happens for VMX, it's a bug.  To be
> > > > > > > defensive (test that VMX case isn't broken), introduce option
> > > > > > > ept_violation_ve_test and when it's set, set error.
> > > > > > 
> > > > > > I don't see why we need this patch.  It may be helpful during your test, but why
> > > > > > do we need this patch for formal submission?
> > > > > > 
> > > > > > And for a normal guest, what prevents one vcpu from sending #VE IPI to another
> > > > > > vcpu?
> > > > > 
> > > > > Paolo suggested it as follows.  Maybe it should be kernel config.
> > > > > (I forgot to add suggested-by. I'll add it)
> > > > > 
> > > > > https://lore.kernel.org/lkml/84d56339-4a8a-6ddb-17cb-12074588ba9c@xxxxxxxxxx/
> > > > > 
> > > > > > 
> > > > 
> > > > OK.  But can we assume a normal guest won't sending #VE IPI?
> > > 
> > > Theoretically nothing prevents that.  I wouldn't way "normal".
> > > Anyway this is off by default.
> > 
> > I don't think whether it is on or off by default matters.
> 
> It matters in the sense that the module param is intended purely for testing, i.e.
> there's zero reason to ever enable it in production.  That changes what is and
> wasn't isn't a reasonable response to an unexpected #VE.
> 
> > If it can happen legitimately in the guest, it doesn't look right to print
> > out something like below:
> > 
> > 	pr_err("VMEXIT due to unexpected #VE.\n");
> 
> Agreed.  In this particular case I think the right approach is to treat an
> unexpected #VE as a fatal KVM bug.  Yes, disabling EPT violation #VEs would likely
> allow the guest to live, but as above the module param should never be enabled in
> production.  And if we get a #VE with the module param disabled, then KVM is truly
> in the weeds and killing the VM is the safe option.
> 
> E.g. something like

Thanks, I finally ended up with the following.

diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index ac290a44a693..9277676057a7 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -140,6 +140,11 @@ static inline bool is_nm_fault(u32 intr_info)
 	return is_exception_n(intr_info, NM_VECTOR);
 }
 
+static inline bool is_ve_fault(u32 intr_info)
+{
+	return is_exception_n(intr_info, VE_VECTOR);
+}
+
 /* Undocumented: icebp/int1 */
 static inline bool is_icebp(u32 intr_info)
 {
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 881db80ceee9..c3e4c0d17b63 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5047,6 +5047,12 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 	if (is_invalid_opcode(intr_info))
 		return handle_ud(vcpu);
 
+	/*
+	 * #VE isn't supposed to happen.  Although vcpu can send
+	 */
+	if (KVM_BUG_ON(is_ve_fault(intr_info), vcpu->kvm))
+		return -EIO;
+
 	error_code = 0;
 	if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
 		error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
@@ -5167,14 +5173,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 		if (handle_guest_split_lock(kvm_rip_read(vcpu)))
 			return 1;
 		fallthrough;
-	case VE_VECTOR:
 	default:
-		if (ept_violation_ve_test && ex_no == VE_VECTOR) {
-			pr_err("VMEXIT due to unexpected #VE.\n");
-			secondary_exec_controls_clearbit(
-				vmx, SECONDARY_EXEC_EPT_VIOLATION_VE);
-			return 1;
-		}
 		kvm_run->exit_reason = KVM_EXIT_EXCEPTION;
 		kvm_run->ex.exception = ex_no;
 		kvm_run->ex.error_code = error_code;



-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>